Closed Bug 1372750 Opened 3 years ago Closed 2 years ago

Embedded WebExtension manifests in startup cache cannot be invalidated

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox55+ wontfix, firefox56+ fixed, firefox57+ verified)

VERIFIED FIXED
mozilla57
Tracking Status
firefox55 + wontfix
firefox56 + fixed
firefox57 + verified

People

(Reporter: _6a68, Assigned: rhelmer)

References

Details

(Whiteboard: [triaged])

Attachments

(4 files, 3 obsolete files)

It turns out that the Screenshots embedded webextension, which manually starts the webextension, cannot invalidate the copy of the webextension manifest.json that is stored in the startup cache. This is preventing a new background script from being detected in certain upgrade cases.

It's not currently possible to pass a reason into the EmbeddedExtension.startup method returned by `LegacyExtensionsUtils.getEmbeddedExtensionFor`.

See https://github.com/mozilla-services/screenshots/issues/3027 for more details and steps to reproduce.
We've found a workaround for this - we're just clearing the StartupCache inside the bootstrap.js for Screenshots.

This isn't a blocker for Screenshots for 55.
Contradicting my last comment, it looks like this is a blocker for Screenshots for 55.

See https://bugzilla.mozilla.org/show_bug.cgi?id=1372310#c13.
Attachment #8878264 - Flags: review?(kmaglione+bmo)
Comment on attachment 8878264 [details]
Bug 1372750 Pass startup/shutdown reasons to embedded webextensions

https://reviewboard.mozilla.org/r/149598/#review154580

I tried this out with Screenshots, and it doesn't fix the bug :(

Note the startup reason for the system add-on upgrade is 1 (`APP_STARTUP`), not `ADDON_UPGRADE`

