1427991 - Can't remove an addon from the detail page if the list page is not the previous page in history

Status: RESOLVED DUPLICATE of bug 1482480

(Toolkit :: Add-ons Manager, defect, P5)

Description

[Cristina Badescu [:cbadescu], Experiments QA]

Attachment: Remove add-on.gif

[Notes]:
- Sometimes, at first add-on install, the theme options ("Default" and "Dark") might not appear and you need to refresh the "about:addons" page to see them.
- This issue is not reproducible when you select "Remove" button from "about:addons" main page.
- This issue is also reproducible after you use "Undo" option.
- See also: https://github.com/mozilla/notes/issues/545

[Affected versions]:
- Firefox 57.0.3 and up

[Affected Platforms]:
- All Mac
- All Linux

[Prerequisites]:
- Have a Firefox profile with the latest "Firefox Notes" add-on version installed from Test Pilot experiment page (https://testpilot.firefox.com/experiments/notes).

[Steps to reproduce]:
1. Open Firefox browser with the profile from prerequisites.
2. Navigate to "about:addons" page and select "Extensions" section.
3. Click on "More" link and select the "Remove" button.
4. Observe the behavior.

[Expected result]:
- The add-on is removed.

[Actual result]:
- The add-on is not removed and the Theme options disappear.

[Additional notes]:
- This issue is also reproducible with the latest Firefox "Notes" add-on version (2.2.0dev custom build from 01.04.2018) installed.
- Attached is a screen recording of the issue.

Comment 1

[Andrew Swan [:aswan]]

This appears to be due to the code here assuming that going back one step in history will take us from the detail page to the list of extensions:
https://searchfox.org/mozilla-central/rev/652fbd6270de0d3ec424d2b88f8375ff546f949f/toolkit/mozapps/extensions/content/extensions.js#1297-1299

I've got a fix in hand (having createOptionsBrowser() call loadURIWithFlags and pass REPLACE_HISTORY) but I need to write a test.  Patch coming shortly.

Comment 2

[Andrew Swan [:aswan]]

Attachment: Bug 1427991 Modernize browser_uninstalling.js

No big functional changes here, just:
- convert from nested callbacks to async/await
- remove tests of non-restartless extensions
- remove unused mock addons and renumber the remaining ones

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

Comment 3

[Andrew Swan [:aswan]]

Attachment: Bug 1427991 Don't create an extra history entry for options browser

The subject pretty much covers the substance of the change here.  Also did
a bit of cleanup on the related mochitest, though that test (as well as many
of its sibilings) could use a more substantial rewrite to reflect the move
from legacy extensions to WebExtensions...

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

Comment 4

[Andrew Swan [:aswan]]

Comment on attachment 8940405 [details]
Bug 1427991 Modernize browser_uninstalling.js

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

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8940406 [details]
Bug 1427991 Don't create an extra history entry for options browser

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

Comment 6

[Andrew Swan [:aswan]]

Comment on attachment 8940406 [details]
Bug 1427991 Don't create an extra history entry for options browser

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

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8940405 [details]
Bug 1427991 Modernize browser_uninstalling.js

https://reviewboard.mozilla.org/r/210678/#review216728

::: toolkit/mozapps/extensions/test/browser/browser_uninstalling.js:55
(Diff revision 2)
>    }
>    return null;
>  }
>  
> -// Tests that uninstalling a normal add-on from the list view can be undone
> -add_test(function() {
> +// Tests that uninstalling a restartless add-on from the list view can be undone
> +add_task(async function() {

Nit, given that we are touching this test file (it was definitely very "old style"), it would be nice to give a name to the async functions that we pass to add_task (only to make them more easily identifiable in the logs).

Comment 8

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8940406 [details]
Bug 1427991 Don't create an extra history entry for options browser

https://reviewboard.mozilla.org/r/210680/#review216682

I'm not set the r+ yet, mainly because as we agreed while discussing briefly about this over IRC, we would like to add an explicit test for the `if (!this._addon) { return; }` added to the `createOptionsBrowser` method and we would also like to double-check if we can fix the similar issue that is going to happen when an extension options page navigates to other extension urls (which would still going to become part of the history of the about:addons top level page when the extension is running in the main process).

::: toolkit/mozapps/extensions/content/extensions.js:3250
(Diff revision 3)
>        stack.remove();
>      }
>  
> +    // If the addon gets disabled before we get here (which probably
> +    // happens more in tests that in regular usage) then just bail out.
> +    if (!this._addon) {

As briefly discussed over IRC, if it is possible that `this._addon` could be null and we need to add this check, it would also make sense to add an explicit test case for it.

Besides the new test case, I'm wondering if just "silently returning after have removing the stack" is the right way to handle this unexpected scenario, I would probably prefer at least to have an error logged in the console (so that it could be easier to investigate in case it is going to introduce unexpected regressions).

::: toolkit/mozapps/extensions/content/extensions.js:3340
(Diff revision 3)
>          browserOptions.stylesheets = extensionStylesheets;
>        }
>  
>        mm.sendAsyncMessage("Extension:InitBrowser", browserOptions);
>  
> -      browser.loadURI(optionsURL);
> +      browser.loadURIWithFlags(optionsURL, {flags: Ci.nsIWebNavigation.LOAD_FLAGS_REPLACE_HISTORY});

I guess that this issue only happens when the extension is running in the main process (on Linux and OSX), because when the browser is remote, its navigation events should not become part of the about:addons page that contains the browser element.

It is quite unfortunate that this fix is not going to be able to cover a similar issue that I'm pretty sure is also happening when the actual main options page (loaded in a non-remote webextension browser element) navigates to another extension page (e.g. an extension could have a main options page and links that navigate that page to other options sub pages, or when it uses the history API internally).

::: toolkit/mozapps/extensions/test/browser/browser_uninstalling.js:14
(Diff revision 3)
>  
> +let epScope = {};
> +Components.utils.import("resource://gre/modules/ExtensionParent.jsm", epScope);
> +let {ExtensionParent} = epScope;
> +
> +function promiseOptionsPageLoaded() {

We have a similar test helper function named `waitOptionsBrowserInserted` and defined in another about:addons test:

- https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/toolkit/mozapps/extensions/test/browser/browser_webext_options_addon_reload.js#8-22

we could move it into a shared file and reuse in both these test files.

Comment 11

[Andrew Swan [:aswan]]

I'm not actively working on this, unassigning myself. Also clarified the title.
With the HTML rewrite of about:addons now actually imminent, I don't think this is a high priority.

Comment 12

[Mark Striemer [:mstriemer]]

This should've been fixed by bug 1482480. I can't reproduce this.