I see this on my samus running Version 57.0.2987.19 as well.

The behavior is inconsistent per window. If I open a new Files App window, I am able to see the Settings menu options.

Reproduced. I'll look into this.

When this occurs, I could not open context menu as well.

 Issue 687199  has been merged into this issue.

 Issue 690738  has been merged into this issue.

Weird...

I confirmed that the third element (<hr>) in gear menu is properly decorated once. (i.e. <hr> element's __proto__ property is set up as cr.ui.MenuItem.prototype)
However, when the gear menu is shown, the <hr> element's __proto__ is turned out to be HTMLHRElement's prototype.

I'm still not able to bisect (due to varying repro rate) nor create reduced code...

Hi @haraken, I have a question in the bottom.

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;


I suspect that <hr> DOM elements that are not referenced from JS code can be re-created or their __proto__ can be reset.

@haraken, I'm not sure if it is possible.
Who should I ask to help investigation?

+yukishiino: I think we're missing the wrapper tracing somewhere.

> The timing seems random.

I think this will depend on when a GC is triggered. The GC is collecting the wrapper of <hr>...

> I suspect that <hr> DOM elements that are not referenced from JS code can be re-created or their __proto__ can be reset.

Yeah, we're trying hard to not collect the wrapper of <hr> if there is any meaningful reference to <hr>, but I suspect we're missing something to keep it alive.

Thank you for the comment, haraken@!

We are going to work around the issue by making explicit references for those <hr>s (at least for M57).

Hmm, the "meaningful reference" doesn't seems so clear to me.

Executing the following line before the <hr>s are decorated did not work.
window.allHRs = document.querySelectorAll('hr');

But, assigning id to every <hr>s (e.g. <hr id="foo">) and executing the following line at the same place worked.
window.foo = document.querySelector('#foo');

yukishiino@, I'm going to follow the latter approach, but if you have any recommended workaround, please let me know.
In addition, if I can help investigation by additional experiments, please let me know too.

> window.allHRs = document.querySelectorAll('hr');

Maybe is it because querySelectorAll returns a live node list?

Would the following work?

  v = document.querySelectorAll('hr');
  window.allHRs = [];
  for (i in v) { window.allHRs.push(v[i]); }

The code you suggested (and the shorthand: window.allHRs = [].slice.call(document.querySelectorAll('hr'))) worked.
I did not know the concept of live collection. Thanks!

Now the workaround gets less ugly :)

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

commit 3bc177a7e5b7d15e6635613373ac316263576150
Author: fukino <fukino@chromium.org>
Date: Tue Feb 28 09:54:35 2017

Files app: Work around the issue about opening menus by keeping references to <hr>s.

Files app's settings menu uses cr.ui.Menu, and <hr>s in the menu needs to be
decorated by cr.ui.MenuItem. However, in some cases, the <hr>s can lose the
decorated functions for some reason (suspected that it's by GC for <hr> wrappers).
This behavior be worked around by explicitly keeping reference to those <hr>s.

BUG= 689255 
TEST=confirmed 300+ trials of repro step did not reproduce the issue. It reproduced around 10% without the patch.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation

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

[modify] https://crrev.com/3bc177a7e5b7d15e6635613373ac316263576150/ui/file_manager/file_manager/foreground/js/ui/file_manager_ui.js

Sorry I am late to the party.

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?

Yes, you can build Chrome for ChromeOS and run it under Linux.

I built Chrome for ChromeOS using https://chromium.googlesource.com/chromium/src/+/master/docs/old_chromeos_build_instructions.md and was able to reproduce the bug once (manually commented out the already landed fix).

thanks!

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

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.

fukino@ What fix was merged to master? Looks like that is not the complete fix?

Thank you mlippautz@ for the investigation!

ketakid@, the fix (workaround) in Comment #15 was merged to main branch and I believe it fixed the issue described in the bug description.
Since the issue is serious to Files app, I'm requesting to merge the workaround to M57.

The root cause (in Blink binding?) is still under investigation.

Your change meets the bar and is auto-approved for M57. Please go ahead and merge the CL to branch 2987 manually. Please contact milestone owner if you have questions.
Owners: amineer@(clank), cmasso@(bling), ketakid@(cros), govind@(desktop)

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

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

commit d26dc12aebd14b71b72081877682a97d11047652
Author: Naoki Fukino <fukino@chromium.org>
Date: Thu Mar 02 02:35:07 2017

Files app: Work around the issue about opening menus by keeping references to <hr>s.

Files app's settings menu uses cr.ui.Menu, and <hr>s in the menu needs to be
decorated by cr.ui.MenuItem. However, in some cases, the <hr>s can lose the
decorated functions for some reason (suspected that it's by GC for <hr> wrappers).
This behavior be worked around by explicitly keeping reference to those <hr>s.

BUG= 689255 
TEST=confirmed 300+ trials of repro step did not reproduce the issue. It reproduced around 10% without the patch.
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation
TBR=oka@chromium.org

Review-Url: https://codereview.chromium.org/2712403003
Cr-Commit-Position: refs/heads/master@{#453557}
(cherry picked from commit 3bc177a7e5b7d15e6635613373ac316263576150)

Review-Url: https://codereview.chromium.org/2728573003 .
Cr-Commit-Position: refs/branch-heads/2987@{#736}
Cr-Branched-From: ad51088c0e8776e8dcd963dbe752c4035ba6dab6-refs/heads/master@{#444943}

[modify] https://crrev.com/d26dc12aebd14b71b72081877682a97d11047652/ui/file_manager/file_manager/foreground/js/ui/file_manager_ui.js

Removing RB label the issue in the bug description was fixed (on M57 and ToT).

I'll keep this open for the Blink issue. Please let me know if it is better to be filed separately.

fukino: wouldn't it be better to file a separate bug for the blink issue?

Yes, as the Files app issue is not reproducible anymore, I agree that the Blink issue should be filed separately.

The problem is... I don't have a reduced code to reproduce the Blink issue.
Simple steps like this did not reproduce the issue.
1) document.querySelector('hr').__proto__ = {foo: true};
2) Call window.gc()
3) Check the value of document.querySelector('hr').__proto__.

mlippautz@, do you have any idea to reproduce the issue?
Or, can I file a bug for the Blink issue without a reduced code (i.e. refer this bug and ask "revert 3bc177a7...")? 

I wonder whether the issue still reproduces if we change https://cs.chromium.org/chromium/src/v8/src/heap/heap.cc?rcl=f7036b132f6bcce9312b52a9ba386ae487280713&l=2945 (Heap::IsUnmodifiedHeapObject) to always return false?

If I revert the workaround (3bc177a7e5b7d15e6635613373ac316263576150), the issue reproduces (frequency: 5/63)
When I made Heap::IsUnmodifiedHeapObject return false in addition to the revert of workaround above, the issue still reproduced. (frequence: 2/59)

Since the issue described in the bug description was already worked around, I filed a bug ( Issue 702490 ) to separate the Blink issue and collect the related information.

Let me mark this issue Fixed for M57+.