::: toolkit/components/extensions/LegacyExtensionsUtils.jsm:209
(Diff revision 1)
>      EmbeddedExtensionManager.untrackEmbeddedExtension(this);
>  
>      // If there is a pending startup,  wait to be completed and then shutdown.
>      if (this.startupPromise) {
>        return this.startupPromise.then(() => {
> -        this.extension.shutdown();
> +        this.extension.shutdown(reason);

I don't believe reason is defined here, maybe it was supposed to be added to the `shutdown()` signature?
Comment on attachment 8878264 [details]
Bug 1372750 Pass startup/shutdown reasons to embedded webextensions

https://reviewboard.mozilla.org/r/149598/#review154580

Huh?  That sounds like a separate bug.  But in screenshots you're calling `startup()` manually right?  Can you determine when the webextension has changed and just pass in `ADDON_UPGRADE` manually in that case?

> I don't believe reason is defined here, maybe it was supposed to be added to the `shutdown()` signature?

Whoops, thanks for catching that, but I think its just the `startup()` case that matters for the StartupCache flush issue.
Looks like this may be important or even a blocker for Screenshots in 55.
No longer blocks: 1372310
Priority: -- → P1
Whiteboard: [triaged]
Comment on attachment 8878264 [details]
Bug 1372750 Pass startup/shutdown reasons to embedded webextensions

https://reviewboard.mozilla.org/r/149598/#review155332

Unless I'm missing something, this shouldn't work, since the Extension class expects reason strings which correspond to the names of reason constants, but you're passing the numeric values of those constants instead.

I suspect the tests pass only because the add-on manager sends startupcache-invalidate observer notifications, and the WebExtension framework is already initialized when it does.
Attachment #8878264 - Flags: review?(kmaglione+bmo)
Finishing this up for aswan who is away.
Assignee: nobody → rhelmer
Comment on attachment 8880108 [details]
Bug 1372750 - allow generated embedded extensions to work in xpcshell tests

https://reviewboard.mozilla.org/r/151484/#review156404

::: toolkit/components/extensions/ExtensionTestCommon.jsm:227
(Diff revision 1)
>                    <!-- Firefox -->
>                    <em:targetApplication>
>                        <Description
>                            em:id="{ec8030f7-c20a-464f-9b0e-13a3a9e97384}"
>                            em:minVersion="51.0a1"
>                            em:maxVersion="*"/>
>                    </em:targetApplication>
> +                  <em:targetApplication>
> +                    <Description>
> +                      <em:id>xpcshell@tests.mozilla.org</em:id>
> +                      <em:minVersion>0</em:minVersion>
> +                      <em:maxVersion>*</em:maxVersion>
> +                    </Description>
> +                  </em:targetApplication>

Let's just use "toolkit@mozilla.org". Presumably we'll want to use this in Fennec tests at some point, too.
Attachment #8880108 - Flags: review?(kmaglione+bmo) → review+
Attachment #8878264 - Attachment is obsolete: true
Comment on attachment 8880107 [details]
Bug 1372750 - pass startup/shutdown reasons to embedded webextensions

https://reviewboard.mozilla.org/r/151482/#review156406
Attachment #8880107 - Flags: review?(kmaglione+bmo) → review+
Welp the test still passes without my reason-stringify change, so there's still some work that needs to be done before landing...
As an addon developer, I'd like to highlight that Screenshots is not the only thing affected by this (since a lot of the early comments focus on that).

I've been tearing my hair out trying to debug changes I thought I was making to my embedded manifest only to discover this bug and that this special new startup cache is not cleared when I use the -purgecaches command line option. I'm now resorting to manually deleting the file that contains the ExtensionStartupCache database (storage/permanent/chrome/idb/2029012401EexhtceanCspiuotnrSat.sqlite in my profile) every time I need the cache cleared while I wait for this to be resolved, which is obviously quite inconvenient...

Should it be possible for this new startup cache to be cleared with -purgecaches? If so, should I open a new bug or is it part of this?
Any updates on this bug? Anything I can do to help drag it across the finish line?

Also, see this question from the previous comment:

> Should it be possible for this new startup cache to be cleared with -purgecaches? If so, should I open a new bug or is it part of this?
Flags: needinfo?(rhelmer)
(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #18)
> Any updates on this bug? Anything I can do to help drag it across the finish
> line?


We discussed in IRC but for posterity I am having trouble reproducing this edge case in the unit test... kmag and I discussed landing the fix (which we are confident about) and landing the test later, if it takes much longer. I think it's close to being done at this point, though.

 
> Also, see this question from the previous comment:
> 
> > Should it be possible for this new startup cache to be cleared with -purgecaches? If so, should I open a new bug or is it part of this?


The "new startup cache" being the webextension manifest cache? If so could you please file a new bug for this?
Flags: needinfo?(rhelmer) → needinfo?(jhirsch)
Blocks: 1383064
Duplicate of this bug: 1383064
I have been stuck on this, trying to teach our test code to support sideloaded webextensions. I'm going to split that bit out to separate bug and land the code here so we don't need to keep waiting on that.
Pushed by rhelmer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/05ba0ff87cba
pass startup/shutdown reasons to embedded webextensions r=kmag
https://hg.mozilla.org/integration/autoland/rev/faaf89285517
allow generated embedded extensions to work in xpcshell tests r=kmag
https://hg.mozilla.org/mozilla-central/rev/05ba0ff87cba
https://hg.mozilla.org/mozilla-central/rev/faaf89285517
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla57
Sounds like this needs a Beta approval request whenever you're comfortable doing so.
Flags: needinfo?(rhelmer)
Comment on attachment 8880107 [details]
Bug 1372750 - pass startup/shutdown reasons to embedded webextensions

Approval Request Comment
[Feature/Bug causing the regression]: 1372750
[User impact if declined]: screenshots team will have to continue to apply special workarounds in their code
[Is this code covered by automated tests?]: yes via existing test coverage - edge case here is a failure to flush the manifest cache specifically for embedded webextensions that ship with Firefox or are side-loaded, our test framework needs some additional work to exercise the edge case effectively
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: yes, contact me and/or screenshots team - should just require ensuring that new manifest changes for screenshots extension show up after app update
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: this patch only has an effect for embedded webextensions, and just triggers manifest cache invalidation
[String changes made/needed]: none
Flags: needinfo?(rhelmer)
Attachment #8880107 - Flags: approval-mozilla-beta?
Comment on attachment 8880108 [details]
Bug 1372750 - allow generated embedded extensions to work in xpcshell tests

Same as comment 29
Attachment #8880108 - Flags: approval-mozilla-beta?
Comment on attachment 8880107 [details]
Bug 1372750 - pass startup/shutdown reasons to embedded webextensions

We certainly want mozilla webextensions to be able to update. Let's uplift this to beta.
Attachment #8880107 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Comment on attachment 8880108 [details]
Bug 1372750 - allow generated embedded extensions to work in xpcshell tests

Thanks for fixing the tests too.
Attachment #8880108 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to Robert Helmer [:rhelmer] from comment #29)
> Comment on attachment 8880107 [details]
> Bug 1372750 - pass startup/shutdown reasons to embedded webextensions
> 
> Approval Request Comment
> [Feature/Bug causing the regression]: 1372750
> [User impact if declined]: screenshots team will have to continue to apply
> special workarounds in their code
> [Is this code covered by automated tests?]: yes via existing test coverage -
> edge case here is a failure to flush the manifest cache specifically for
> embedded webextensions that ship with Firefox or are side-loaded, our test
> framework needs some additional work to exercise the edge case effectively
> [Has the fix been verified in Nightly?]: no
> [Needs manual test from QE? If yes, steps to reproduce]: yes, contact me
> and/or screenshots team - should just require ensuring that new manifest
> changes for screenshots extension show up after app update
> [List of other uplifts needed for the feature/fix]: none
> [Is the change risky?]: no
> [Why is the change risky/not risky?]: this patch only has an effect for
> embedded webextensions, and just triggers manifest cache invalidation
> [String changes made/needed]: none

Hello, Robert! What are the steps required in order to reproduce / verify this issue and that you mentioned in comment 29? Thanks!
Flags: needinfo?(rhelmer)
(In reply to Iulia Cristescu, QA [:JuliaC] from comment #34)
> (In reply to Robert Helmer [:rhelmer] from comment #29)
> > Comment on attachment 8880107 [details]
> > Bug 1372750 - pass startup/shutdown reasons to embedded webextensions
> > 
> > Approval Request Comment
> > [Feature/Bug causing the regression]: 1372750
> > [User impact if declined]: screenshots team will have to continue to apply
> > special workarounds in their code
> > [Is this code covered by automated tests?]: yes via existing test coverage -
> > edge case here is a failure to flush the manifest cache specifically for
> > embedded webextensions that ship with Firefox or are side-loaded, our test
> > framework needs some additional work to exercise the edge case effectively
> > [Has the fix been verified in Nightly?]: no
> > [Needs manual test from QE? If yes, steps to reproduce]: yes, contact me
> > and/or screenshots team - should just require ensuring that new manifest
> > changes for screenshots extension show up after app update
> > [List of other uplifts needed for the feature/fix]: none
> > [Is the change risky?]: no
> > [Why is the change risky/not risky?]: this patch only has an effect for
> > embedded webextensions, and just triggers manifest cache invalidation
> > [String changes made/needed]: none
> 
> Hello, Robert! What are the steps required in order to reproduce / verify
> this issue and that you mentioned in comment 29? Thanks!

Jared would you be able to help QA test this scenario? Thanks!
Flags: needinfo?(rhelmer)
Attached file screenshots-17.0.0-97-g0eebf8be.xpi (obsolete) —
I'm attempting to attach an unsigned Screenshots xpi that changes the icon path, and replaces the usual icon with a clown face emoji.

If manifest cache invalidation works, the icon should appear; I see it on today's Nightly.
If manifest cache invalidation doesn't work, the icon will be missing; I can see this on today's Dev Edition.

To install this addon, you'll need to visit about:config and flip some prefs:
  - set xpinstall.signatures.required to false (allow unsigned xpi)
  - set extensions.legacy.enabled to true (allow embedded webextensions)
  - if extensions.screenshots.system-disabled exists (it shouldn't), set it to false

Needinfo me if there's anything else I can do :-)
Flags: needinfo?(jhirsch)
Here's a screenshot of the clown emoji in the page action menu
(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #37)
> Created attachment 8903346 [details]
> screenshots-17.0.0-97-g0eebf8be.xpi
> 
> I'm attempting to attach an unsigned Screenshots xpi that changes the icon
> path, and replaces the usual icon with a clown face emoji.
> 
> If manifest cache invalidation works, the icon should appear; I see it on
> today's Nightly.
> If manifest cache invalidation doesn't work, the icon will be missing; I can
> see this on today's Dev Edition.
> 
> To install this addon, you'll need to visit about:config and flip some prefs:
>   - set xpinstall.signatures.required to false (allow unsigned xpi)
>   - set extensions.legacy.enabled to true (allow embedded webextensions)
>   - if extensions.screenshots.system-disabled exists (it shouldn't), set it
> to false
> 
> Needinfo me if there's anything else I can do :-)

(In reply to Jared Hirsch [:_6a68] [:jhirsch] from comment #38)
> Created attachment 8903347 [details]
> Screen Shot 2017-08-31 at 2.45.25 PM.png
> 
> Here's a screenshot of the clown emoji in the page action menu

Tried to verify this fox on 56.0 build6 (20170926190823), 57.0b4 build1 (20170928180207) and on 58.0a1 (2017-10-01), but the addon couldn't be installed on 56 and 57 builds (see the screenshot https://goo.gl/bpZwk3). Also, on 58 build, the addon is installed, but the icon doesn't appear (see the screencast https://goo.gl/UrMPDd). 
Jared, any thoughts about this?
Flags: needinfo?(jhirsch)
Sorry, my screenshot was a bit unclear. The clown emoji should appear in the _page action_ menu, but not the context menu.

I forgot that Release and Beta require addons to be signed. We're supposed to have unbranded builds to use for testing, but it looks like the builds haven't been updated in a while[1]. I'll ping wezhou to see if we can get the test addon signed, which is probably the quickest path to a solution.

[1] https://wiki.mozilla.org/Add-ons/Extension_Signing#Unbranded_Builds
Flags: needinfo?(jhirsch)
Hi wezhou, could you sign the attached addon (screenshots-17.0.0-97-g0eebf8be.xpi)? Release QA needs it for verifying a bug fix.
Flags: needinfo?(wezhou)
Attached file signed.8903346.xpi (obsolete) —
Attachment signed. Please test.
Flags: needinfo?(wezhou)
Hmm. The signed addon still doesn't work in beta, despite setting extensions.legacy.enabled to true.

I asked RyanVM about an unbranded copy of beta, and he said that dev edition can be used to test legacy addons. Does that work for release QA? If not, I can look into pushing beta to Try with some flags set.

For release testing, the unbranded release builds should work, they are the ones ending in -add-on-devel: https://tools.taskcluster.net/index/gecko.v2.mozilla-release.latest.firefox

Iulia, mind giving these builds a try with the signed addon in comment 42, and setting the prefs from comment 37?
Flags: needinfo?(iulia.cristescu)
Sorry, I don't think I was specific enough in my first request. Could this be signed with the mozilla key, not the AMO key? We need to verify a system addon update correctly invalidates the manifest.
Flags: needinfo?(wezhou)
Never mind - I think the test pilot key is the right one. I'll bug Wil to get it signed.
Flags: needinfo?(wezhou)
Clearing ni for JuliaC, too, until I get the addon correctly signed
Flags: needinfo?(iulia.cristescu)
OK, here's the test addon, signed with the correct test pilot key. This should work for testing in beta and release.
Attachment #8903346 - Attachment is obsolete: true
Attachment #8914570 - Attachment is obsolete: true
Flags: needinfo?(iulia.cristescu)
Used the test add-on provided in comment 47,  57.0b5 build1 (20171002181526) and 56.0.1 build2 (20171002220106) builds. The add-on was properly installed, but the page action menu isn't available on the 56 builds. I can confirm the fix for the 57 build, across platforms (Windows 10 x64, Ubuntu 16.04 x64 and Mac OS X 10.11.6).
Flags: needinfo?(iulia.cristescu)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.