1359558 - When the browser is closed after loading a temporary add-on, the add-on is not sent an "UNINSTALLED" reason

Status: RESOLVED FIXED

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

Description

[Mike Kaply [:mkaply]]

When you close the browser with a temporary add-on, the add-on is going to be uninstalled.

You should get this notification in your add-on.

Right now you only get APP_SHUTDOWN.

This means you can't truly test what happens when your add-on is uninstalled.

Comment 1

[Mike Kaply [:mkaply]]

Might be fixed by bug 1356826

Comment 2

[Julian Descottes [:jdescottes]]

Mike: Just to be sure, do you really want to test what happens when an addon is uninstalled while the browser closes? Otherwise, if the goal is just to test the addon uninstall, can't you do that by removing/disabling the addon in about:addons? Maybe in this case what you actually want is an additional action in about:debugging to remove an addon?

Comment 3

[Julian Descottes [:jdescottes]]

(did not intend to move the bug ... reverting to about:debugging)

For reference Bug 1338827 was filed to request for a remove button to be added to about:debugging.

Comment 4

[Mike Kaply [:mkaply]]

> Mike: Just to be sure, do you really want to test what happens when an addon is uninstalled while the browser closes? Otherwise, if the goal is just to test the addon uninstall, can't you do that by removing/disabling the addon in about:addons? Maybe in this case what you actually want is an additional action in about:debugging to remove an addon?

It's not really just about testing.

The problem is that if you load the add-on in about:debugging and then close the browser, your addon is gone but shutdown isn't done properly, so the add-on is left in an invalid state.

If we're going to encourage people to use about:debugging, it needs to do the right thing when the browser is closed.

Comment 5

[Julian Descottes [:jdescottes]]

Sure, makes sense. 

In this case though I'm moving the bug to Toolkit / Addons Manager (about:debugging is relying on the AddonManager to perform the temporary install). And as I said, for testing through about debugging we already have bug 1338827

cc-ing Luca who might also have some input.

Comment 6

[Robert Helmer [:rhelmer]]

Is this fixed by bug 1356826 ? 

Temporary add-ons are now uninstalled at shutdown (instead of found to be missing on next startup) and the uninstall reason should be passed now:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca9dcf386ceb81146a0ff101dc35cbee20e212b0

Comment 7

[:shell escalante]

based on comment 6, is this working for you?

Comment 8

[Mike Kaply [:mkaply]]

No, this is still not working for me. 

I have an example addon that removes a search engine at shutdown like this:

function shutdown(data, reason) {
  if (reason == REASONS.ADDON_DISABLE ||
      reason == REASONS.ADDON_UNINSTALL) {
    let engine = Services.search.getEngineByName(kEngineName);
    if (engine) {
      Services.search.removeEngine(engine);
    }
  }
}

and if I close the browser after installing this addon via about:debugging and then restart, the search engine is still there.

Comment 9

[Mike Kaply [:mkaply]]

So uninstall gets called with the ADDON_UNINSTALL reason, but shutdown is still called with APP_SHUTDOWN.

shutdown should be called with ADDON_UNINSTALL.

The reason shutdown is important is because uninstall is not called when you are disabled, so you have to handle things like that inside of shutdown, not uninstall.

Comment 10

[:shell escalante]

Hi Mike, with the backlog we likely won't be fixing before 57 - but do plan on taking.  Does that block you for anything?

Comment 11

[Andrew Swan [:aswan]]

I'm not seeing any more elegant way to fix this than just changing the code here:
http://searchfox.org/mozilla-central/rev/6c2dbacbba1d58b8679cee700fd0a54189e0cf1b/toolkit/mozapps/extensions/internal/XPIProvider.jsm#3377

To do something like
let reason = (addon._installLocation.name == KEY_APP_TEMPORARY) ? BOOTSTRAP_REASONS.ADDON_UNINSTALL : BOOTSTRAP_REASONS.APP_SHUTDOWN

Ideally that would be accompanied by some tests.  If its pressing and you can assemble the patch, I'd be happy to review it.  Otherwise this goes in the (very long) backlog.

