Closed Bug 1360354 Opened 2 years ago Closed 2 years ago

[e10s] Don't use aAddon.bootstrap as a check for "is it a webextension"

Categories

(Firefox :: General, defect)

54 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 55
Tracking Status
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: Felipe, Assigned: Felipe)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

From bug 1352388 comment comment 40, the check we used in bug 1346288 was `aAddon.bootstrap`, which seemed sensible (as it blocks bootstrapped addons and SDK addons), but there's a category of Much-Older addons (the ones that requires restart) that would also return `false` here, which would signal that they can allow the e10s-multi to run.

This change will ensure that only webextensions are allowed.
Hmm as I was running the test, I see that there's a test that explictly tests that non-restartless addons still allow multi [1]. Is that on purpose?

[1] http://searchfox.org/mozilla-central/rev/068e6f292503df13515a52321fc3844e237bf6a9/toolkit/mozapps/extensions/test/xpcshell/test_e10s_restartless.js#478
Flags: needinfo?(gkrizsanits)
The requirement was to disable multi if there is an SDK based add-on is present. Filtering out the bootstrapped add-ons was the only way we could safely do that but it came with the cost of filtering out some non WebExtension based add-ons, which is a shame.

I see no reason to disable multi for non-bootrsrapped add-ons, except maybe consistency and a better story to explain. But if it makes your life a whole lot easier during the beta experiment I'm not against it. Let's bring this up during the cross functional meeting today.
Flags: needinfo?(gkrizsanits)
Attachment #8862921 - Flags: review?(gkrizsanits)
Comment on attachment 8862921 [details]
Bug 1360354 - Do not qualify users of legacy addons (non-bootstrapped) for e10s-multi.

https://reviewboard.mozilla.org/r/134798/#review137734
Attachment #8862921 - Flags: review?(gkrizsanits) → review+
Pushed by felipc@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/431d552f54e6
Do not qualify users of legacy addons (non-bootstrapped) for e10s-multi. r=krizsa
https://hg.mozilla.org/mozilla-central/rev/431d552f54e6
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Comment on attachment 8862921 [details]
Bug 1360354 - Do not qualify users of legacy addons (non-bootstrapped) for e10s-multi.

Approval Request Comment
[Feature/Bug causing the regression]: adjusting the plans for the e10s-multi beta experiment to only include users of webextensions
[User impact if declined]: users of old legacy addons will remain qualified for e10s-multi
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]:  no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple change covered by tests
[String changes made/needed]: none
Attachment #8862921 - Flags: approval-mozilla-beta?
Comment on attachment 8862921 [details]
Bug 1360354 - Do not qualify users of legacy addons (non-bootstrapped) for e10s-multi.

Avoid old legacy addons remaining qualified for e10s-multi and include test. Beta54+. Should be in 54 beta 5.
Attachment #8862921 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(In reply to :Felipe Gomes (needinfo me!) from comment #8)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: yes
> [Needs manual test from QE? If yes, steps to reproduce]:  no

Setting qe-verify- based on Felipe's assessment on manual testing needs and the fact that this fix has automated coverage.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.