New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 906847 link

Starred by 3 users

console.log(window -or- [native code]) resulting in "Detached Window" in Heap Snapshot, after page reload (apparent memory leak).

Reported by dal...@gmail.com, Nov 19

Issue description

UserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/70.0.3538.102 Safari/537.36

Steps to reproduce the problem:
1. index.html:
<script>
  console.log("ResizeObserver", ResizeObserver)
</script> 
2. Open index.html
3. Open DevTools
4. Take Heap Snapshot #1
5. Reload
6. Take Heap Snapshot #2

What is the expected behavior?
Expect Heap Snapshot #1 and #2 to be ~identical (in total size)
Expect no detached elements in either

What went wrong?
Heap Snapshot #2 is showing Detached Window (~78k), along with other DetachedHTML* entries

Overall Heap Memory goes from 5.9MB to 6.2MB

Did this work before? Yes 70.0.3538.102

Chrome version: 71.0.3578.53  Channel: beta
OS Version: 10.0
Flash Version: 

Reloading page multiple times doesn't seem to create additional "detached" elements (just happens once).

Issue doesn't seem to occur if DevTools is not open (e.g. don't open DevTools, open index.html & reload multiple times, then open DevTools and snapshot --> no detached elements).

Clearing console or manual GC before running Heap Snapshot doesn't seem to help.

The size of the leak ~proportional to how large your window object is (in our application, was ~10MB).

Simple workaround is to avoid logging native / window objects to console; and this should not be affecting in-production code (only happens when DevTools open)
 
2018-11-19 23_04_38-Memory Profiling - OneNote.png
54.6 KB View Download
Seems that invoking other DevTools operations, which will want to access window, also cause this to happen.
e.g. (see attached), debugging Heap Usage by examining Detached Nodes. 
1. When click on the "Detached HTMLDivElement", the "Retainers" view is populated (presumably, this will have references to window)
2. Reload page
3. Take another Heap Snapshot --> "Detached Window" (as above..)

