1461115 - browser.menus.onShown fails to trigger in browser_action popup of WebExtension

Status: VERIFIED FIXED

(WebExtensions :: Frontend, defect)

Description

[rugk]

User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:60.0) Gecko/20100101 Firefox/60.0
Build ID: 20180507144316

Steps to reproduce:

Created a web extension and this in the background script:

browser.menus.onShown.addListener(menuShown);

where menuShown is

function menuShown(info, tab) {
        console.log(JSON.parse(JSON.stringify(tab)));
        if (!tab.url.startsWith(browser.extension.getURL())) {
            return;
        }

        browser.menus.onHidden.addListener(createItems);

        browser.menus.remove(CONVERT_TEXT_SELECTION); // contants contain IDs, should just hide them
        browser.menus.remove(CONVERT_LINK_TEXT_SELECTION);

        browser.menus.refresh();
    }


Actual results:

On websites this actually works, but not when I open the popup of my extension (browser action), and show the context menu there.

I get this, instead:

WeakMap key must be an object, got undefined  ext-browser.js:274
	setId chrome://browser/content/ext-browser.js:274:5
	getId chrome://browser/content/ext-browser.js:254:5
	wrapTab chrome://browser/content/ext-browser.js:942:47
	TabManagerBase/this._tabs< chrome://extensions/content/ext-tabs-base.js:1694:44
	get resource://gre/modules/ExtensionUtils.jsm:92:15
	getWrapper chrome://extensions/content/ext-tabs-base.js:1765:12
	hasActiveTabPermission chrome://extensions/content/ext-tabs-base.js:1736:12
	listener chrome://browser/content/ext-menus.js:831:13
	emit resource://gre/modules/ExtensionUtils.jsm:227:40
	emit resource://gre/modules/Extension.jsm:1293:37
	dispatchOnShownEvent chrome://browser/content/ext-menus.js:397:7
	forEach self-hosted:5844:9
	afterBuildingMenu chrome://browser/content/ext-menus.js:403:7
	build chrome://browser/content/ext-menus.js:62:5
	observe chrome://browser/content/ext-menus.js:750:5
	CM_initMenu chrome://browser/content/nsContextMenu.js:132:7
	nsContextMenu chrome://browser/content/nsContextMenu.js:85:3
	onpopupshowing chrome://browser/content/browser.xul:1:119


Expected results:

With that code, I wanted to hide the add-on's own menu entries in the add-on's own popup, as this is quite senseless for my add-on at least.

Comment 1

[Rob Wu [:robwu]]

Reproduced. The issue is that there is no "tab" associated with the context menu in the popup (not even in the onClicked event).
The MDN documentation of onShown (and onClicked) both allow for the possibility of the menu being empty, so the onShown dispatcher in ext-menus.js needs to check whether the tab is set before trying to convert it.


Steps to reproduce:
1. Load the extension (described below) in Chrome or Firefox.
2. Click on the browser_action extension button to show the popup.
3. Right-click in the popup and click on the menu item "Some menu item 1".
4. Look at the console of the background page:

Result:
- In Chrome (66), the tab is printed.
- In Firefox (60), the tab parameter is undefined.


> background.js

chrome.contextMenus.create({
    id: "item1",
    title: "Some menu item 1",
    onclick(info, tab) {
        console.log("menus.onClicked", info, tab);
    },
});

browser.menus.onShown.addListener(function(info, tab) {
    console.log("menus.onShown", info, tab);
});


> manifest.json

{
    "name": "browserAction popup menu",
    "version": "1",
    "manifest_version": 2,
    "background": {
        "scripts": ["background.js"]
    },
    "browser_action": {
        "browser_style": true,
        "default_popup": "popup.html"
    },
    "permissions": [
        "activeTab",
        "contextMenus",
        "menus"
    ]
}


> popup.html

This is a popup,<br>
Right-click me.

Comment 2

[Rob Wu [:robwu]]

Attachment: Bug 1461115 - Emit menus.onShown even if there is no tab

Review commit: https://reviewboard.mozilla.org/r/245088/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/245088/

Comment 3

[Rob Wu [:robwu]]

I fixed the bug by checking for emptiness of the tab.

As I showed in comment 1, there is a difference in how Chrome and Firefox set the tab property (in onClicked, and I extend the logic to onShown too):
- In Chrome, the tab is set even if the menu is opened in a browserAction popup.
- In Firefox, the tab is not set.

I decided to continue with Firefox's logic of omitting the tab, since a menu *inside* an extension popup is clearly not associated with the tab. Extensions can always use tabs.query if they want to determine the current tab.

Comment 4

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8976941 [details]
Bug 1461115 - Emit menus.onShown even if there is no tab

https://reviewboard.mozilla.org/r/245088/#review251032

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/010fd502cf60
Emit menus.onShown even if there is no tab r=mixedpuppy

Comment 6

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/010fd502cf60

Comment 7

[CosminB]

Attachment: Bug1461115.png

I can reproduce this issue on Firefox 60.0.1 (20180516032328) under Win 7 64-bit and Mac OS X 10.13.3.

The actual result from the description is displayed.

This issue is verified as fixed on Firefox 62.0a1 (20180525005138) under Win 7 64-bit and Mac OS X 10.13.3.

Comment 8

[Pixievolt No. 1]

FYI, in Firefox 60.0.1 this bug also affects menu entries created for the bookmark context, with the same error. I'll bet the same fix will work, but maybe it merits a separate test?

Comment 9

[rugk]

@[e-mail] To bump this up again, I think this question is very valid. Unit tests are likely useful. :)