1246044 - Split the context menu test into separate files to test icons, radio groups, checkboxes, and uninstalling.

Status: VERIFIED FIXED

(WebExtensions :: Untriaged, defect, P2)

Description

[Matthew Wein [:mattw]]

Actual:

When you remove an extension which modifies the context menu and install a new one, the context menu items added by the deleted extension still show up in the context menu.

Expected: 

The context menu items from the deleted extension should not show up in the context menu.

Comment 1

[Kris Maglione [:kmag]]

Interesting... It looks like it's because we delete the root item for the extension in the startup observer rather than in the shutdown observer.

Can you take this one, Matt, and hopefully add some tests for it as well?

Comment 2

[Matthew Wein [:mattw]]

Sure, will do.

Comment 4

[Matthew Wein [:mattw]]

Attachment: Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

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

Comment 5

[Matthew Wein [:mattw]]

I confirmed manually that the change fixes the issue.  However, I can't seem to reproduce the bug in the tests, so I'm looking for feedback on how I might be able to do this.  Right now, if I load a new extension in the test without the fix, it seems to work fine.

Comment 6

[Matthew Wein [:mattw]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

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

Comment 7

[Matthew Wein [:mattw]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

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

Comment 8

[Matthew Wein [:mattw]]

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

Comment 9

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/44157/#review41809

::: browser/components/extensions/test/browser/browser_ext_contextMenus_uninstall.js:59
(Diff revision 3)
> +  EventUtils.synthesizeMouseAtCenter(items[0].childNodes[0], {});
> +  yield popupShownPromise;
> +
> +  // Make sure the context menu items from the previous extension are gone.
> +  items = contentAreaContextMenu.getElementsByAttribute("ext-type", "top-level-menu");
> +  is(items.length, 1, "only one top level menu item should exist");

I confirmed that this test fails without the change and passes with the change (items.length equals 2 without the change).

Comment 10

[Kris Maglione [:kmag]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

https://reviewboard.mozilla.org/r/44157/#review42445

Thanks! This is much clearer. Just need to fix some code duplication and it should be good to land.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_radioGroups.js:71
(Diff revision 3)
> +
> +  function* closeContextMenu(itemToSelect) {
> +    let popupHiddenPromise = BrowserTestUtils.waitForEvent(contentAreaContextMenu, "popuphidden");
> +    EventUtils.synthesizeMouseAtCenter(itemToSelect, {});
> +    yield popupHiddenPromise;
> +  }

Can you move these shared helper functions to `head.js` or another shared script?

::: browser/components/extensions/test/browser/browser_ext_contextMenus_uninstall.js:22
(Diff revision 3)
> +    },
> +
> +    background: function() {
> +      browser.contextMenus.create({title: "a"});
> +      browser.contextMenus.create({title: "b"});
> +      browser.test.notifyPass();

Please always pass a relevant string to `notifyPass` and `awaitFinish`

::: browser/components/extensions/test/browser/browser_ext_contextMenus_uninstall.js:69
(Diff revision 3)
> +  EventUtils.synthesizeMouseAtCenter(items[0], {});
> +  yield popupHiddenPromise;
> +
> +  // Uninstall the extension.
> +  yield extension.unload();
> +  yield BrowserTestUtils.removeTab(tab1);

Please check that there are no more context menu items after the second extension is unloaded.

Comment 11

[Matthew Wein [:mattw]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/3-4/

Comment 12

[Matthew Wein [:mattw]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/4-5/

Comment 13

[Matthew Wein [:mattw]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/5-6/

Comment 14

[Matthew Wein [:mattw]]

Kris, this should be ready for review now, but I'm not sure if it makes sense to have the method for opening the extension menu as well as a method for opening the context menu in head.js, although I do use them both.

Also, I'm not sure if the method for closing the extension menu should support checking the click info, since doing so requires it to have a reference to the extension.

What do you think?

Comment 15

[Matthew Wein [:mattw]]

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

Comment 16

[Kris Maglione [:kmag]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

https://reviewboard.mozilla.org/r/44157/#review43491

::: browser/components/extensions/test/browser/browser.ini:73
(Diff revisions 3 - 6)
> +[browser_ext_windows.js]
>  [browser_ext_windows_create.js]
>  tags = fullscreen
>  [browser_ext_windows_create_tabId.js]
> -[browser_ext_windows.js]
> +[browser_ext_windows_events.js]
>  [browser_ext_windows_size.js]

I'm fine with re-sorting this file, but please do it in a second commit, since it's likely to cause merge conflicts.

::: browser/components/extensions/test/browser/head.js:145
(Diff revision 6)
>  }
>  
> +function* openContextMenu() {
> +  let contentAreaContextMenu = document.getElementById("contentAreaContextMenu");
> +  let popupShownPromise = BrowserTestUtils.waitForEvent(contentAreaContextMenu, "popupshown");
> +  yield BrowserTestUtils.synthesizeMouseAtCenter("#img1", {type: "contextmenu", button: 2}, gBrowser.selectedBrowser);

"#img1" should be passed in as an argument.

::: browser/components/extensions/test/browser/head.js:161
(Diff revision 6)
> +}
> +
> +function* openExtensionContextMenu() {
> +  yield openContextMenu();
> +
> +  let contentAreaContextMenu = document.getElementById("contentAreaContextMenu");

This is returned by `openContextMenu`.

::: browser/components/extensions/test/browser/head.js:185
(Diff revision 6)
> +  }
> +  let contentAreaContextMenu = document.getElementById("contentAreaContextMenu");
> +  let popupHiddenPromise = BrowserTestUtils.waitForEvent(contentAreaContextMenu, "popuphidden");
> +  EventUtils.synthesizeMouseAtCenter(itemToSelect, {});
> +
> +  if (extension) {

This `if` statement and the `checkClickInfo` function are too context-specific to be in `head.js`. I'm fine with just duplicating them for each test file.

Comment 17

[Matthew Wein [:mattw]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/6-7/

Comment 18

[Matthew Wein [:mattw]]

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

Comment 19

[Matthew Wein [:mattw]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/7-8/

Comment 20

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/44157/#review43491

> I'm fine with re-sorting this file, but please do it in a second commit, since it's likely to cause merge conflicts.

I un-sorted the file, but I'm confused because the original revision looks like it's sorted...

Comment 21

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/44157/#review43491

> I un-sorted the file, but I'm confused because the original revision looks like it's sorted...

Actually, I think the file was sorted already in Bug 1257583, and it got sorted here when I pulled down the latest from fx-team in revision 7.  Should I re-sort it, or do you think isn't the case?

Comment 22

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/44157/#review43491

> Actually, I think the file was sorted already in Bug 1257583, and it got sorted here when I pulled down the latest from fx-team in revision 7.  Should I re-sort it, or do you think isn't the case?

Clarification: I unsorted it in revision 7 per your request, but I think it should stay sorted since that's what's arleady in fx team.

Comment 23

[Matthew Wein [:mattw]]

https://reviewboard.mozilla.org/r/44157/#review43491

> Clarification: I unsorted it in revision 7 per your request, but I think it should stay sorted since that's what's arleady in fx team.

s/Clarification/Correction/g :)

Comment 24

[Kris Maglione [:kmag]]

(In reply to Matthew Wein [:mattw] from comment #21
> Actually, I think the file was sorted already in Bug 1257583, and it got
> sorted here when I pulled down the latest from fx-team in revision 7. 
> Should I re-sort it, or do you think isn't the case?

Please use the same sorting as is used in the parent revision.

Comment 25

[Kris Maglione [:kmag]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

https://reviewboard.mozilla.org/r/44157/#review45885

::: browser/components/extensions/test/browser/browser_ext_contextMenus_checkboxes.js:6
(Diff revision 8)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +/* globals content */
> +/* eslint-disable mozilla/no-cpows-in-tests */

These two comments aren't needed.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_radioGroups.js:6
(Diff revision 8)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +/* globals content */
> +/* eslint-disable mozilla/no-cpows-in-tests */

These two comments aren't needed.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_uninstall.js:6
(Diff revision 8)
> +/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
> +/* vim: set sts=2 sw=2 et tw=80: */
> +"use strict";
> +
> +/* globals content */
> +/* eslint-disable mozilla/no-cpows-in-tests */

These two comments aren't needed.

Comment 26

[drugan]

Tested this issue in nightly 49.0a1 and found that after disabling or removing an extension the custom entry is disappeared from context menu but enabling the extension or clicking "Undo" in Addons Manager duplicates the menu entry as many times as you disable/enable the extension and only the last of them actually works.

Comment 27

[Matthew Wein [:mattw]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/8-9/

Comment 28

[Matthew Wein [:mattw]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/9-10/

Comment 29

[Matthew Wein [:mattw]]

Attachment: Part 2 - Bug 1246044 - Fix issue where context menu items show up after extension is uninstalled.

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

Comment 30

[Matthew Wein [:mattw]]

Attachment: Part 3 - Bug 1246044 - Add unit tests for uninstall bug.

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

Comment 31

[Matthew Wein [:mattw]]

Attachment: Part 4 - Bug 1246044 - Fix icon regression caused by bug 1270742.

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

Comment 32

[Matthew Wein [:mattw]]

Attachment: Part 4 - Bug 1246044 - Add unit tests for context menu icons.

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

Comment 33

[Matthew Wein [:mattw]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/10-11/

Comment 34

[Kris Maglione [:kmag]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

https://reviewboard.mozilla.org/r/44157/#review59504

::: browser/components/extensions/test/browser/.eslintrc:28
(Diff revision 11)
>      "focusWindow": true,
>      "makeWidgetId": true,
> +    "openContextMenu": true,
> +    "closeContextMenu": true,
> +    "openExtensionContextMenu": true,
> +    "closeExtensionContextMenu": true,

Please sort the symbols in this list.

Comment 35

[Kris Maglione [:kmag]]

Comment on attachment 8768244 [details]
Part 2 - Bug 1246044 - Fix issue where context menu items show up after extension is uninstalled.

https://reviewboard.mozilla.org/r/62566/#review59510

Comment 36

[Kris Maglione [:kmag]]

Comment on attachment 8768245 [details]
Part 3 - Bug 1246044 - Add unit tests for uninstall bug.

https://reviewboard.mozilla.org/r/62568/#review59512

::: browser/components/extensions/test/browser/browser_ext_contextMenus_uninstall.js:9
(Diff revision 1)
> +
> +add_task(function* () {
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser,
> +    "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html");
> +
> +  gBrowser.selectedTab = tab1;

This isn't necessary.

Comment 37

[Kris Maglione [:kmag]]

Comment on attachment 8768246 [details]
Part 4 - Bug 1246044 - Fix icon regression caused by bug 1270742.

https://reviewboard.mozilla.org/r/62570/#review59514

This commit summary is inaccurate.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:9
(Diff revision 1)
> +
> +add_task(function* () {
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser,
> +    "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html");
> +
> +  gBrowser.selectedTab = tab1;

This isn't necessary.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:11
(Diff revision 1)
> +  let tab1 = yield BrowserTestUtils.openNewForegroundTab(gBrowser,
> +    "http://mochi.test:8888/browser/browser/components/extensions/test/browser/context.html");
> +
> +  gBrowser.selectedTab = tab1;
> +
> +  let dataURI = "iVBORw0KGgoAAAANSUhEUgAAACQAAAAkCAYAAADhAJiYAAAC4klEQVRYhdWXLWzbQBSADQtDAwsHC1tUhUxqfL67lk2tdn+OJg0ODU0rLByqgqINBY6tmlbn7LMTJ5FaFVVBk1G0oUGjG2jT2Y7jxmmcbU/6iJ+f36fz+e5sGP9riCGm9hB37RG+scd4Yo/wsDXCZyIE2xuXsce4bY+wXkAsQtzYmExrfFgvkJkRbkzo1ehoxx5iXcgI/9iYUGt8WH9MqDXEcmNChmEYrRCf2SHWeYgQx3x0tLNRIeKQLTtEFyJEep4NTuhk8BC+yMrwEE3+iozo42d8gK7FAOkMsRiiN8QhW2ttSK5QTfRRV4QoymVeJMvPvDp7gCZigD613MN6yRFA3SWarow9QB9LCfG+NeF9qCtjAKOSQjCqVKhfVsiHEQ+grgx/lRGqUihAc1uL8EFD+KCRO+GrF4J61phcoRoPoEzkYhZYpykh5sMb7kOdIeY+jHKur4QI4Feh4AFX1nVeLxrAvQchGsBz5ls6wa2QdwcvIcE2863bTH79KOvsz/uUYJsp+J0pSzNlDckVqqVGUAF+n6uS7txcOl6wot4JVy70ufDLy4pWLUQVPE81pRI0mGe9oxLMHSeohHvMs/STUNaUK6vDPCvOyxMFDx4achehRDJmHnydnkPww5OFfLxrGIZBFDyYl4LpMzlTQFIP6AQx86w2UeYBccFpJrcKv5L9eGDtUAU6RIELqsB74uynjy/UBRF1gS5BTFxwQT1wTiXoUg9MH7m/3NZRRoi5IJytUbMgzv4Wc832+oQkiKgEehmyMkkpKsFkQV11QsRJL5rJYBLItQgRaUZEmnoZXsomz3vGiWw+I9KMF9SVFOqZEemZekli1jN3U/UOqhHHvC6oWWGElhfSpGdOk6+O9prdwvtLj5BjRsQxdRnot+Zeifpy/2/0stktKTRNLmbk0mwXyl8253fyojj+8rxOHNAhjjm5n0/5OOCGOKBzkrMO0Z75lvSAzKlrF32Z/3z8BqLAn+yMV7VhAAAAAElFTkSuQmCC";

This is not actually a data URI.

::: browser/components/extensions/test/browser/browser_ext_contextMenus_icons.js:47
(Diff revision 1)
> +  });
> +
> +  let confirmContextMenuIcon = (rootElement) => {
> +    let expectedURL = new RegExp(String.raw`^moz-extension://[^/]+/extension\.png$`);
> +    let imageUrl = rootElement.getAttribute("image");
> +    is(expectedURL.test(imageUrl), true, "The context menu should display the extension icon next to the root element");

`ok(expectedURL.test(...), "...")`

Comment 38

[Kris Maglione [:kmag]]

Comment on attachment 8768247 [details]
Part 4 - Bug 1246044 - Add unit tests for context menu icons.

https://reviewboard.mozilla.org/r/62572/#review59516

This commit summary is inaccurate, this is fixed from a patch in another bug, and this patch adds a test file to the manifest which was created in the previous patch, rather than this one.

Comment 39

[Matthew Wein [:mattw]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/11-12/

Comment 40

[Matthew Wein [:mattw]]

Comment on attachment 8768244 [details]
Part 2 - Bug 1246044 - Fix issue where context menu items show up after extension is uninstalled.

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

Comment 41

[Matthew Wein [:mattw]]

Comment on attachment 8768245 [details]
Part 3 - Bug 1246044 - Add unit tests for uninstall bug.

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

Comment 42

[Matthew Wein [:mattw]]

Comment on attachment 8768247 [details]
Part 4 - Bug 1246044 - Add unit tests for context menu icons.

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

Comment 43

[Kris Maglione [:kmag]]

Comment on attachment 8768247 [details]
Part 4 - Bug 1246044 - Add unit tests for context menu icons.

https://reviewboard.mozilla.org/r/62572/#review59590

Comment 44

[Matthew Wein [:mattw]]

Comment on attachment 8737867 [details]
Part 1 - Bug 1246044 - Split out the testing for radio groups and checkboxes into separate files.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/44157/diff/12-13/

Comment 45

[Matthew Wein [:mattw]]

Comment on attachment 8768244 [details]
Part 2 - Bug 1246044 - Fix issue where context menu items show up after extension is uninstalled.

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

Comment 46

[Matthew Wein [:mattw]]

Comment on attachment 8768245 [details]
Part 3 - Bug 1246044 - Add unit tests for uninstall bug.

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

Comment 47

[Matthew Wein [:mattw]]

Comment on attachment 8768247 [details]
Part 4 - Bug 1246044 - Add unit tests for context menu icons.

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

Comment 48

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/b113ed488daf
Part 1 - Split out the testing for radio groups and checkboxes into separate files. r=kmag
https://hg.mozilla.org/integration/fx-team/rev/2eb259ef1448
Part 2 -  Fix issue where context menu items show up after extension is uninstalled. r=kmag
https://hg.mozilla.org/integration/fx-team/rev/db20be5bef8a
Part 3 - Add unit tests for uninstall bug. r=kmag
https://hg.mozilla.org/integration/fx-team/rev/811b5f9b7ae8
Part 4 - Add unit tests for context menu icons. r=kmag

Comment 49

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

https://hg.mozilla.org/mozilla-central/rev/b113ed488daf
https://hg.mozilla.org/mozilla-central/rev/2eb259ef1448
https://hg.mozilla.org/mozilla-central/rev/db20be5bef8a
https://hg.mozilla.org/mozilla-central/rev/811b5f9b7ae8

Comment 52

[Vasilica Tamas, QA [:vtamas]]

I was able to reproduce the initial issue on Firefox 48.0.2 (20160823121617) under Windows 10 64-bit.

Verified as fixed on Firefox 50.0a2 (2016-09-05) and Firefox 51.0a1 (2016-09-05) under Windows 10 64-bit and here are the results:
- context menu item is no longer multiplied while disabling/enabling/reloading the webextension
- context menu item is completely erased when the webextension is removed.