1701843 - Can't open bookmarks toolbar context menu if about:config context menu was opened first

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Markus Stange [:mstange]]

Steps to reproduce:

1. Open a fresh Firefox window.
2. Set the bookmarks bar visibility to "Always show" and make sure you have at least one bookmark visible in the bookmarks bar.
3. Go to about:config and click the button to accept the risk, if shown.
4. Right-click in the content area of about:config.
5. Right-click on a bookmark in the bookmarks bar.

Expected results:
A context menu for the bookmark should open.

Actual results:
No context menu opens. Right-clicking repeatedly does not work either.

I think this was kinda-broken for a long time and then got really broken more recently.

Comment 1

[Alice0775 White]

There are 2 regression.
#1, Wrong context menu. i.e, incorrect context menu items are enabled and disabled.
#2, No context menu pops up. i.e, this bug.

#1 Regression window:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=522a2bc06f9e5d0767bb3f2e127cc21eeaeb8a78&tochange=b26753fb35e7c53082ca6eaff1fe60a4a815a740

Suspect #1 bug: Bug 1505909

#2 Regression window:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a95d10a2de4dabc987709e67ca5d05be41441fb9&tochange=019d689bcd88855f2f63eb4e6c24d25e55e766c4

Suspect #2bug : Bug 1681138

Comment 2

[Marco Bonardo [:mak]]

Daisuke-san, would you have any spare time to debug this and try to see if we understand what's up?

Comment 3

[Daisuke Akatsuka (:daisuke)]

Marco-san, yes, I will take a look at this.
Thanks!

Comment 4

[Daisuke Akatsuka (:daisuke)]

Hi Marco-san,
I have investigated this issue and I'd like to know your opinion.

In direct, it seems the reason is that getViewForNode(document.popupNode) returns null if the document.popupNode points to a content element, at below place.
https://searchfox.org/mozilla-central/rev/a5bf5d0720f9454687f500513ac82b0c8abce5a4/browser/components/places/PlacesUIUtils.jsm#1347-1353
And, it seems we can resolve it if clear the document.popupNode.

Regarding the timing, I can think 2 ways.
First one, clear the document.popupNode even if trigger content is none when nsMenuPopupFrame::HidePopup() like below.
https://hg.mozilla.org/try/rev/ccedd46064b0aacec38cb14c4ad88dc77ccf7e84
The other one, if getViewForNode(document.popupNode) returns null, clear document.popupNode once, then re-take document.popupNode again.
https://hg.mozilla.org/try/rev/7a64a9416d249371a9f938d0a81778746a74d208

In the try, it seems that both approaches have no failures that are caused by the patch.
https://treeherder.mozilla.org/jobs?repo=try&revision=abc076b4159eb76b9d348a2786af83feb91b36c8
https://treeherder.mozilla.org/jobs?repo=try&revision=a3ea3708fbc3dcced84261c0d60a0695200af3b6

What do you think above approaches? And, one question, are there cases that we should not display the context menu because of that getViewForNode(document.popupNode) returns null?

Comment 5

[Marco Bonardo [:mak]]

Thank you Daisuke, great analysis as usual.
So the first question would be why document.popupNode is sticking to the privileged content element, when we right clicked a XUL element in the chrome toolbox? Though, considered document.popupNode is deprecated according to https://developer.mozilla.org/en-US/docs/Web/API/Document/popupNode, we should probably move to the newer alternative rather than trying to fix/break it.

Using menupopup.triggerNode seems to do the job here, I don't know why this code is still using document.popupNode, in the past there used to be be bugs forcing that, but those may have gone.
I'd suggest to do some general quick testing of context menus in the Places views (menu, tree, toolbar, overflow menu, library, sidebar...) and if everything is working good, just switch the code to use triggerNode, as it should.

I'd also suggest filing a bug to remove any remaining usage of document.popupNode in Places, I think we could do someting like caching triggerNode in an expando of the context menu popup during popupshowing, and then reuse it in the various methods invoked through the context menu (though it looks like we're already caching _view, thus I wonder if that'd be enough already...)

Comment 6

[Daisuke Akatsuka (:daisuke)]

Attachment: menupopup-triggerNode.png

