Closed Bug 1295567 Opened 4 years ago Closed 4 years ago

System add-ons should always be multiprocessCompatible, and error if not

Categories

(Toolkit :: Add-ons Manager, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: mconley, Assigned: standard8)

Details

(Whiteboard: [system add-on] MPC, triaged)

Attachments

(1 file)

A few days ago, Standard8 went back and marked all of our shipped system add-ons as multiprocessCompatible.

(See bug 1291662 for example)

This means that the system add-ons no longer use the CPOW shims, which is good.

As more and more components start being shipped as system add-ons in Firefox, we should probably make sure that system add-ons are _always_ multiprocessCompatible. In fact, I'd argue that we shouldn't load the system add-on unless multiprocessCompatible is set to true. Either that, or we just assume that system add-ons are multiprocessCompatible.
(In reply to Mike Conley (:mconley) - (Needinfo me!) from comment #0)
> As more and more components start being shipped as system add-ons in
> Firefox, we should probably make sure that system add-ons are _always_
> In fact, I'd argue that we shouldn't load the system
> add-on unless multiprocessCompatible is set to true.


I think we should do this - I'd rather be explicit than implicit. We should log an error and refuse to load any system add-on that isn't marked as 1) restartless and 2) multiprocess (or a WebExtension which implies both.)
Assignee: nobody → rhelmer
Status: NEW → ASSIGNED
bug 1291350 is to make all system add-ons MPC.  this is to give an error if they are not in client Add-ons Manager.
Priority: -- → P2
Summary: System add-ons should always be multiprocessCompatible → System add-ons should always be multiprocessCompatible, and error if not
Whiteboard: [system add-on] MPC, triaged
Rob, can I take this off you?

