Closed Bug 1323938 Opened 8 years ago Closed 7 years ago

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

Categories

(WebExtensions :: General, defect, P3)

Product:
WebExtensions
Component:
General
Type:
defect

Tracking

(firefox53 fixed, firefox55 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla53
Tracking Flags:
Tracking Status
firefox53 --- fixed
firefox55 --- fixed

People

(Reporter: frfxtst, Assigned: rhelmer)

References

Details

(Whiteboard: [runtime]triaged)

Attachments

(3 files, 1 obsolete file)

Testcase
3.15 KB, application/zip
Details
Bug 1323938 - pass install reason to startup method for temporary installs
59 bytes, text/x-review-board-request
aswan
: review+
Details
Bug 1323938 - test that install reason is passed to shutdown method for temporary installs
59 bytes, text/x-review-board-request
aswan
: review+
Details
Attached file 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.
I tried it with Chrome where it worked as expected.
Assignee: nobody → aswan
Priority: -- → P3
Whiteboard: triaged
Summary: browser.runtime.oninstalled does not fire → browser.runtime.oninstalled does not fire when extension is installed temporarily
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?
Flags: needinfo?(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.
Flags: needinfo?(rhelmer)
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...
Attachment #8822567 - Flags: review?(aswan) → review+
Assignee: aswan → rhelmer
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
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.
Try was pretty busted last time I tried... giving it another shot.
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c2eb3668be3d
use the ADDON_INSTALL bootstrap reason for temporarily-loaded add-ons r=aswan
https://hg.mozilla.org/mozilla-central/rev/c2eb3668be3d
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox53: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Whiteboard: triaged → [runtime]triaged
Any news here?
(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.
Attachment #8822567 - Attachment is obsolete: true
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
Attachment #8844780 - Flags: review?(aswan) → review+
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
Attachment #8844781 - Flags: review?(aswan) → review+
Pushed by rhelmer@mozilla.com:
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
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: