jquery / jquery

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

Memory Leak: OriginAnchor #5457

Closed melloware closed 1 month ago

melloware commented 1 month ago

Description

Running plain jQuery 3.7.1 and then do a Chrome Heap Snapshot. You will see the OriginAnchor is detached.

image

Reason

You are storing a ref to an anchor and constantly checking it.

// Anchor tag for parsing the document origin
originAnchor = document.createElement( "a" );

originAnchor.href = location.href;

s.crossDomain = originAnchor.protocol + "//" + originAnchor.host !==
                    urlAnchor.protocol + "//" + urlAnchor.host;

Solution

The solution is to store off the value you need and de-reference the originAnchor.

// Anchor tag for parsing the document origin
originAnchor = document.createElement( "a" );

originAnchor.href = location.href;
var originProtocolHost = originAnchor.protocol + "//" + originAnchor.host;
originAnchor = null;

s.crossDomain = originProtocolHost !== urlAnchor.protocol + "//" + urlAnchor.host;
mgol commented 1 month ago

Thanks for the report but I don’t understand the issue. Can you elaborate why the current behavior is problematic?

melloware commented 1 month ago

Well its a detached unused element reported by Chrome as a leak. Why keep an anchor around you don't need? I don't understand what you are asking that you don't understand?

The goal should be to never have detached DOM elements right?

mgol commented 1 month ago

It’s a single element, it doesn’t block a lot of memory. By keeping it, we avoid having to recreate one each time we need to normalize the href parts.

I understand you can find it in Chrome memory report but does it really cause any issue for the running website? Just seeing the node in a memory report is not a sufficient reason to change the code IMO.

timmywil commented 1 month ago

Thank you for your contribution, but I don't think a change is needed here. This is not a memory leak. See https://github.com/jquery/jquery/issues/5458#issuecomment-2022921263

melloware commented 1 month ago

Understood but since this value won't change ever why keep the anchor around and not just store the simple string seems like an odd design choice...

timmywil commented 1 month ago

That's a fair question. I think in this case the value you're referring to can change depending on the context so it needs to continue to reference the property on the element, rather than cache a previous value.