(This is a little more frustrating than original bug report. I don't see an easy workaround in this case. And it makes it pretty much impossible to debug Detached DOM nodes if the debugging causes the more detached nodes..)

As above, this is not reproducible in 70.0.3538.102
2018-11-20 01_33_17-Memory Profiling - OneNote.png
69.1 KB View Download
Labels: Needs-Triage-M71 Needs-Bisect
Cc: swarnasree.mukkala@chromium.org
Labels: Triaged-ET Needs-Feedback
Tried testing the issue on reported chrome version #71.0.3578.53 using Windows 10 y following below steps.

Steps:
=====
1.Launched chrome.
2.Copied code snippet (from comment#0 Step1) and saved it as "index.html".
3.Opened "index.html" file.
4.Opened devtool->Memory and took a Snapshot.
5.Reloaded the page and took another snapshot
6.Observed that both snapshots are identical in total size and unable to see any detached elements in either of the snapshots.

Attached screencast for reference.
@reporter: Could you please review attached screencast and let us know if anything is being missed here. Request you to retry the issue by creating a new person without any apps and extensions in it, reset all flags to default and let us if the issue still exists.

Thanks.!
906847.mp4
7.2 MB View Download
OK, it seems there is an additional step required to reproduce:

Steps:
=====
1.Launched chrome.
2.Copied code snippet (from comment#0 Step1) and saved it as "index.html".
3.Opened "index.html" file.
4.Opened devtool->Memory and took a Snapshot.

** Open the "Console" tab in DevTools (click Console tab, or hit "esc") ** 

5.Reloaded the page and took another snapshot

(I guess DevTools might not execute the ```console.log("ResizeObserver", ResizeObserver)``` unless the Console view has actually been activated)
Project Member

Comment 5 by sheriffbot@chromium.org, Nov 20

Labels: -Needs-Feedback
Thank you for providing more feedback. Adding the requester to the cc list.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Pri-2 -Needs-Bisect ReleaseBlock-Stable Target-71 Target-72 M-72 M-71 RegressedIn-71 FoundIn-71 FoundIn-72 hasbisect-per-revision OS-Linux OS-Mac Pri-1
Owner: mlippautz@chromium.org
Status: Assigned (was: Unconfirmed)
Able to reproduce the issue on reported chrome version #71.0.3578.53 and latest chrome #72.0.3616.0 by following steps as per comment#4 using Windows 10, Ubuntu 17.10 and Mac OS 10.13.6. The following is the bisect information.

Bisect information:
===================
Good Build: 71.0.3558.0
Bad Build: 71.0.3559.0

Change Log: https://chromium.googlesource.com/chromium/src/+log/758685cf3c66f1ec376744aacef8b3f46f040149..99ff5481f60de0c8791c465b8553aa967bd71461
v8-ci-autoroll-builder Change Log: https://chromium.googlesource.com/v8/v8/+log/0664f0cf..980ce54f
Reviwed on: https://chromium-review.googlesource.com/1236253

@Michael Lippautz: Please help us in re-assigning if this is not related to your change.
Adding Release-Block-Stable as this is a recent regression. Please feel free to remove if not applicable.

Thanks.!
Cc: pbomm...@chromium.org hablich@chromium.org
Reminder M71 Stable is coming VERY soon. Please review this bug and assess if this is indeed a RBS. If not, please remove the RBS label. If yes, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.

+hablich@ (V8 TPM)

Cc: mlippautz@chromium.org
Owner: u...@chromium.org
Components: Blink>JavaScript>GC
Cc: u...@chromium.org
Owner: mvstan...@chromium.org
Cc: dgozman@chromium.org
Owner: alph@chromium.org
Thanks for the report. The key part is that the leak happens only when DevTools is open.

> Simple workaround is to avoid logging native / window objects to console; and this should not be affecting in-production code (only happens when DevTools open)

alph@, dgozman@, any ideas how to avoid memory leak via DevTools console?
There were similar reports in the past. This issue is confusing users that are trying to debug memory leaks.


Cc: pfeldman@chromium.org
+ pfeldman@ (Devtools TL)
Clearing console should clear any references, so that objects can be garbage collected. If it doesn't, we have a bug somewhere.
Friendly ping to look into this issue and to provide further update on this issue as it has been marked as a stable blocker.

Thanks!
M71 Stable promotion is coming VERY soon. Your bug is labelled as Stable  ReleaseBlock, pls make sure to land the fix and request a merge into the release branch ASAP. Thank you.
Owner: u...@chromium.org
I bisected it down to this patch https://chromium.googlesource.com/v8/v8/+/d9fbfeb8946a91a050e4e996bd5389c1f5be49e2

However I was not able to find any flaws in it. The patch introduces a debugger internal WeakMap where it stores objects as keys. Particularly ResizeObserver constructor from the original testcase.
The entry should be removed by GC once the page is reloaded and ResizeObserver is gone, but it is not.

Ulan do you have an idea why's that?
Owner: yangguo@chromium.org
Thanks alph@ for bisecting. I investigated with retaining path tracing and see that the native context of the previous page is kept alive by the WeakMap's hidden class:

WeakMap => hidden class of the WeakMap => constructor function => native context => SimpleNumberDictionary => ResizeObserver

The full path is attached.

So the CL indeed has a memory leak. If possible it should be reverted and re-landed with another weakness mechanism (not JSWeakMap). I am happy to help with implementing internal weak hash table that doesn't keep other objects.

Assigning to Yang to make the decision regarding the revert of the CL.

retaining-path-weak-map.txt
54.6 KB View Download
Hmm, I'd like to clarify.
Do you say that JSWeakMap works as intended, by strongly retaining its keys? I'd expect it to retain them as weak references. Am I wrong?
Keys and values of JSWeakMap are weak as expected.

It is the hidden class (a.k.a map) of the JSWeakMap that is strong and it retains the constructor function of JSWeakMap, which retains the native context of the previous page.
Labels: -ReleaseBlock-Stable -Target-71 -Target-72 Target-73
I don't think this is bad enough to justify release blocker.
Owner: alph@chromium.org
Ulan, thanks for clarification. I'll take the issue back as it seems to be an issue with debugger.
Cc: viswa.karala@chromium.org
 Issue 907955  has been merged into this issue.
I saw a variant of what Ulan mentions above today when debugging a Webassembly OOM issue on page reload. In essence, I saw a retaining path from a weak map to ultimately a webassembly memory object. This made no sense to me but fortunately I have access to Ulan, so I could make sense of this. 

For mere mortals, this bug breaks heap profiling across reloads. In particular, if people are not aware of the subtleties around reload and new tab, this is super confusing.
Project Member

Comment 25 by bugdroid1@chromium.org, Dec 6

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

commit ae6a568a8b488d45c718fb71cd84bcce29b1c4c9
Author: Dmitry Gozman <dgozman@chromium.org>
Date: Thu Dec 06 00:58:32 2018

[DevTools] Disable some tests until v8 roll

TBR=alph@chromium.org

Bug:  906847 
Change-Id: I4a1038896900fedd1737b10b865b3dad29bdf1ca
Reviewed-on: https://chromium-review.googlesource.com/c/1363845
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Commit-Queue: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#614206}
[modify] https://crrev.com/ae6a568a8b488d45c718fb71cd84bcce29b1c4c9/third_party/blink/web_tests/TestExpectations

Project Member

Comment 26 by bugdroid1@chromium.org, Dec 8

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

commit c198a9b497b0cbf4082c2b213bcd9b2fa972abc1
Author: Alexei Filippov <alph@chromium.org>
Date: Sat Dec 08 00:05:01 2018

[inspector] Move m_internalObjects into InspectedContext.

That should prevent leak of objects when page is reloaded.

BUG= chromium:906847 

Change-Id: I90928a5c4979c0ddc01c201bf60a693e2b03863a
Reviewed-on: https://chromium-review.googlesource.com/c/1366449
Commit-Queue: Alexei Filippov <alph@chromium.org>
Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
Cr-Commit-Position: refs/heads/master@{#58110}
[modify] https://crrev.com/c198a9b497b0cbf4082c2b213bcd9b2fa972abc1/src/inspector/inspected-context.cc
[modify] https://crrev.com/c198a9b497b0cbf4082c2b213bcd9b2fa972abc1/src/inspector/inspected-context.h
[modify] https://crrev.com/c198a9b497b0cbf4082c2b213bcd9b2fa972abc1/src/inspector/v8-debugger.cc
[modify] https://crrev.com/c198a9b497b0cbf4082c2b213bcd9b2fa972abc1/src/inspector/v8-debugger.h
[modify] https://crrev.com/c198a9b497b0cbf4082c2b213bcd9b2fa972abc1/src/inspector/value-mirror.cc

Blocking: 909723
Status: Fixed (was: Assigned)
Thanks for fixing! Is it feasible to back merge to M72 & M71?

Labels: Merge-Request-72
This is the main fix:

    Revert "inspector: return [[StableObjectId]] as internal property"
    
    This reverts commit d9fbfeb8946a91a050e4e996bd5389c1f5be49e2.
    
    Reason for revert: see bug.
    Bug:  906847 
    
    Original change's description:
    > inspector: return [[StableObjectId]] as internal property
    >
    > This property might be useful for fast '===' check.
    >
    > R=<U+200B>dgozman@chromium.org,yangguo@chromium.org
    >
    > Bug: none
    > Cq-Include-Trybots: luci.chromium.try:linux_chromium_headless_rel;luci.chromium.try:linux_chromium_rel_ng;master.tryserver.blink:linux_trusty_blink_rel
    > Change-Id: Iabc3555ce1ec2c14cf0ccd40b7d964ae144e7352
    > Reviewed-on: https://chromium-review.googlesource.com/1226411
    > Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
    > Reviewed-by: Yang Guo <yangguo@chromium.org>
    > Reviewed-by: Jakob Gruber <jgruber@chromium.org>
    > Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
    > Cr-Commit-Position: refs/heads/master@{#56095}
    
    TBR=dgozman@chromium.org,yangguo@chromium.org,kozyatinskiy@chromium.org,jgruber@chromium.org
    
    # Not skipping CQ checks because original CL landed > 1 day ago.
    
    Bug: none
    Change-Id: I68c700b7b8fd0a015f099460c15665d74e4da183
    Cq-Include-Trybots: luci.chromium.try:linux_chromium_headless_rel;luci.chromium.try:linux_chromium_rel_ng;master.tryserver.blink:linux_trusty_blink_rel
    Reviewed-on: https://chromium-review.googlesource.com/c/1363558
    Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
    Reviewed-by: Alexei Filippov <alph@chromium.org>
    Cr-Commit-Position: refs/heads/master@{#58077}


Is this well tested in canary and how safe is this merge overall?
It is in canary for more than a week and it's a revert of a previous change, so I think it's safe. Btw, it's a merge to V8.
Project Member

Comment 34 by sheriffbot@chromium.org, Dec 12

Labels: -Merge-Request-72 Hotlist-Merge-Approved Merge-Approved-72
Your change meets the bar and is auto-approved for M72. Please go ahead and merge the CL to branch 3626 manually. Please contact milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

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

Comment 35 by sheriffbot@chromium.org, Dec 17

This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: Merge-Request-7.2
Labels: -Merge-Request-7.2 Merge-Request-72
Project Member

Comment 38 by sheriffbot@chromium.org, Dec 17

Labels: -Merge-Request-72 Merge-Review-72 Hotlist-Merge-Review
This bug requires manual review: M72 has already been promoted to the beta branch, so this requires manual review
Please contact the milestone owner if you have questions.
Owners: govind@(Android), kariahda@(iOS), djmm@(ChromeOS), abdulsyed@(Desktop)

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: -Merge-Review-72 Merge-Request-7.2
Merge-Request-7.2 label is for V8 review according to https://v8.dev/docs/merge-patch process.
Labels: -Hotlist-Merge-Review
Re #39, got it. hablich@ (V8 TPM) for M72 review.
Project Member

Comment 42 by bugdroid1@chromium.org, Dec 17

Labels: -merge-approved-72 merge-merged-3626
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/998bb8290579e506a4069079fb04c499be18dfec

commit 998bb8290579e506a4069079fb04c499be18dfec
Author: Alexei Filippov <alph@chromium.org>
Date: Mon Dec 17 18:37:16 2018

DevTools: Assign proper distances to non user reachable objects.

Do not assign zero distance to all GC root objects.
Instead assign zero to the global root and follow strong roots only
from there.

BUG= 906847 
TBR=alph@chromium.org

(cherry picked from commit 6befba52c6daa1ebf7b542859af8afe3e499d2fb)

Change-Id: Iee22162ac11a6700e019276daee1357ef33e61b9
Reviewed-on: https://chromium-review.googlesource.com/c/1353995
Commit-Queue: Alexei Filippov <alph@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#612438}
Reviewed-on: https://chromium-review.googlesource.com/c/1380531
Reviewed-by: Alexei Filippov <alph@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#392}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
[modify] https://crrev.com/998bb8290579e506a4069079fb04c499be18dfec/third_party/blink/renderer/devtools/front_end/heap_snapshot_worker/HeapSnapshot.js
[modify] https://crrev.com/998bb8290579e506a4069079fb04c499be18dfec/third_party/blink/web_tests/inspector-protocol/heap-profiler/heap-snapshot-merged-nodes-expected.txt
[modify] https://crrev.com/998bb8290579e506a4069079fb04c499be18dfec/third_party/blink/web_tests/inspector-protocol/heap-profiler/heap-snapshot-with-event-listener-expected.txt
[modify] https://crrev.com/998bb8290579e506a4069079fb04c499be18dfec/third_party/blink/web_tests/inspector-protocol/heap-profiler/heap-snapshot-with-multiple-retainers-expected.txt

Labels: -Merge-Request-7.2 merge-merged-7.2
Merge approved.
alph@, shouldn't we merge back [Revert "inspector: return [[StableObjectId]] as internal property"]?

The merged CL [DevTools: Assign proper distances to non user reachable objects.] is not going to fix the memory leak.
Yes, we should merge all three patches: 1 to chromium (merged already) and 2 to v8. I was waiting for Michael's approval for v8 merge.
Labels: -merge-merged-7.2 merge-approved-7.2
Labels: Merge-Merged-72-3626
The following revision refers to this bug: 
https://chromium.googlesource.com/chromium/src.git/+/998bb8290579e506a4069079fb04c499be18dfec

Commit: 998bb8290579e506a4069079fb04c499be18dfec
Author: alph@chromium.org
Commiter: alph@chromium.org
Date: 2018-12-17 18:37:16 +0000 UTC

DevTools: Assign proper distances to non user reachable objects.

Do not assign zero distance to all GC root objects.
Instead assign zero to the global root and follow strong roots only
from there.

BUG= 906847 
TBR=alph@chromium.org

(cherry picked from commit 6befba52c6daa1ebf7b542859af8afe3e499d2fb)

Change-Id: Iee22162ac11a6700e019276daee1357ef33e61b9
Reviewed-on: https://chromium-review.googlesource.com/c/1353995
Commit-Queue: Alexei Filippov <alph@chromium.org>
Reviewed-by: Ulan Degenbaev <ulan@chromium.org>
Cr-Original-Commit-Position: refs/heads/master@{#612438}
Reviewed-on: https://chromium-review.googlesource.com/c/1380531
Reviewed-by: Alexei Filippov <alph@chromium.org>
Cr-Commit-Position: refs/branch-heads/3626@{#392}
Cr-Branched-From: d897fb137fbaaa9355c0c93124cc048824eb1e65-refs/heads/master@{#612437}
Project Member

Comment 48 by bugdroid1@chromium.org, Dec 20

Labels: merge-merged-7.2
The following revision refers to this bug:
  https://chromium.googlesource.com/v8/v8.git/+/9310baccb0a0b6fcaeddf52c29d3b7da9496e813

commit 9310baccb0a0b6fcaeddf52c29d3b7da9496e813
Author: Alexei Filippov <alph@chromium.org>
Date: Thu Dec 20 02:09:05 2018

Merged: [inspector] Move m_internalObjects into InspectedContext.

Revision: c198a9b497b0cbf4082c2b213bcd9b2fa972abc1

BUG= chromium:906847 
LOG=N
NOTRY=true
NOPRESUBMIT=true
NOTREECHECKS=true
R=dgozman@chromium.org

Change-Id: I6a43d80a7a2f2cf65cff62f631055c258e61dd19
Reviewed-on: https://chromium-review.googlesource.com/c/1385168
Reviewed-by: Alexei Filippov <alph@chromium.org>
Cr-Commit-Position: refs/branch-heads/7.2@{#29}
Cr-Branched-From: 6acd03c9b8a8232aee95f25fbf6ae822aaedae75-refs/heads/7.2.502@{#1}
Cr-Branched-From: b03041de094610ef24e0e4fb6bf4c700fa1553ed-refs/heads/master@{#57910}
[modify] https://crrev.com/9310baccb0a0b6fcaeddf52c29d3b7da9496e813/src/inspector/inspected-context.cc
[modify] https://crrev.com/9310baccb0a0b6fcaeddf52c29d3b7da9496e813/src/inspector/inspected-context.h
[modify] https://crrev.com/9310baccb0a0b6fcaeddf52c29d3b7da9496e813/src/inspector/v8-debugger.cc
[modify] https://crrev.com/9310baccb0a0b6fcaeddf52c29d3b7da9496e813/src/inspector/v8-debugger.h
[modify] https://crrev.com/9310baccb0a0b6fcaeddf52c29d3b7da9496e813/src/inspector/value-mirror.cc

Everything is merged successfully.
Project Member

Comment 50 by sheriffbot@chromium.org, Dec 24

Cc: gov...@chromium.org
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible!

If all merges have been completed, please remove any remaining Merge-Approved labels from this issue.

Thanks for your time! To disable nags, add the Disable-Nags label.

For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
Labels: TE-Verified-M73 TE-Verified-73.0.3652.0
Able to reproduce the issue on reported chrome version #71.0.3578.53 Windows 10 by following steps as per comment#0 and comment#4.

Verified the fix on latest chrome #73.0.3652.0 using Mac OS 10.13.6, Windows 10 and Ubuntu 17.10.
Attaching screencast for reference.
Observed the Heap Snapshot #1 and #2 are identical and without any detached elements in them.
Hence, the fix is working as expected.
Adding the verified labels.

Thanks...!!
906847_M73.mp4
2.5 MB View Download

Sign in to add a comment