I actually wrote a unit test a little while ago that would check all system add-ons were multiprocess enabled. Is that enough, or do we think we should go with additional checks in the add-ons manager? (I guess additional checks might be better for those add-ons that only get published to older versions).
Flags: needinfo?(rhelmer)
(In reply to Mark Banner (:standard8) from comment #3)
> Rob, can I take this off you?
> 
> I actually wrote a unit test a little while ago that would check all system
> add-ons were multiprocess enabled. Is that enough, or do we think we should
> go with additional checks in the add-ons manager? (I guess additional checks
> might be better for those add-ons that only get published to older versions).

Sure thing!

I think probably all that is needed is a check in http://searchfox.org/mozilla-central/rev/572e74ee991bbfd812766b4524237eb77577a4b1/toolkit/mozapps/extensions/internal/XPIProvider.jsm#8541 along with a test, but I'll leave it up to you to verify that :)
Assignee: rhelmer → standard8
Flags: needinfo?(rhelmer)
(In reply to Mark Banner (:standard8) from comment #3)
> Rob, can I take this off you?
> 
> I actually wrote a unit test a little while ago that would check all system
> add-ons were multiprocess enabled. Is that enough, or do we think we should
> go with additional checks in the add-ons manager? (I guess additional checks
> might be better for those add-ons that only get published to older versions).

To answer your question more directly - yes we should verify it, probably in the method I linked to. I think verifying the in-tree system add-ons is great too but we definitely do have out-of-tree types as well (especially one-off hotfixes etc) and it'd be better to just consistently reject these.

We should definitely log this at the WARN level so it shows up in non-debugging logging, as the only real concern I have here is that it'll be confusing for folks doing testing.
(In reply to Robert Helmer [:rhelmer] from comment #4)
> I think probably all that is needed is a check in
> http://searchfox.org/mozilla-central/rev/
> 572e74ee991bbfd812766b4524237eb77577a4b1/toolkit/mozapps/extensions/internal/
> XPIProvider.jsm#8541 along with a test, but I'll leave it up to you to
> verify that :)

That code only seems to apply to the update case. I haven't found the bit for built-in add-ons yet.

Am I right in thinking there's only two system add-on tests currently? test_system_update.js & test_system_reset.js

Also, we're going to need to regenerate the test xpis and re-sign them. Any ideas how this is done, I couldn't see any documentation on it?
Flags: needinfo?(rhelmer)
(In reply to Mark Banner (:standard8) from comment #6)
> (In reply to Robert Helmer [:rhelmer] from comment #4)
> > I think probably all that is needed is a check in
> > http://searchfox.org/mozilla-central/rev/
> > 572e74ee991bbfd812766b4524237eb77577a4b1/toolkit/mozapps/extensions/internal/
> > XPIProvider.jsm#8541 along with a test, but I'll leave it up to you to
> > verify that :)
> 
> That code only seems to apply to the update case. I haven't found the bit
> for built-in add-ons yet.


Yes you're right, we only validate updates. Here's where it happens at startup:
https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/internal/XPIProviderUtils.js#2036

If validation fails we fall back to the built-in set and assume it's good. We could do validation of the built-in set here too, I think given that we test and QA the built-in set it's not as strictly necessary but I think we might as well validate these too.

 
> Am I right in thinking there's only two system add-on tests currently?
> test_system_update.js & test_system_reset.js


That's correct. The built-in set isn't really using any new feature, it's mostly just another install location - all the new features are around the update mechanism.


> Also, we're going to need to regenerate the test xpis and re-sign them. Any
> ideas how this is done, I couldn't see any documentation on it?


I think it was just done manually by uploading to bugs etc. :/
Flags: needinfo?(rhelmer)
(In reply to Robert Helmer [:rhelmer] from comment #7)
> (In reply to Mark Banner (:standard8) from comment #6)
> > That code only seems to apply to the update case. I haven't found the bit
> > for built-in add-ons yet.
> 
> 
> Yes you're right, we only validate updates. Here's where it happens at
> startup:
> https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> internal/XPIProviderUtils.js#2036

That doesn't get called on startup. From what I've understood so far the best place would possibly be:

https://dxr.mozilla.org/mozilla-central/rev/c7d62e6d052c5d2638b08d480a720254ea09ff2d/toolkit/mozapps/extensions/internal/XPIProvider.jsm#2269

However, getting the add-on details would be difficult there.

How about we have a simple test to cover in-tree add-ons, then modify the update code as we discussed before to check for multi-process?
Flags: needinfo?(rhelmer)
(In reply to Mark Banner (:standard8) from comment #8)
> (In reply to Robert Helmer [:rhelmer] from comment #7)
> > (In reply to Mark Banner (:standard8) from comment #6)
> > > That code only seems to apply to the update case. I haven't found the bit
> > > for built-in add-ons yet.
> > 
> > 
> > Yes you're right, we only validate updates. Here's where it happens at
> > startup:
> > https://dxr.mozilla.org/mozilla-central/source/toolkit/mozapps/extensions/
> > internal/XPIProviderUtils.js#2036
> 
> That doesn't get called on startup. From what I've understood so far the
> best place would possibly be:
> 
> https://dxr.mozilla.org/mozilla-central/rev/
> c7d62e6d052c5d2638b08d480a720254ea09ff2d/toolkit/mozapps/extensions/internal/
> XPIProvider.jsm#2269
> 
> However, getting the add-on details would be difficult there.
> 
> How about we have a simple test to cover in-tree add-ons, then modify the
> update code as we discussed before to check for multi-process?

WFM
Flags: needinfo?(rhelmer)
Comment on attachment 8799444 [details]
Bug 1295567 - Ensure system add-ons are always be multiprocessCompatible.

Rob, I've not got the add-ons signed yet, so the tests will fail (and will need a little bit of tweaking), however, can you take a look at this and let me know what you think before I get them signed?
Attachment #8799444 - Flags: feedback?(rhelmer)
Comment on attachment 8799444 [details]
Bug 1295567 - Ensure system add-ons are always be multiprocessCompatible.

This looks like what I'd expect, thanks!
Attachment #8799444 - Flags: feedback?(rhelmer) → feedback+
Comment on attachment 8799444 [details]
Bug 1295567 - Ensure system add-ons are always be multiprocessCompatible.

https://reviewboard.mozilla.org/r/84622/#review84208

Thanks for going through the pain of re-signing the test XPIs!
Attachment #8799444 - Flags: review?(rhelmer) → review+
Pushed by mbanner@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/dbf37ac1be57
Ensure system add-ons are always be multiprocessCompatible. r=rhelmer
https://hg.mozilla.org/mozilla-central/rev/dbf37ac1be57
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Mark, is this something manual QA should be looking at?
Flags: qe-verify?
Flags: needinfo?(standard8)
(In reply to Andrei Vaida, QA [:avaida] – please ni? me from comment #18)
> Mark, is this something manual QA should be looking at?

No QA required. The automated test I added covers this.
Flags: qe-verify?
Flags: qe-verify-
Flags: needinfo?(standard8)
You need to log in before you can comment on or make changes to this bug.