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)
,
Nov 20
,
Nov 20
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.!
,
Nov 20
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)
,
Nov 20
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
,
Nov 21
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.!
,
Nov 21
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)
,
Nov 21
,
Nov 21
,
Nov 21
,
Nov 21
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.
,
Nov 21
+ pfeldman@ (Devtools TL)
,
Nov 21
Clearing console should clear any references, so that objects can be garbage collected. If it doesn't, we have a bug somewhere.
,
Nov 23
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!
,
Nov 25
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.
,
Nov 27
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?
,
Nov 27
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.
,
Nov 27
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?
,
Nov 27
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.
,
Nov 27
I don't think this is bad enough to justify release blocker.
,
Nov 29
Ulan, thanks for clarification. I'll take the issue back as it seems to be an issue with debugger.
,
Nov 29
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6befba52c6daa1ebf7b542859af8afe3e499d2fb commit 6befba52c6daa1ebf7b542859af8afe3e499d2fb Author: Alexei Filippov <alph@chromium.org> Date: Thu Nov 29 23:44:14 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 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-Commit-Position: refs/heads/master@{#612438} [modify] https://crrev.com/6befba52c6daa1ebf7b542859af8afe3e499d2fb/third_party/blink/renderer/devtools/front_end/heap_snapshot_worker/HeapSnapshot.js [modify] https://crrev.com/6befba52c6daa1ebf7b542859af8afe3e499d2fb/third_party/blink/web_tests/inspector-protocol/heap-profiler/heap-snapshot-merged-nodes-expected.txt [modify] https://crrev.com/6befba52c6daa1ebf7b542859af8afe3e499d2fb/third_party/blink/web_tests/inspector-protocol/heap-profiler/heap-snapshot-with-event-listener-expected.txt [modify] https://crrev.com/6befba52c6daa1ebf7b542859af8afe3e499d2fb/third_party/blink/web_tests/inspector-protocol/heap-profiler/heap-snapshot-with-multiple-retainers-expected.txt
,
Dec 4
,
Dec 5
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.
,
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
,
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
,
Dec 11
,
Dec 11
,
Dec 11
Thanks for fixing! Is it feasible to back merge to M72 & M71?
,
Dec 11
,
Dec 11
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}
,
Dec 12
Is this well tested in canary and how safe is this merge overall?
,
Dec 12
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.
,
Dec 12
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
,
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
,
Dec 17
,
Dec 17
,
Dec 17
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
,
Dec 17
Merge-Request-7.2 label is for V8 review according to https://v8.dev/docs/merge-patch process.
,
Dec 17
,
Dec 17
Re #39, got it. hablich@ (V8 TPM) for M72 review.
,
Dec 17
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
,
Dec 18
Merge approved.
,
Dec 18
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.
,
Dec 18
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.
,
Dec 19
,
Dec 19
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}
,
Dec 20
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
,
Dec 20
Everything is merged successfully.
,
Dec 24
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
,
Dec 26
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...!! |
|||||||||||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||||||||||
Comment 1 by dal...@gmail.com
, Nov 2069.1 KB
69.1 KB View Download