[WebExtensions] chrome_url_overrides/newtab remains after removing the extension

RESOLVED FIXED in Firefox 56

Status

()

Toolkit
WebExtensions: General
P1
normal
RESOLVED FIXED
2 months ago
a month ago

People

(Reporter: Francois, Assigned: rpl)

Tracking

56 Branch
mozilla57
Points:
---

Firefox Tracking Flags

(firefox56 fixed, firefox57 fixed)

Details

MozReview Requests

()

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

Attachments

(2 attachments)

(Reporter)

Description

2 months ago
Created attachment 8900072 [details]
Screen Shot 2017-08-23 at 10.37.53 AM.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_12_6) AppleWebKit/603.3.8 (KHTML, like Gecko) Version/10.1.2 Safari/603.3.8

Steps to reproduce:

1. Install a webExtension which has a chrome_url_override setting for new tab:
"chrome_url_overrides" : {
    "newtab": "homepage.html"
},

2. Open a new tab and verify that the resource is correctly opened
3. Delete the extension (and refresh the about:addons page to confirm the deletion)
4. Open a new tab


Actual results:

The resource is still called: every new tab I open is a blank page with URL moz-extension://439d6393-754f-a145-a8e9-07dacc44d083/homepage.html


Expected results:

It should have been the regular about:newtab page
(Reporter)

Comment 1

2 months ago
After I delete the addon, every new tab will still go to moz-extension://439d6393-754f-a145-a8e9-07dacc44d083/homepage.html until I close and restart my browser. Then every new tab is going correctly to about:home.
Note that I'm using the Firefox Developer Edition 56.0b4 (64 bit) which is up to date at this time.

Comment 2

2 months ago
When you say "delete the addon/extension", does that mean you clicked "Remove" in about:addons, or something else?
Flags: needinfo?(francoyy)
(Reporter)

Comment 3

2 months ago
Correct. I click "Remove" and then I also refresh the page.

OK, so I got to the bottom of this.
It doesn't happen with a real webExtension.
What I am experiencing is with a hybrid addon, with the structure described here >> https://github.com/mdn/webextensions-examples/tree/master/embedded-webextension-sdk/step1-hybrid-addon

I am working on a hybrid addon in order to copy the important settings from about:config (sdk/preferences/service) and from sdk/simple-storage into WebExtension's storage.local API.

So basically, when I start my extension as a SDK addon with embedded extension, it dooesn't seem to clear correctly the resource set in chrome_url_overrides/newtab. But when I start the extension as a webExtension, it does.
Flags: needinfo?(francoyy)
(Reporter)

Comment 4

2 months ago
Easy way to reproduce:
Clone the repository, and then edit this file:
https://github.com/mdn/webextensions-examples/blob/master/embedded-webextension-sdk/step1-hybrid-addon/webextension/manifest.json
Just to add the following lines:
"chrome_url_overrides" : {
    "newtab": "popup.html"
  },

(the resource is already there).

After that start it using jpm xpi or jpm run
When the addon is installed, you can see every new tab opens popup.html
After you delete the addon, it keeps trying to open popup.html

// Francois

Comment 5

2 months ago
What version of Firefox are you using?  This sounds like bug 1372750 which is fixed in 56 (which is currently beta)...
Flags: needinfo?(francoyy)

Comment 6

2 months ago
BTW, I notice that the SDK webextension module doesn't pass startup reason to the embedded webextension, but its probably too late to do anything about that.  Luca, what do you think?
Flags: needinfo?(lgreco)
(Reporter)

Comment 7

2 months ago
Recording: https://drive.google.com/open?id=0BxRDJAHTldqMVkxiTTJ0TkY4VjQ
I'm using Firefox Developer edition 56.0b4 (64-bit), it says it's up to date
Flags: needinfo?(francoyy)
(Reporter)

Comment 8

2 months ago
For the startup reason, I got it working with a workaround - since this is a hybrid extension whose goal is so send some Legacy API data into the WebExtension storage, I also pass the startup reason when I do the postMessage

