Closed Bug 1392872 Opened 7 years ago Closed 7 years ago

[WebExtensions] chrome_url_overrides/newtab remains after removing the extension

Categories

(WebExtensions :: General, defect, P1)

56 Branch
defect

Tracking

(firefox56 fixed, firefox57 fixed)

VERIFIED FIXED
mozilla57
Tracking Status
firefox56 --- fixed
firefox57 --- fixed

People

(Reporter: francoyy, Assigned: rpl)

References

Details

Attachments

(2 files)

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
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.
When you say "delete the addon/extension", does that mean you clicked "Remove" in about:addons, or something else?
Flags: needinfo?(francoyy)
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)
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
What version of Firefox are you using?  This sounds like bug 1372750 which is fixed in 56 (which is currently beta)...
Flags: needinfo?(francoyy)
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)
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)
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.
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
Attachment #8900265 - Flags: review?(aswan)
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+
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)
Missing jsdoc for the new reason parameter added in the updated patch.
Flags: needinfo?(lgreco)
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/28c558b52ec0
Fix missing startup/shutdown reason in LegacyExtensionsUtils. r=aswan
https://hg.mozilla.org/mozilla-central/rev/28c558b52ec0
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
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!
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?
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+
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 → ---
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
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.
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
Closed: 7 years ago7 years ago
Resolution: --- → FIXED
Verified as fixed in FF56 and FF57 (Win7 64 bit) and MacOS 10.13.1. using the steps from comment 0
Marking this bug as verified fixed.
Status: RESOLVED → VERIFIED
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.