jquery / jquery

jQuery JavaScript Library
https://jquery.com
MIT License
58.93k stars 20.62k forks source link

Memory Leak: boxSizingReliable #5458

Closed melloware closed 1 month ago

melloware commented 1 month ago

Description

Loading vanilla jQuery 3.7.1 using the Chrome Heap Snapshot tool its is reported detached Dom elements for the container and div elements.

image

This is due to the elements being initialized immeidately.

    var pixelPositionVal, boxSizingReliableVal, scrollboxSizeVal, pixelBoxStylesVal,
        reliableTrDimensionsVal, reliableMarginLeftVal,
        container = document.createElement( "div" ),
        div = document.createElement( "div" );

But never cleaned up until computeStyleTests(); is called which looks like it is done lazily and may NEVER be called by an app.

    jQuery.extend( support, {
        boxSizingReliable: function() {
            computeStyleTests();
            return boxSizingReliableVal;
        },

Solution

Either lazily created those DIVS or just call computeStyleTests(); immediately instead of lazily.

Also the container must be NULLED when the div is nulled.

        // Nullify the div so it wouldn't be stored in the memory and
        // it will also be a sign that checks already performed
        container = null;
        div = null;
timmywil commented 1 month ago

Thanks for opening an issue. As noted in your other issue, retaining something this small should not cause problems. Intentionally retaining a variable is not the same as a memory leak. A memory leak is when a particular operation continues to increase memory, but here there is only ever one element retained. The question is, does this cause a real world issue? It's so small I don't see how.

Anyway, the container element is no longer on the main branch. There is still a div and it is no longer nulled, but its purpose goes a bit beyond the support test.

melloware commented 1 month ago

Understood but in chrome it forces a whole tree of things to be retained according to the heap dump.

mgol commented 1 month ago

@melloware can you provide more details about this tree?

We’re all for solving real memory issues but we first need to know there’s a real one.

BTW, we cannot call computeStyleTests() eagerly; we spent a lot of effort in jQuery 1.11/2.1 to make sure support tests that trigger layout are done lazily for performance reasons.

melloware commented 1 month ago

@mgol i totally understand. I think locally I just need to build a debug Jquery version that does this. I came to this because I am helping a client debug client side memory leaks and their Detached DOM keeps growing so I am trying to filter out real issues from noise.

No worries leave this closed and thanks for the responsiveness!