Issue metadata
Sign in to add a comment
|
hr elements decorated as cr-menu-item seem to be dropped during tracing. |
||||||||||||||||||||||
Issue descriptionThis issue was separated from Issue 689255 (Files app issue) to focus on the Blink issue. Chrome Version: ToT OS: Chrome OS What steps will reproduce the problem? (1) Revert 3bc177a7e5b7d15e6635613373ac316263576150 (workaround for the Files app issue 689255 ) (2) Open Files app (by Shift-Alt-M) (3) Click 3-dot setting icon at top-right corner (4) Click "New window" in the menu (5) Repeat (3)(4) for the newly created window repeatedly What is the expected result? Settings menu is shown in step 3. What happens instead? Sometimes clicking 3-dot icon does not show Settings menu. Console in devtools (Ctrl-Shift-J on Files app opens it) shows following error: Uncaught TypeError: menuItem.isSeparator is not a function at HTMLElement.updateCommands (main_scripts.js:11044) at HTMLButtonElement.showMenu (main_scripts.js:11335) at HTMLButtonElement.handleEvent (main_scripts.js:11264) Please use labels and text to provide additional information. This can reproduce on Chrome OS on Linux. How to build/run: https://chromium.googlesource.com/chromium/src/+/master/docs/old_chromeos_build_instructions.md I observed so far is: 1) <hr> elements in the dropdown menu are decorated (methods added) by cr.ui.MenuItem. A simplified code is as follows: var el = document.querySelector('hr'); el.__proto__ = cr.ui.MenuItem.prototype MenuItem.prototype is https://codesearch.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/menu_item.js?l=27 At this point, the <hr> element has a method in cr.ui.MenuItem.prototype. 2) At some point, the <hr> element loses the method in cr.ui.MenuItem.prototype. At this point, <hr> element's __proto__ is HTMLHRElement.prototype The timing seems random. 3) If we keep a reference to the <hr> element in JavaScript code, the <hr>'s __proto__ was not reset (as far as I checked so far) var el = document.querySelector('hr'); el.__proto__ = cr.ui.MenuItem.prototype window.foo = el; As a workaround which keeps meaningful reference to <hr>s, this did not work (presumably due to LiveNodeList). window.allHRs = document.querySelectorAll('hr'); The following worked. window.allHRs = [].slice.call(document.querySelectorAll('hr'))); mlippautz@ have further observations: https://bugs.chromium.org/p/chromium/issues/detail?id=689255#c17 1) If there's a proper reference from JS, e.g., regular Array or local variable, then we can never get rid of the wrapper. (LiveNodeList doesn't retain the wrappers though.) 2) Once we set a custom __proto__, the map gets changed and we should never be able to collect it based on Heap::IsUnmodifiedHeapObject, since we cannot just recreate the wrapper. My conclusion would be that there's some tracing missing. In theory, to repro this we would just need to run the file manager with --js-flags="--expose-gc" and trigger a gc() sometime after annotating the splitters with the custom proto. Any chance we can run the file manager on regular linux? https://bugs.chromium.org/p/chromium/issues/detail?id=689255#c20 Some observations: - V8's logic on dropping unmodified wrappers is fine. The bug even happens when turning off this optimization. - The code that is affected is in cr-menu and loops over all menu items [1]. menuItems is a getter that returns a LiveNodeList as it is using querySelectorAll [2]. The code looping crashes as the retrieved menuItem is null at some point, and we fail to continue to set up all the other menu items. I only have a rough understanding of this by my theory is that we access the m_collectionItemsCache in LiveNodeList [3] which is not valid after a GC. IIUC we should clear those caches, as fine-grained as possible, upon finalizing a wrapper tracing GC. Using object grouping this was not a problem as DOM trees were kept alive as a whole, not getting rid of wrappers on a fine-grained level. haraken and bindings folks: Does this make any sense? [1] https://codesearch.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/menu.js?l=291 [2] https://codesearch.chromium.org/chromium/src/ui/webui/resources/js/cr/ui/menu.js?l=148 [3] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/LiveNodeList.h?q=m_collectionItemsCache&sq=package:chromium&l=79 https://bugs.chromium.org/p/chromium/issues/detail?id=689255#c21 After some more digesting of this problem, I am not sure the problem is with LiveNodeList and the corresponding cache. Since we find an hr in the list but cannot call isSeparator, it seems that it's really "just" dropping an hr during tracing for whatever reason. Even when Heap::IsUnmodifiedHeapObject always returns false, the issue is reproducible.
,
Mar 20 2017
So I boiled this down to a missing write barrier. We converted all the Blink-side barriers to use the automatic barrier with TraceWrapperMember and *most* of the Blink->V8 entry to use TraceWrapperV8Reference. Unfortunately, we missed converting the most important one in ScriptWrappable which still is a regular weak Persistent without any write barrier. The bug happens when: (1) We create a SW (2) Do an incremental marking step marking the SW. There's no wrapper on the V8 side yet, so we skip that one. (3) We associate a wrapper and have no write barrier. (4) We prematurely collect the wrapper in V8. Short-term solution is to just emit a manual write barrier in ScriptWrappable::setWrapper. It's the only call that needs to be patched. This should be a small CL as we want to backmerge it. Medium-term, we should just make this a regular TraceWrapperV8Reference that automatically issues the write barrier.
,
Mar 20 2017
,
Mar 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a2011780ffefd6f9c120b80651015b3f6646244b commit a2011780ffefd6f9c120b80651015b3f6646244b Author: mlippautz <mlippautz@chromium.org> Date: Tue Mar 21 09:26:56 2017 [wrapper-tracing] Emite write barrier upon associating a wrapper Whenever we assign a reference that should be traced into V8 we need to emit a write barrier. We automatically emit write barriers for TraceWrapperv8Reference. However, we still use a regular (weak) Persistent in ScriptWrappable, so we need to emit the barrier there manually. A follow up should convert the reference in ScriptWrappable to TraceWrapperv8Reference. BUG= chromium:702490 , chromium:701601, chromium:468240 Review-Url: https://codereview.chromium.org/2759243002 Cr-Commit-Position: refs/heads/master@{#458357} [modify] https://crrev.com/a2011780ffefd6f9c120b80651015b3f6646244b/third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h [modify] https://crrev.com/a2011780ffefd6f9c120b80651015b3f6646244b/third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp [modify] https://crrev.com/a2011780ffefd6f9c120b80651015b3f6646244b/third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h [modify] https://crrev.com/a2011780ffefd6f9c120b80651015b3f6646244b/third_party/WebKit/Source/core/dom/MutationCallback.h
,
Mar 21 2017
Once this is through Canary, we should backmerge this fix.
,
Mar 21 2017
,
Mar 22 2017
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
,
Mar 23 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/5fe2bd0c1d820fed608f60f1e6dcda168e013bf7 commit 5fe2bd0c1d820fed608f60f1e6dcda168e013bf7 Author: Michael Lippautz <mlippautz@chromium.org> Date: Thu Mar 23 12:27:40 2017 [wrapper-tracing] Emite write barrier upon associating a wrapper Whenever we assign a reference that should be traced into V8 we need to emit a write barrier. We automatically emit write barriers for TraceWrapperv8Reference. However, we still use a regular (weak) Persistent in ScriptWrappable, so we need to emit the barrier there manually. A follow up should convert the reference in ScriptWrappable to TraceWrapperv8Reference. BUG= chromium:702490 Review-Url: https://codereview.chromium.org/2759243002 Cr-Commit-Position: refs/heads/master@{#458357} (cherry picked from commit a2011780ffefd6f9c120b80651015b3f6646244b) Review-Url: https://codereview.chromium.org/2769773003 . Cr-Commit-Position: refs/branch-heads/3029@{#381} Cr-Branched-From: 939b32ee5ba05c396eef3fd992822fcca9a2e262-refs/heads/master@{#454471} [modify] https://crrev.com/5fe2bd0c1d820fed608f60f1e6dcda168e013bf7/third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h [modify] https://crrev.com/5fe2bd0c1d820fed608f60f1e6dcda168e013bf7/third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp [modify] https://crrev.com/5fe2bd0c1d820fed608f60f1e6dcda168e013bf7/third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h [modify] https://crrev.com/5fe2bd0c1d820fed608f60f1e6dcda168e013bf7/third_party/WebKit/Source/core/dom/MutationCallback.h
,
Mar 29 2017
Ping: Release managers, this is potentially breaking websites as JavaScript objects get lost. It's not a security issues because the objects are dynamically replaced with a dummy but we still have unexpected behavior and silently failing JS.
,
Mar 30 2017
,
Mar 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/24f8804aa139486723395483bda7015d3c57e1b2 commit 24f8804aa139486723395483bda7015d3c57e1b2 Author: Michael Lippautz <mlippautz@chromium.org> Date: Thu Mar 30 14:11:16 2017 [wrapper-tracing] Emite write barrier upon associating a wrapper Whenever we assign a reference that should be traced into V8 we need to emit a write barrier. We automatically emit write barriers for TraceWrapperv8Reference. However, we still use a regular (weak) Persistent in ScriptWrappable, so we need to emit the barrier there manually. A follow up should convert the reference in ScriptWrappable to TraceWrapperv8Reference. BUG= chromium:702490 Review-Url: https://codereview.chromium.org/2759243002 Cr-Commit-Position: refs/heads/master@{#458357} (cherry picked from commit a2011780ffefd6f9c120b80651015b3f6646244b) Review-Url: https://codereview.chromium.org/2783203002 . Cr-Commit-Position: refs/branch-heads/2987@{#896} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/24f8804aa139486723395483bda7015d3c57e1b2/third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h [modify] https://crrev.com/24f8804aa139486723395483bda7015d3c57e1b2/third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp [modify] https://crrev.com/24f8804aa139486723395483bda7015d3c57e1b2/third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h [modify] https://crrev.com/24f8804aa139486723395483bda7015d3c57e1b2/third_party/WebKit/Source/core/dom/MutationCallback.h
,
Mar 30 2017
,
Mar 31 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ba0f387f04ae5673c6360611fc1bb2bff9a73b24 commit ba0f387f04ae5673c6360611fc1bb2bff9a73b24 Author: Michael Lippautz <mlippautz@chromium.org> Date: Fri Mar 31 08:09:35 2017 [wrapper-tracing] Emite write barrier upon associating a wrapper Whenever we assign a reference that should be traced into V8 we need to emit a write barrier. We automatically emit write barriers for TraceWrapperv8Reference. However, we still use a regular (weak) Persistent in ScriptWrappable, so we need to emit the barrier there manually. A follow up should convert the reference in ScriptWrappable to TraceWrapperv8Reference. BUG= chromium:702490 Review-Url: https://codereview.chromium.org/2759243002 Cr-Commit-Position: refs/heads/master@{#458357} (cherry picked from commit a2011780ffefd6f9c120b80651015b3f6646244b) Review-Url: https://codereview.chromium.org/2785423002 . Cr-Commit-Position: refs/branch-heads/2987@{#900} Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943} [modify] https://crrev.com/ba0f387f04ae5673c6360611fc1bb2bff9a73b24/third_party/WebKit/Source/bindings/core/v8/ScriptWrappable.h [modify] https://crrev.com/ba0f387f04ae5673c6360611fc1bb2bff9a73b24/third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.cpp [modify] https://crrev.com/ba0f387f04ae5673c6360611fc1bb2bff9a73b24/third_party/WebKit/Source/bindings/core/v8/ScriptWrappableVisitor.h [modify] https://crrev.com/ba0f387f04ae5673c6360611fc1bb2bff9a73b24/third_party/WebKit/Source/core/dom/MutationCallback.h
,
Apr 12 2017
Issue 709375 has been merged into this issue.
,
Apr 25 2017
Issue 709064 has been merged into this issue.
,
May 30 2017
,
Aug 1 2017
,
Jan 22 2018
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by fukino@chromium.org
, Mar 17 2017Owner: mlippautz@chromium.org
Status: Assigned (was: Untriaged)