1926630 - Menuitems added via async onPopupShowing handler create duplicates on macos

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Attachment: Screenshot showing duplicated menuitems

If a menubar menupopup is populated by an async onPopupShowing handler, which awaits some async data, and then renders it into menuitems, then on macos only (not windows) each of the inserted menuitems creates a second menuitem, rendered in the menu in reverse order, see screenshot.

Attempting to delete these extra menuitems via DOM JS APIs fails--these extra menuitems are not counted as children from the DOM perspective.

This behavior is observed whether the items are inserted one-by-one or whether they are added to a document fragment which is then inserted into the XUL DOM.

Based on initial debugging it seems that dynamically added items are double-inserted by a mutation observer: nsINode::InsertBefore is called, triggering MutationObserverWrapper::ContentInserted, which calls nsMenuGroupOwnerX::ContentInserted, then nsMenuX::ObserveContentInserted, then nsMenuX::InsertMenuChild.

On the other hand, elements left over from the previous rendering of the menu are initially displayed via nsMenuX::menuOpened calling nsMenuX::rebuildMenu calling nsMenuX::addMenuChild.

See screenshot and https://phabricator.services.mozilla.com/D226415 for a demo of this bug (you should have some extra profiles to test this out, they can be created via about:profiles).

Comment 1

[Markus Stange [:mstange]]

I can reproduce this. I haven't started debugging yet.

Comment 2

[Markus Stange [:mstange]]

This is a legitimate bug. I think this might be the first use of this type:

• A menupopup is being mutated while it's open
• The menu is not the root menupopup of a context menu

From what I can tell, the way the native menu system sets up its mutation observers is bound to cause duplicate notifications: We register the observer on the <menubar>, on the context menu's root <menupopup>, and on every nested <menu>. But mutation observers observe the entire DOM subtree. So we get lots of duplicate mutation callbacks for anything that's nested at least one level deep.

And it seems we've always been getting these duplicate callbacks. It's just that, if our observer is called while the menu is closed, we just set a "need to rebuild" flag and return.

But when the menu is open, these duplicate notifications cause trouble.

Comment 3

[Markus Stange [:mstange]]

I have a super simple fix, writing a test now.

Comment 4

[Markus Stange [:mstange]]

Attachment: Bug 1926630 - Only register the mutation observer with the subtree root of the menu structure, to avoid duplicate notifications. r=#mac-reviewers

Comment 5

[Markus Stange [:mstange]]

Attachment: Bug 1926630 - Make CheckNativeMenuConsistency detect duplicate menuitems more reliably. r=#mac-reviewers

In the case of this bug, we were creating different NSMenuItem
objects which ended up referring to the same nsIContent object.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d0fb0e1ce38
Make CheckNativeMenuConsistency detect duplicate menuitems more reliably. r=mac-reviewers,bradwerth
https://hg.mozilla.org/integration/autoland/rev/e369109c2e87
Only register the mutation observer with the subtree root of the menu structure, to avoid duplicate notifications. r=mac-reviewers,bradwerth

Comment 7

[Sandor Molnar[:smolnar]]

Backed out for causing macOS assertion failures

Backout link: https://hg.mozilla.org/integration/autoland/rev/8b099f2b18f3af46c2624ade20b2b0bbd2417ebb

Push with failures

Failure log -> Assertion failure: mRawPtr != nullptr (You can't dereference a NULL RefPtr with operator->().), at /builds/worker/workspace/obj-build/dist/include/mozilla/RefPtr.h:343

Failure log -> PROCESS-CRASH | application crashed [@ PLDHashTable::EntryStore::IsAllocated] | toolkit/content/tests/chrome/test_menu_activateitem.xhtml

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f5b68fee6dbd
Make CheckNativeMenuConsistency detect duplicate menuitems more reliably. r=mac-reviewers,bradwerth
https://hg.mozilla.org/integration/autoland/rev/dbc9c8f60473
Only register the mutation observer with the subtree root of the menu structure, to avoid duplicate notifications. r=mac-reviewers,bradwerth

Comment 9

[agoloman]

Backed out for causing mochitest failures at test_native_menus.xhtml.

• Backout link
• Push with failures
• Failure Log

Comment 10

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&revision=ce3f48f18c93396729a198cef48eeecd05902784

Comment 11

[Markus Stange [:mstange]]

In the try push, two out of the three bc4 tests fail with "WARNING: YOU ARE LEAKING THE WORLD", so I think having an unconditional mutation listener on the root element of the menu hierarchy keeps things alive for too long. I'll try to make it so that we only have the root mutation listener if there are registered menu change observers.

Comment 12

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&revision=fec15d65f505783486fe0b60ffe93df77983c277

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c57ae217dc8c
Make CheckNativeMenuConsistency detect duplicate menuitems more reliably. r=mac-reviewers,bradwerth
https://hg.mozilla.org/integration/autoland/rev/f001943cbf9b
Only register the mutation observer with the subtree root of the menu structure, to avoid duplicate notifications. r=mac-reviewers,bradwerth

Comment 14

[amarc]

https://hg.mozilla.org/mozilla-central/rev/c57ae217dc8c
https://hg.mozilla.org/mozilla-central/rev/f001943cbf9b