Comment 12

[Mike Kaply [:mkaply]]

> Hi Mike, with the backlog we likely won't be fixing before 57 - but do plan on taking.  Does that block you for anything?

This doesn't block me. My concern is that with the removal of developer edition and move the WebExtensions, we're going to get a lot more usage of about:debugging so we want it to work well.

Comment 13

[Bob Silverberg [:bsilverberg]]

This is causing real-world issues for ext-url-overrides so it needs to be fixed. I'll take it and implement aswan's suggestion.

Comment 14

[Tomislav Jovanovic :zombie]

So, I just stumbled on a situation that this bug is going to make fairly apparent:

1) Have your addon installed from AMO
2) temporarily load a new version via about:debugging
3) click remove (or simply restart the browser after the proposed "fix")

This does/will treat that as an UNINSTALL, and wipe all sorts of stuff from the addon, including browser.storage.


:aswan over irc:

> seems like the problem is here
> http://searchfox.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2342-2360
> if that checked before removing and used an appropriate reason
> (ie UPGRADE or DOWNGRADE instead of UNINSTALL/INSTALL) then you'd be fine

Comment 15

[Bob Silverberg [:bsilverberg]]

I discussed comment 14 with zombie on irc and we came to the conclusion that perhaps his described case should just be unsupported. It seems like it would be difficult to assume what behaviour a user expects in that scenario.

In other news, I tried the suggested solution from comment 11, but it doesn't work in my case. The code in ext-url-overrides uses callOnClose to remove settings when an extension is uninstalled, but I don't think callOnClose will be triggered by the code in comment 11 because that code runs on browser startup rather than shutdown. It seems like it would be better to make sure that temporary extensions are uninstalled on shutdown rather than on startup, and that the appropriate event is dispatched. Knowing very little about AoM, I'm not sure what needs to be done to support that.

Andrew, do you have some suggestions for approaching this?

Comment 16

[Andrew Swan [:aswan]]

Regarding zombie's use case, it doesn't seem to me like it should be unsupported.  Assuming that the temporary installation happens with an UPGRADE or DOWNGRADE reason (I haven't looked at the code to verify that it is) it seems pretty clear to me that the uninstall should have the complementary reason and that extension storage should not be cleared.  What other behaviors might a user reasonably expect?

On the second point, the code referenced in comment 11 has become obsolete since the comment was made, see http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2338-2361

Comment 17

[Bob Silverberg [:bsilverberg]]

(In reply to Andrew Swan [:aswan] from comment #16)
> Regarding zombie's use case, it doesn't seem to me like it should be
> unsupported.  Assuming that the temporary installation happens with an
> UPGRADE or DOWNGRADE reason (I haven't looked at the code to verify that it
> is) it seems pretty clear to me that the uninstall should have the
> complementary reason and that extension storage should not be cleared.  What
> other behaviors might a user reasonably expect?
> 

Fair enough.

> On the second point, the code referenced in comment 11 has become obsolete
> since the comment was made, see
> http://searchfox.org/mozilla-central/rev/
> 3a3af33f513071ea829debdfbc628caebcdf6996/toolkit/mozapps/extensions/internal/
> XPIProvider.jsm#2338-2361

I can see that code, and it looks like it should be working, but it is not. When closing the browser with an extension temporarily loaded, I am seeing APP_SHUTDOWN as the extension.shutdownReason, not ADDON_UNINSTALL. I realize that this is just restating the original bug, but I am not seeing any suggestions here for how to address the fact that it isn't working.

Comment 18

[Tomislav Jovanovic :zombie]

(In reply to Andrew Swan [:aswan] from comment #16)
> Regarding zombie's use case, it doesn't seem to me like it should be
> unsupported.  Assuming that the temporary installation happens with an
> UPGRADE or DOWNGRADE reason (I haven't looked at the code to verify that it
> is) it seems pretty clear to me that the uninstall should have the
> complementary reason and that extension storage should not be cleared.  What
> other behaviors might a user reasonably expect?

I'm not sure that any devs really expect and handle that case (I searched DXR, and only found mentions in SDK/boilerplate).

We should at least add a warning to about:debugging in such case, as it would be unexpected to some.

Comment 19

[Bob Silverberg [:bsilverberg]]

This has the potential to cause real bugs for users who install extensions temporarily. For example, if a user has permanent extension A, which uses a setting (e.g., overrides the new tab), and then installs extension B temporarily, which uses the same setting, then when the browser is restarted it will appear as if extension B still has control and therefore extension A will not work as expected.

For that reason I have changed the priority to P1 as I think we need to fix this, ideally in time for the merge, and, if possible, uplift to beta before the merge as well. I have also unassigned myself as I don't think I'm going to have time to figure this out and fix it due to my lack of experience with and knowledge of the Add-ons Manager codebase.

Andrew or Rob, could either of you take this?

Comment 20

[Robert Helmer [:rhelmer]]

Per IRC, Andrew volunteered to take a look.

Comment 21

[Andrew Swan [:aswan]]

I was too hasty when I wrote comment 16, the code I referenced there calls the uninstall() method, which is distinct from the code in comment 11 that calls the shutdown() method.  I think comment 11 is still correct (and comment 15 isn't right, that code is in an observer that runs at shutdown.  The observer is created at startup but runs at shutdown).
I'll try to put together a patch with a test.

Comment 22

[Andrew Swan [:aswan]]

Attachment: Bug 1359558 Part 1: Move webextension theme test to test_webextension.js

This test is about webextension themes but it was stuck into an
unrelated test of temporary addon installation.  Move it to a
more appropriate place.

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

Comment 23

[Andrew Swan [:aswan]]

Attachment: Bug 1359558 Part 2 Pass appropriate bootstrap reasons when temporarily installing an addon on top of an existing one

Prior to this patch we always passed APP_SHUTDOWN as the reason when
shutting down and uninstall a temporary addon, even if the same addon
was installed permanently.  Now we send an appropriate reason.

Also untangled a bunch of stuff that had been added to the test case
for temporary-addon-installed-over-permanently-installed-addon and
then extended the basic test to cover this scenario.

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

Comment 24

[Andrew Swan [:aswan]]

Attachment: Bug 1359558 Part 3 Add a test for extension newtab overrides with temporary addon installation

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

Comment 25

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8892220 [details]
Bug 1359558 Part 3 Add a test for extension newtab overrides with temporary addon installation

https://reviewboard.mozilla.org/r/163174/#review168966

This looks good, thanks Andrew. The specific use case I was looking at was when the browser was restarted, but I gather that restarting the addons manager does basically the same thing.

Comment 26

[Kris Maglione [:kmag]]

Comment on attachment 8892218 [details]
Bug 1359558 Part 1: Move webextension theme test to test_webextension.js

https://reviewboard.mozilla.org/r/163170/#review170090

Comment 27

[Kris Maglione [:kmag]]

Comment on attachment 8892219 [details]
Bug 1359558 Part 2 Pass appropriate bootstrap reasons when temporarily installing an addon on top of an existing one

https://reviewboard.mozilla.org/r/163172/#review170096

::: toolkit/mozapps/extensions/internal/XPIProvider.jsm:2362
(Diff revision 1)
> +        let locations = Array.from(XPIStates.db.values())
> +                             .filter(loc => loc != tempLocation);
> +        let existing = XPIStates.findAddon(id, locations);
> +        if (existing) {
> +          reason = Services.vc.compare(addon.version, existing.version) < 0 ?
> +                        BOOTSTRAP_REASONS.ADDON_UPGRADE :
> +                        BOOTSTRAP_REASONS.ADDON_DOWNGRADE;
> +        }

Please move this repeated code into a helper function somewhere.

Comment 28

[Andrew Swan [:aswan]]

Comment on attachment 8892219 [details]
Bug 1359558 Part 2 Pass appropriate bootstrap reasons when temporarily installing an addon on top of an existing one

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

Comment 29

[Andrew Swan [:aswan]]

Comment on attachment 8892220 [details]
Bug 1359558 Part 3 Add a test for extension newtab overrides with temporary addon installation

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

Comment 30

[Andrew Swan [:aswan]]

Here's a good try run for the latest revision:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4654e4f4ad252f3eeb51c7dbcd2593be4919db1e

Comment 31

[Andrew Swan [:aswan]]

Comment on attachment 8892219 [details]
Bug 1359558 Part 2 Pass appropriate bootstrap reasons when temporarily installing an addon on top of an existing one

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

Comment 32

[Andrew Swan [:aswan]]

Comment on attachment 8892220 [details]
Bug 1359558 Part 3 Add a test for extension newtab overrides with temporary addon installation

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

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b039b6974405
Part 1: Move webextension theme test to test_webextension.js r=kmag
https://hg.mozilla.org/integration/autoland/rev/652c9d3a4f0d
Part 2 Pass appropriate bootstrap reasons when temporarily installing an addon on top of an existing one r=kmag
https://hg.mozilla.org/integration/autoland/rev/80293ee27bc5
Part 3 Add a test for extension newtab overrides with temporary addon installation r=bsilverberg

Comment 34

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b039b6974405
https://hg.mozilla.org/mozilla-central/rev/652c9d3a4f0d
https://hg.mozilla.org/mozilla-central/rev/80293ee27bc5

Comment 35

[marius.santa]

Attachment: webext uninstalled.gif

This bug is reproducible on Firefox 57.0b9 (20171016185129) under Windows 10 64bit. Clean profile is required in order to reproduce.

Please see attached gif.

Comment 36

[Tomislav Jovanovic :zombie]

I'm guessing you meant to reassign to Andrew.

Comment 37

[Andrew Swan [:aswan]]

I'm sorry I don't really understand what I'm supposed to be getting from the attached gif, can you explain please?

Comment 38

[marius.santa]

Steps:
1. Install the second to last version of a webextension(https://addons.mozilla.org/en-US/firefox/addon/nectar-browser-add-on/versions/?page=1#version-1.0.3).
2. Download the latest version of that webextension(https://addons.mozilla.org/en-US/firefox/addon/nectar-browser-add-on/versions/?page=1#version-1.0.4).
3. Use "Load Temporary Add-on" to install the previously downloaded version of the webextension.
4. Restart the browser.
5. Go to "about:addons>Extensions".

Actual results:
The installed webextension is uninstalled.

Expected results:
The webextension installed at step 1 is displayed inside the "Extensions" section.

Notes:
It overwrites the existing extension with the new version and after restart the extension installed from AMO is uninstalled together with the temporary loaded xpi.

Comment 39

[Andrew Swan [:aswan]]

This is sporadic but I did manage to reproduce it once, when this happened during shutdown:

1509064130866	addons.manager	WARN	Failure during shutdown of XPIProvider: Error: Unable to arm timer, the object has been finalized. (resource://gre/modules/DeferredTask.jsm:196:13) JS Stack trace: [e-mail]:196:13 < [e-mail]:279:12 < [e-mail]:1740:5 < [e-mail]:965:7 < [e-mail]:1010:9 < [e-mail]:2368:26 < async*_startProvider/AMProviderShutdown/<@AddonManager.jsm:745:21 < [e-mail]:743:16 < [e-mail]:703:23 < [e-mail]:850:7 < [e-mail]:834:28 < [e-mail]:1122:15 < Async*[e-mail]:703:23 < [e-mail]:850:7 < [e-mail]:834:28 < [e-mail]:517:17

Comment 40

[Andrew Swan [:aswan]]

The issue first reported in comment 35 is separate from the issue that was originally addressed by this bug, but it will be addressed by the patches for bug 1409245.  I don't want to mark this a duplicate since the original issue is distinct but consider the new issue a duplicate.

Comment 41

[marius.santa]

I was unable to see the logs for temporary add-ons uninstall after browser restart in the console.
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.

Comment 42

[Andrew Swan [:aswan]]

this has automated tests