https://github.com/mdn/webextensions-examples/blob/master/embedded-webextension-sdk/step1-hybrid-addon/lib/user-data-storage.js#L8

adding loadReason: require('sdk/self').loadReason to it.
Comment hidden (mozreview-request)
(Assignee)

Comment 10

2 months ago
I digged into it and while the SDK webextension module doesn't need to explicitly pass the reason (because it happens internally in the XPIProvider.jsm), the startup/shutdown LegacyExtensionUtils.jsm should pass the reason to the embedded webextension startup/shutdown methods (which also means that this issue is not specific to the SDK hybrid addons, also the hybrid bootstrapped addons have the same issue).

The attached patch contains the change needed to ensure that the startup/shutdown reason reaches the embedded webextension.
Assignee: nobody → lgreco
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(lgreco)
Priority: -- → P1
(Assignee)

Updated

2 months ago
Attachment #8900265 - Flags: review?(aswan)

Comment 11

2 months ago
mozreview-review
Comment on attachment 8900265 [details]
Bug 1392872 - Fix missing startup/shutdown reason in LegacyExtensionsUtils.

https://reviewboard.mozilla.org/r/171664/#review177286
Attachment #8900265 - Flags: review?(aswan) → review+

Comment 12

2 months ago
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/aa772cbe4a8e
Fix missing startup/shutdown reason in LegacyExtensionsUtils. r=aswan
Backed out for eslint failures in LegacyExtensionsUtils.jsm:

https://hg.mozilla.org/integration/autoland/rev/3a2eca3f933957b2198d0957155d19189022aa6e

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=aa772cbe4a8ea772c24205140920fa030d3a5a9f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=125496834&repo=autoland

