Reproduced on Windows 7 and Chrome 57.
Adding DOM and HTML since it might be .append() or .innerHTML, adding garbage collector since it might be Oilpan.

Also reproducible in Windos Server 2008, Linux debian based distros - Ubuntu and Linux Mint - and Mac OS X.

Also reproducible on Suse Leap 4.2

The original attached file (chrome-57-memory-leak-test-jquery-html.html) is using jQuery's .append().  

The one attached here (chrome57-leak.html) is a much more simplified version of what jQuery's .append is doing, and it uses pure DOM instead of jQuery.

The key is the line `elem.getElementsByTagName('script' || '*');`. Removing that gets rid of the leak.

Deleted: chrome57-leak.html 545 bytes

chrome57-leak.html 545 bytes View Download

That document fragment is causing many memory leaks across various contexts. :(
(Or two ;))

Sounds like cache for getElementsByTagName() isn't collected.

I tried bisect-builds.py and found that the range is between 439853 (known good)
and 439866 (known bad).
This is same as issue 701601, so this is quite probably the issue with
introducing TraceWrapper.
The difference is, that the fix for 701601 should be in for M58 and M59,
but this issue still reproduces on ToT build.
Adding Blink>Javascript.

Attaching my accelerated version of the repro script (original one is in
comment#5).
This replaces span with <input> (which uses more memory per node) and
repeat in 0.1sec (originally 1sec).

Deleted: chrome57-leak.html 545 bytes

chrome57-leak.html 545 bytes View Download

Michael, could you take a look?

 Issue 609137  may be another instance of this. I suggest you write a C++ version of this in a unit test and see if you can cause a leak without JavaScript.

Keishi-san, could you take a stab at this? This looks like more of an
Oilpan issue, rather than V8's GC.

Adding people interested in this issue in cc list.

Looks like a wrapper tracing issue. Assigning to mlippautz

I was able to reduce the test down to https://codereview.chromium.org/2805813003

The problem seems to be that live HTMLCollections is leaking.
According to devtools memory panel the retainer is HTMLDocument.
I don't know how trace wrapper works but I am suspecting that somehow Document::m_nodeLists's WeakMembers are traced as strong references in traceWrapper, which retains HTMLCollection wrapper, which retains HTMLCollection, which means it doesn't get removed from Document::m_nodeLists.

Thanks for the analysis, I'll follow up as soon as I have some cycles.

Keishi, your analysis looks correct. Wrapper tracing only knows about strong references, and we definitely trace Document::m_nodeLists [1]

haraken,jochen: Can you provide some guidance here? We usually try to avoid tracing references that are only held weakly in Oilpan, but this one was in before I started working on the project, so I guess there was a reason to do so.

[1]: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Document.cpp?q=Document.cpp&sq=package:chromium&dr&l=6592

Looks like the tracing was added in https://chromium.googlesource.com/chromium/src/+/a43e2359d03497a2a8a7e8327470abcf18eaa7d0

But I don't see how we would want to trace Document::m_nodeLists. They aren't JS exposed.

Could we just delete it like
https://codereview.chromium.org/2803313003

+haraken

The following tests failed when we don't trace Document::m_nodeLists
fast/dom/NodeList/nodelist-reachable.html
fast/dom/gc-9.html
fast/dom/htmlallcollection-reachable.html
fast/dom/htmlcollection-reachable.html

I can't find it written in the spec but it looks like we need to keep the LiveNodeList wrappers alive as long as the LiveNodeList::rootNode() element is alive.

I think the LiveNodeList::rootNode() should retain the LiveNodeList, and the LiveNodeList should retain its wrapper. But I can't figure out how to express that in TraceWrapper.

Yes. If we call:

  node.getElementsByTagName();

then we need to keep alive the node list while the node is alive. In other words, we need to trace from the node to the node list (*).

The problem is that we don't have a trivial reference from the node to the node list. That's why we're (intentionally) doing something tricky; i.e., strongly trace Document::m_nodeLists.

I'm a bit confused but is the node list really leaking? If we want to realize the behavior (*), we need to keep alive the node list while the node is alive. It's expected that the node list doesn't get collected until the node gets collected.

Another option would be to change the behavior (*). How do other browsers behave?

Looks like Firefox does not leak. Also Chrome didn't leak either prior to trace wrapper.

Would it be possible to instead have a weak callback (on Document) magically do some incremental wrapper tracing of the live NodeLists?

Let me ask the probably naive question here: Since m_nodeLists are weak for Oilpan anyways, what prohibits us from having a vector of wrapper tracing references on the actual nodes pointing to the corresponding node lists?

Ideally we would converge to a solution where the object graph is properly set up so regular parent/child relationships determine liveness.

#24: That's a very good question.

Sigbjorn: Do you know why we don't have a HeapMember<LiveNodeList> on NodeRareData? Then we can simply trace the node list without relying on the weak member on Document.

I think NodeLists are already owned by NodeListsNodeData, which is a part of NodeRareData.
Document::node_lists_ is used for DOMTreeVersion invalidation.

I now understand that it is wrong to wrapper-trace the node list from the document. If we do that, we end up with keeping the node list alive as long as the document is alive, not the node is alive. (i.e., even if we remove the node, the node list will be kept alive as long as the document is alive.) This leaks memory.

So, yes, we should wrapper-trace the node list from the node.

> I think NodeLists are already owned by NodeListsNodeData, which is a part of NodeRareData.

Thanks. Yeah, it makes more sense to trace NodeListsNodeData::child_node_list_.

By the way, do you know why NodeListsNodeData::child_node_list_ are weak members?

Do we already wrapper trace the WeakMember NodeListsNodeData::child_node_list_ as strong?
https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/NodeListsNodeData.cpp?rcl=f3e971b60293bb5f7e6bd8de03b7f3ea77a2be13&l=53

Yes we trace in NodeListsNodeData and as far as I understand it this should be enough. I would add some debugging information and check why we need to trace the node lists on the document.

I am not sure I will get to this issue this week though as we have a short week here (holiday) and I have other non-arguable deadlines coming up.

> Yes we trace in NodeListsNodeData and as far as I understand it this should be enough. I would add some debugging information and check why we need to trace the node lists on the document.

Sounds good to me!

Quick update: I had a look and we indeed seem to hold unnecessary HTMLCollection and LiveNodeList alive. The question now is how to avoid tracing through Document but rather trace through the Node.

NodeListsNodeData::child_node_list_ is just the list of children (as the name suggests). We have two more cashes though in NodeListsNodeData which are atomic_name_caches_ and tag_collection_cache_ns_. Will now have a look at those.

The problem seems to be indeed that we only trace wrappers for a subset of the lists on NodeListsNodeData. Potentialy fix is in  https://codereview.chromium.org/2809183003/

- I checked against the first two tests that were failing and they pass.
- I also checked against the minimized example. We hover somewhere around 40M of JS memory, depending on GC timing.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/76798dbd04a30a8e1acdf71ecab93d09dac4ec5b

commit 76798dbd04a30a8e1acdf71ecab93d09dac4ec5b
Author: mlippautz <mlippautz@chromium.org>
Date: Wed Apr 12 10:39:53 2017

[wrapper-tracing] Fix node list leak

Only trace node lists that hang of live nodes.

BUG= chromium:707544 

Review-Url: https://codereview.chromium.org/2809183003
Cr-Commit-Position: refs/heads/master@{#463986}

[modify] https://crrev.com/76798dbd04a30a8e1acdf71ecab93d09dac4ec5b/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/76798dbd04a30a8e1acdf71ecab93d09dac4ec5b/third_party/WebKit/Source/core/dom/NodeListsNodeData.cpp
[modify] https://crrev.com/76798dbd04a30a8e1acdf71ecab93d09dac4ec5b/third_party/WebKit/Source/core/dom/NodeListsNodeData.h

This should be fixed now and merged back to M58 once backed into Canary.

Your change meets the bar and is auto-approved for M58. Please go ahead and merge the CL to branch 3029 manually. Please contact milestone owner if you have questions.
Owners: amineer@(Android), cmasso@(iOS), bhthompson@(ChromeOS), govind@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot

How can I identify the M58 snapshots containing this fix?
https://storage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Win/

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb5051f1e67383a98cf36b21f7cf78a9a44e6301

commit fb5051f1e67383a98cf36b21f7cf78a9a44e6301
Author: Michael Lippautz <mlippautz@chromium.org>
Date: Thu Apr 13 11:34:41 2017

[wrapper-tracing] Fix node list leak

Only trace node lists that hang of live nodes.

BUG= chromium:707544 

Review-Url: https://codereview.chromium.org/2809183003
Cr-Commit-Position: refs/heads/master@{#463986}
(cherry picked from commit 76798dbd04a30a8e1acdf71ecab93d09dac4ec5b)

Review-Url: https://codereview.chromium.org/2810253003 .
Cr-Commit-Position: refs/branch-heads/3029@{#690}
Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471}

[modify] https://crrev.com/fb5051f1e67383a98cf36b21f7cf78a9a44e6301/third_party/WebKit/Source/core/dom/Document.cpp
[modify] https://crrev.com/fb5051f1e67383a98cf36b21f7cf78a9a44e6301/third_party/WebKit/Source/core/dom/NodeListsNodeData.cpp
[modify] https://crrev.com/fb5051f1e67383a98cf36b21f7cf78a9a44e6301/third_party/WebKit/Source/core/dom/NodeListsNodeData.h

"How can I identify the M58 snapshots containing this fix?
https://storage.googleapis.com/chromium-browser-snapshots/index.html?prefix=Win/"

LAST_CHANGE -> 464386

 Issue 711448  has been merged into this issue.

Tested the issue on windows 7, Ubuntu 14.04 and Mac 10.12.3 using chrome version 58.0.3029.81 with the below steps

1.Opened the attached chrome57-leak.html from comment #10.
2.Opened task manager
3.Observed memory increases and again freed which is not observed in M57.

Please find the attached screen cast for the reference.
Adding TE-Verified labels.

Thanks,

Deleted: 707544.mp4 939 KB

	707544.mp4 939 KB View Download

 Issue 704546  has been merged into this issue.

This was reported in M57 - should we merge it back there too?

#44 - Chrome 58 is a few days away, so Chrome 57 will not go to the public at this point.

See https://bugs.chromium.org/p/chromium/issues/detail?id=691298#c17