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

RESOLVED FIXED in Firefox 53

Status

()

Toolkit
WebExtensions: General
P3
normal
RESOLVED FIXED
8 months ago
5 months ago

People

(Reporter: testit, Assigned: rhelmer)

Tracking

unspecified
mozilla53
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox53 fixed, firefox55 fixed)

Details

(Whiteboard: [runtime]triaged)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

8 months ago
Created attachment 8819223 [details]
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.
(Reporter)

Comment 1

8 months ago
I tried it with Chrome where it worked as expected.

Updated

8 months ago
Assignee: nobody → aswan
Priority: -- → P3
Whiteboard: triaged

Updated

8 months ago
Summary: browser.runtime.oninstalled does not fire → browser.runtime.oninstalled does not fire when extension is installed temporarily

Comment 2

8 months ago
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)
(Assignee)

Comment 3

8 months ago
(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 hidden (mozreview-request)

Comment 5

8 months ago
mozreview-review
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)

Updated

8 months ago
Assignee: aswan → rhelmer
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 6

8 months ago
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 hidden (mozreview-request)
(Assignee)

Comment 8

8 months ago
Try was pretty busted last time I tried... giving it another shot.
Comment hidden (mozreview-request)

Comment 10

8 months ago
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

Comment 11

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c2eb3668be3d
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox53: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
(Reporter)

Comment 12

7 months ago
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 → ---

Updated

6 months ago
Whiteboard: triaged → [runtime]triaged
(Reporter)

Comment 13

6 months ago
Any news here?
(Assignee)

Comment 14

6 months ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

6 months ago
Attachment #8822567 - Attachment is obsolete: true

Comment 17

5 months ago
mozreview-review
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 18

5 months ago
mozreview-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+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 21

5 months ago
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

Comment 22

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/bede8c23e522
https://hg.mozilla.org/mozilla-central/rev/027247e0a84d
Status: REOPENED → RESOLVED
Last Resolved: 7 months ago5 months ago
status-firefox55: --- → fixed
Resolution: --- → FIXED

Updated

5 months ago
Duplicate of this bug: 1278025
You need to log in before you can comment on or make changes to this bug.