1323938 - browser.runtime.oninstalled does not fire when extension is installed temporarily

Status: RESOLVED FIXED

(WebExtensions :: General, defect, P3)

Description

[testit]

Attachment: Testcase

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:50.0) Gecko/20100101 Firefox/50.0
Build ID: 20161208153507

Steps to reproduce:

Download and unzip the attached zip file and load the add-on via "about:debugging".


Actual results:

A toolbar button is added. No badge text is displayed.


Expected results:

The badge text "New" should be displayed. If you reload the add-on the badge text "Update" should be displayed. The add-on registered as a listener to the oninstalled event but it seems to be that the event never fires.

Comment 1

[testit]

I tried it with Chrome where it worked as expected.

Comment 2

[Andrew Swan [:aswan]]

This appears to be because we use the startup reason ADDON_ENABLE instead of ADDON_INSTALL for temporarily installed addons:
http://searchfox.org/mozilla-central/rev/51aa673e28802fe6ea89f4793721fc55334a6ac8/toolkit/mozapps/extensions/internal/XPIProvider.jsm#4152-4153

Rob, is there some reason for this that I'm overlooking?  Any objection to changing it to _INSTALL?

Comment 3

[Robert Helmer [:rhelmer]]

(In reply to Andrew Swan [:aswan] from comment #2)
> This appears to be because we use the startup reason ADDON_ENABLE instead of
> ADDON_INSTALL for temporarily installed addons:
> http://searchfox.org/mozilla-central/rev/
> 51aa673e28802fe6ea89f4793721fc55334a6ac8/toolkit/mozapps/extensions/internal/
> XPIProvider.jsm#4152-4153
> 
> Rob, is there some reason for this that I'm overlooking?  Any objection to
> changing it to _INSTALL?

I agree - ENABLE doesn't seem right here. I can't find much outside of test code listening for either of these anyway (except for webextension code!) so it is probably easy enough to change.

Comment 4

[Robert Helmer [:rhelmer]]

Attachment: Bug 1323938 - use the ADDON_INSTALL bootstrap reason for temporarily-loaded add-ons

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

Comment 5

[Andrew Swan [:aswan]]

Comment on attachment 8822567 [details]
Bug 1323938 - use the ADDON_INSTALL bootstrap reason for temporarily-loaded add-ons

https://reviewboard.mozilla.org/r/101430/#review101952

I delegate my review to try, if its happy, I'm happy...

Comment 6

[Robert Helmer [:rhelmer]]

Comment on attachment 8819223 [details]
Testcase

This test case WFM with the latest patch on this bug.

We should consider adding a test for the WebExtension code that checks this too.

I'll land this when try is happy.

Comment 7

[Robert Helmer [:rhelmer]]

Comment on attachment 8822567 [details]
Bug 1323938 - use the ADDON_INSTALL bootstrap reason for temporarily-loaded add-ons

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

Comment 8

[Robert Helmer [:rhelmer]]

Try was pretty busted last time I tried... giving it another shot.

Comment 9

[Robert Helmer [:rhelmer]]

Comment on attachment 8822567 [details]
Bug 1323938 - use the ADDON_INSTALL bootstrap reason for temporarily-loaded add-ons

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c2eb3668be3d
use the ADDON_INSTALL bootstrap reason for temporarily-loaded add-ons r=aswan

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[testit]

I just tried it with the new build from 2017-01-08.

Now "New" is displayed when the add-on is installed first. When I reload the add-on, "New" is displayed too because the reason which is passed to the listener is in both cases "install". But in the reload case the reason should be "update". That's the way it works in Chrome and which makes much sense for me.

For webextension developers it's important that different reasons are sent so both cases can be tested.

Comment 13

[testit]

Any news here?

Comment 14

[Robert Helmer [:rhelmer]]

(In reply to testit from comment #12)
> I just tried it with the new build from 2017-01-08.
> 
> Now "New" is displayed when the add-on is installed first. When I reload the
> add-on, "New" is displayed too because the reason which is passed to the
> listener is in both cases "install". But in the reload case the reason
> should be "update". That's the way it works in Chrome and which makes much
> sense for me.
> 
> For webextension developers it's important that different reasons are sent
> so both cases can be tested.

This makes sense, thanks. I've got a patch just about ready for this.

Comment 15

[Robert Helmer [:rhelmer]]

Attachment: Bug 1323938 - pass install reason to startup method for temporary installs

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

Comment 16

[Robert Helmer [:rhelmer]]

Attachment: Bug 1323938 - test that install reason is passed to shutdown method for temporary installs

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

Comment 17

[Andrew Swan [:aswan]]

Comment on attachment 8844780 [details]
Bug 1323938 - pass install reason to startup method for temporary installs

https://reviewboard.mozilla.org/r/118106/#review120288

r=me with the changes below

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:507
(Diff revision 1)
> +  equal(install.data.version, "2.0");
> +  equal(install.reason, BOOTSTRAP_REASONS.ADDON_UPGRADE);

i think you want `startup.` and not `install.` here

::: toolkit/mozapps/extensions/test/xpcshell/test_temporary.js:552
(Diff revision 1)
> +  equal(install.data.version, "0.8");
> +  equal(install.reason, BOOTSTRAP_REASONS.ADDON_DOWNGRADE);

same comment as above

Comment 18

[Andrew Swan [:aswan]]

Comment on attachment 8844781 [details]
Bug 1323938 - test that install reason is passed to shutdown method for temporary installs

https://reviewboard.mozilla.org/r/118108/#review120292

Comment 19

[Robert Helmer [:rhelmer]]

Comment on attachment 8844780 [details]
Bug 1323938 - pass install reason to startup method for temporary installs

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

Comment 20

[Robert Helmer [:rhelmer]]

Comment on attachment 8844781 [details]
Bug 1323938 - test that install reason is passed to shutdown method for temporary installs

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/bede8c23e522
pass install reason to startup method for temporary installs r=aswan
https://hg.mozilla.org/integration/autoland/rev/027247e0a84d
test that install reason is passed to shutdown method for temporary installs r=aswan

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/bede8c23e522
https://hg.mozilla.org/mozilla-central/rev/027247e0a84d