Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Datepicker Memory Leak #2280

Open
porterclev opened this issue Aug 13, 2024 · 1 comment
Open

Datepicker Memory Leak #2280

porterclev opened this issue Aug 13, 2024 · 1 comment

Comments

@porterclev
Copy link
Contributor

Building off #2268, there is a problem when destroying the datepicker where it doesn’t completely get removed from memory. This pull request, 817ce38, removes the instance reference but the actual UI reference is tied to

this.dpDiv = datepicker_bindHover( $( "<div id='" + this._mainDivId + "' class='ui-datepicker ui-widget ui-widget-content ui-helper-clearfix ui-corner-all'></div>" ) );

When the widget is destroyed, the datepicker UI element remains in memory preventing it from closing. #2268 patches this issue by hiding the component, however dpDiv is still in memory. One solution could be to remove dpDiv from the instance and then set both _curInst = null and this.dpDiv = null.
5e4f1b8
However, many unit tests will break because they rely on dpDiv to still exist in memory, requiring the datepicker to be recreated for each test. There is also a problem that many developers rely on hiding the datepicker before destroying it $.datepicker("hide").destroy("destroy") which will fail because dpDiv will be null.

@mgol
Copy link
Member

mgol commented Aug 14, 2024

Thanks for the report.

PR #1362 fixing Trac issue #10668 and PR #1852 fixing Trac issue #15270 removed references to a Datepicker instance, which is way more than just the div representing the datepicker UI. Also, $.datepicker.dpDiv is created only once - and even if you load multiple versions of jQuery & jQuery UI on a page, the div is only attached to the document once. Thus, this issue is not fully comparable to the previous ones.

I think we'll just need to accept this dpDiv instance as an inherent part of Datepicker and its memory usage. There's no memory leak per se as creating multiple datepickers & destroying them will not build up memory from past destroyed date pickers. It's just maybe taking up a bit more memory that it could - but it's not even clear to me it's a bad thing as not having to recreate the datepicker container has its advantages as well.

One thing that could be considered is calling this.dpDiv.empty() when destroy is called on a date picker which was the most recently shown (or is still shown) using this global dpDiv, since we already empty it on showing here:

//to avoid flashes on Firefox
inst.dpDiv.empty();

so the previous contents would not be used anyway. We need to be careful here, though, to not delete contents meant for a different datepicker instance, especially when multiple jQuery UI versions are running on the same page.

I'd be willing to review a PR, but it might not be a trivial thing to do right.

(Since the issue is already in 1.12, given limited team resources it's not likely to be fixed by the UI team; see the project status at https://blog.jqueryui.com/2021/10/jquery-maintainers-update-and-transition-jquery-ui-as-part-of-overall-modernization-efforts/.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

2 participants