[task 2017-08-24T11:37:57.207006Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/extensions/LegacyExtensionsUtils.jsm:128:3 | Missing JSDoc for parameter 'reason'. (valid-jsdoc)
[task 2017-08-24T11:37:57.207128Z] TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/toolkit/components/extensions/LegacyExtensionsUtils.jsm:200:3 | Missing JSDoc for parameter 'reason'. (valid-jsdoc)
Flags: needinfo?(lgreco)
Comment hidden (mozreview-request)
(Assignee)

Comment 15

2 months ago
Missing jsdoc for the new reason parameter added in the updated patch.
Flags: needinfo?(lgreco)

Comment 16

2 months ago
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/28c558b52ec0
Fix missing startup/shutdown reason in LegacyExtensionsUtils. r=aswan

Comment 17

2 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/28c558b52ec0
Status: ASSIGNED → RESOLVED
Last Resolved: 2 months ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
(Reporter)

Comment 18

2 months ago
Just want to say congrats for the incredible speed of responding, fixing the bug and even fixing another thing (loadReason) in such a short time. That is awesome!
Duplicate of this bug: 1394111
(Assignee)

Comment 20

2 months ago
Comment on attachment 8900265 [details]
Bug 1392872 - Fix missing startup/shutdown reason in LegacyExtensionsUtils.

Approval Request Comment
[Feature/Bug causing the regression]: 
Bug 1372750 has applied the changes needed in the XPIProvider to propagate the startup/shutdown reason from the `callBootstrapMethod` to the LegacyExtensionsUtils.jsm's EmbeddedExtension instance, but it didn't applied to the EmbeddedExtension class the changes needed to ensure that this reason is then propagated to the embedded webextension instance.

[User impact if declined]: 
The startup/shutdown of an embedded webextension can be affected by this issue, especially when the embedded webextension uses WebExtensions APIs and feature that looks at the startup/shutdown reasons (e.g. chrome_url_overrides.newtab is not cleared after removing an hybrid extension), which is especially important in Firefox 56, where we can expect that a number of add-ons could be migrating their users from the legacy to the webextensions-only version of the add-on using an hybrid extension to migrate the user data.

[Is this code covered by automated tests?]:
No (there are existing tests for the hybrid extensions in general but none of them is explicitly checking the startup/shutdown reason)

[Has the fix been verified in Nightly?]:
Yes

[Needs manual test from QE? If yes, steps to reproduce]: 
Yes, it can be verified using the STR in Comment 0.

[List of other uplifts needed for the feature/fix]: 
None

[Is the change risky?]:
Low

[Why is the change risky/not risky?]:
The fix is small, it can only affect on the hybrid add-ons, and it is limited to introduce the missing reason parameter to the existent EmbeddedExtension startup/shutdown methods.

[String changes made/needed]: 
None
Attachment #8900265 - Flags: approval-mozilla-beta?
status-firefox56: --- → affected
Comment on attachment 8900265 [details]
Bug 1392872 - Fix missing startup/shutdown reason in LegacyExtensionsUtils.

Fix a WebExtensions issue. Beta56+.
Attachment #8900265 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 22

2 months ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/04a9ce3155dc
status-firefox56: affected → fixed
(Reporter)

Comment 23

2 months ago
I feel there is a problem in the last update.
Now, whenever I try to load an embedded webExtension which has an "newtab" setting under chrome_url_overrides, it doesn't load the "newtab" at all. It simply shows the default about:newtab page.

I see some error in the browser console, but it doesn't seem to come from my code (since opening the resource should directly initiate from the manifest.json anyway, no code involved).
The error I see is Error: page-thumbnail:error BackgroundPageThumbs.jsm:116:24

I tried to load the very same webExtension without being embedded, and this time it worked just fine.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 24

2 months ago
Sorry, after adding this comment I tried to refresh Firefox (previously I only restarted it).
After the refresh, the behavior is now normal.

Not sure why it didn't work before the refresh, but I think you could ignore my previous comment and close the ticket again... :s
(Reporter)

Comment 25

2 months ago
Hi, I have one question regarding this fix. I am the owner of https://addons.mozilla.org/en-US/firefox/addon/yahoo-homepage/ which has 87k users. As we need to migrate to webExtensions very soon, I must go through the intermediary step of creating an embedded WebExtension within a legacy addon in order to migrate the user data (sdk/simple-storage + sdk/preferences/service) into the webExtension's storage API.
So I need to push this embedded WebExtension very soon to our users, before doing another update later, which will be the pure webExtension (not embedded).

The bug that was fixed in this ticket only applies to FF 56. This means that all our users which currently mostly have FF versions between FF52 and FF55 (depending on their FF auto-update settings) will receive the embedded extension, but if they were to uninstall the extension, it would not properly remove the chrome_url_overrides newtab URL. It means if they delete the extension, every new tab will still try to open the resource URL (because the fix is only for FF56).

Is there anything that can be done for that? Or do we need to accept the fact that FF versions less than 56 won't have that bugfix? thanks.

Comment 26

a month ago
Closing bug as fixed per comment 24.


(In reply to Francois from comment #25)
> The bug that was fixed in this ticket only applies to FF 56. This means that
> all our users which currently mostly have FF versions between FF52 and FF55
> (depending on their FF auto-update settings) will receive the embedded
> extension, but if they were to uninstall the extension, it would not
> properly remove the chrome_url_overrides newtab URL. It means if they delete
> the extension, every new tab will still try to open the resource URL
> (because the fix is only for FF56).
> 
> Is there anything that can be done for that? Or do we need to accept the
> fact that FF versions less than 56 won't have that bugfix? thanks.

The easiest solution is to set the minimum version of the extension to version 56, by adding the following to manifest.json:
"applications": {"strict_min_version": "56.0"}

A more complicated alternative is to use ExtensionPreferencesManager.jsm to clear the extension pref when your add-on unloads, as seen in https://searchfox.org/mozilla-central/source/toolkit/components/extensions/ext-browserSettings.js#19-40.

I recommend the easiest solution, i.e. setting strict_min_version.
Status: REOPENED → RESOLVED
Last Resolved: 2 months agoa month ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.