1274877 - contextMenus API starts logging errors after removing an item

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P1)

Description

[Will Bamberg [:wbamberg]]

Attachment: context-menu.zip

Here's a WebExtension, that creates a context menu item labeled "Remove me!". If you click the item, it is removed.

After this, if you open the context menu, the item no longer appears, but an error is logged to the console:

TypeError: rootElement.firstChild is null   ext-contextMenus.js:43:11

Comment 1

[Bob Silverberg [:bsilverberg]]

I'll take this for now as it's a P1 and unassigned. I think Matt is our resident contextMenus expert, but he's on PTO.

Comment 2

[Bob Silverberg [:bsilverberg]]

The problem occurs if you add contextMenu items and then remove all of them. I am attaching a fix.

Comment 3

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1274877 - contextMenus API starts logging errors after removing an item,

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

Comment 4

[Bob Silverberg [:bsilverberg]]

https://reviewboard.mozilla.org/r/58292/#review55154

::: browser/components/extensions/test/browser/browser_ext_contextMenus.js:398
(Diff revision 1)
>    yield extension.unload();
>  
>    yield BrowserTestUtils.removeTab(tab1);
>  });
> +
> +add_task(function* test_remove_only_item() {

Note that this test can be run to reproduce the error without the patch to `ext-contextMenus.js`. Unfortunately, the only manifestation of the bug is an error message to the JavaScript console, and I'm not sure how to check for that (or rather, the absence of that) in the test, so right now this test does not disprove the bug.

Comment 5

[Kris Maglione [:kmag]]

If there are multiple extensions with context menus, and one of them produces this error, it will also break the extensions that come after it.

So we should try testing this with two active extensions.

Comment 6

[Bob Silverberg [:bsilverberg]]

(In reply to Kris Maglione [:kmag] from comment #5)
> If there are multiple extensions with context menus, and one of them
> produces this error, it will also break the extensions that come after it.
> 
> So we should try testing this with two active extensions.

Do you mean manually, or in an automated test. Can we load two extensions in a test?

Comment 7

[Kris Maglione [:kmag]]

(In reply to Bob Silverberg [:bsilverberg] from comment #6)
> Do you mean manually, or in an automated test.

Automated, but it might be worth trying to reproduce it manually as well.

> Can we load two extensions in a test?

Yes, but not easily. If you want to file a bug to add a utility for that, and
a follow-up bug to add tests for this that depends on it, this is probably
good to land. We should also request uplift, if you can verify the breakage
with multiple extensions installed.

Comment 8

[Bob Silverberg [:bsilverberg]]

(In reply to Kris Maglione [:kmag] from comment #7)
> (In reply to Bob Silverberg [:bsilverberg] from comment #6)
> > Do you mean manually, or in an automated test.
> 
> Automated, but it might be worth trying to reproduce it manually as well.
> 
> > Can we load two extensions in a test?
> 
> Yes, but not easily. If you want to file a bug to add a utility for that, and
> a follow-up bug to add tests for this that depends on it, this is probably
> good to land. We should also request uplift, if you can verify the breakage
> with multiple extensions installed.

I have manually verified that it does break contextMenus for other extensions that are installed. For landing this, I assume I should remove the current test that doesn't actually prove anything, so that all we'll land right now is the fix. Agreed?

Comment 9

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8760883 [details]
Bug 1274877 - contextMenus API starts logging errors after removing an item,

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58292/diff/1-2/

Comment 10

[Kris Maglione [:kmag]]

(In reply to Bob Silverberg [:bsilverberg] from comment #8)
> For landing this, I assume I should remove the current test that doesn't
> actually prove anything, so that all we'll land right now is the fix.
> Agreed?

Yes.

Comment 11

[Kris Maglione [:kmag]]

Comment on attachment 8760883 [details]
Bug 1274877 - contextMenus API starts logging errors after removing an item,

https://reviewboard.mozilla.org/r/58292/#review55256

Comment 12

[Bob Silverberg [:bsilverberg]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=60f38abe350c

Comment 13

[Bob Silverberg [:bsilverberg]]

Vasilica, we're going to want this uplifted to 48 and 49, so could you please verify this fix. Without the patch, if you have two extensions each of which add to the contextMenu, and one of them removes all of the items it created from the contextMenu, the contextMenu items for the other extension should stop showing up as well. You'll also notice the error mentioned in the initial description in the JavaScript console after removing all of the items, without the patch.

Comment 14

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce this issue on Firefox 50.0a1 (2016-06-08), Firefox 49.0a2 (2016-06-07) and Firefox 48.0a2 (2016-06-06) under Windows 10 64-bit and Ubuntu 14.04 32-bit.

Tested using https://treeherder.mozilla.org/#/jobs?repo=try&revision=60f38abe350c&selectedJob=22155083
Under Windows 10 64-bit and it seems to be fixed. The contextMenu item for the other webextension remains displayed after clicking on “Remove me!” and there is no javascript error thrown in Browser Console.

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/aaac9952244e
contextMenus API starts logging errors after removing an item, r=kmag

Comment 16

[Bob Silverberg [:bsilverberg]]

Thanks Vasilica!

Comment 17

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 18

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 49.0a1 (2016-05-20) under Windows 10 64-bit.

Verified fixed on Firefox 50.0a1 (2016-07-31) under Windows 10 64-bit and Ubuntu 14.04 32-bit. The error is no longer thrown in Browser Console.