Thank you very much for your clear direction, Marco!

I tried to use menupopup.triggerNode instead of document.popupNode like below.
menupopup._view = this.getViewForNode(menupopup.triggerNode);
It seems some places work, but as the screenshot, at least the context menu on the bookmark button is not correct after right-clicking on content element.
I will investigate the reason.

Comment 7

[Marco Bonardo [:mak]]

It's possible here there's a widget bug where privileged content confuses the code, and it's breaking both popupNode and triggerNode, if that's the case we should involve Neil Deakin or someone in widgets to help, since the problem may be more widespread than just bookmarks.

Comment 8

[Daisuke Akatsuka (:daisuke)]

Thank you very much, Marco.

As far as I have investigated it, the reason why we could not build context menu correctly seems that we are referring to document.popupNode in other places as well when building it.
For example here.
https://searchfox.org/mozilla-central/rev/a8b75e4ba3f8ddf0e76b42681d0a7b7e78e67730/browser/components/places/content/browserPlacesViews.js#219

So, I try to address with that set menupopup.triggerNode to document.popupNode before building since it seems that such places are scattered all over the place. Please see the patch.
https://hg.mozilla.org/try/rev/c67e709d66b7c5a866d132818e02f4dac9e7fbf9
(and try: https://treeherder.mozilla.org/jobs?repo=try&revision=937642129efc503b85b30f07eafd6344af4cedcf)
After that, I want to replace the places referring to document.popupNode with menupopup.triggerNode in bug 1706004.
But, is it okay for you?

Comment 9

[Daisuke Akatsuka (:daisuke)]

Sorry, forgot ni.

Comment 10

[Marco Bonardo [:mak]]

I'm sorry there's too much bugmail traffic these days!

(In reply to Daisuke Akatsuka (:daisuke) from comment #8)

So, I try to address with that set menupopup.triggerNode to document.popupNode before building since it seems that such places are scattered all over the place. Please see the patch.
https://hg.mozilla.org/try/rev/c67e709d66b7c5a866d132818e02f4dac9e7fbf9

Maybe? Unfortunately I don't know the popupNode code well to tell if we can just replace it like that, but it may be a good stop-gap change if we have a long term plan to remove it. You should add a comment above that explaining why we do it and a TODO (Bug 1706004) with the long term plan, and then we may want to ask for review to Neil Deakin to ensure this short term strategy doesn't break other stuff.

Comment 11

[Daisuke Akatsuka (:daisuke)]

Thank you very much for replying in your busy!
Okay, I will append the comment and make a test.
And also, I will ask Neil to review this.

Comment 12

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1701843: Set menupopup.triggerNode to document.popupNode to show context menu correctly.

The reason why the context menu is not shown is that
getViewForNode(document.popupNode) returns null if the document.popupNode
points to a content element, at below place.
https://searchfox.org/mozilla-central/rev/a5bf5d0720f9454687f500513ac82b0c8abce5a4/browser/components/places/PlacesUIUtils.jsm#1347-1353
So, we try to take the view from menupopup.triggerNode instead of
document.popupNode.
However, replacing with menupopup.triggerNode here is not enough to show the
context menu correctly since we are refferring document.popupNode while building
the context menu.
e.g. https://searchfox.org/mozilla-central/rev/a8b75e4ba3f8ddf0e76b42681d0a7b7e78e67730/browser/components/places/content/browserPlacesViews.js#219
Thus, for now, we set menupopup.triggerNode to document.popupNode to resolve
for short term.
In future, we may replace places refferring document.popupNode with
other elements in Places in bug 1706004.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a74febc3cc23
Set menupopup.triggerNode to document.popupNode to show context menu correctly. r=NeilDeakin

Comment 14

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/a74febc3cc23

Comment 15

[Catalin Sasca, Desktop QA [:csasca]]

Was able to reproduce the issue on Firefox 89.0a1 (2021-03-30) under macOS 10.15.7 by following the info provided in Comment 0.

The issue is fixed on Firefox 90.0b4 and Firefox 91.0a1 (2021-06-06). Tests were performed on macOS 10.15.7, Ubuntu 20.04 and Windows 10.