Closed Bug 1434076 Opened 5 years ago Closed 5 years ago

browserAction breaks in extensions that use bundled experiments

Categories

(WebExtensions :: Experiments, defect, P1)

Product:
WebExtensions
Component:
Experiments
Version:
56 Branch
Type:
defect

Tracking

(firefox59 fixed, firefox60 fixed)

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: aswan, Assigned: aswan)

Details

Attachments

(1 file)

Bug 1434076 Correct calls to getAPI() on wrong api manager
59 bytes, text/x-review-board-request
kmag
: review+
RyanVM
: approval-mozilla-release+
Details 
Rob and Donovan both discovered this independently, including an embedded experiment in a WebExtension breaks browserAction (and probably other things too, this is just the first one we've come across).

It looks like some code was overlooked in the conversion to per-extension api managers in bug 1323845, such as:
https://searchfox.org/mozilla-central/rev/97cb0aa64ae51adcabff76fb3b5eb18368f5f8ab/toolkit/components/extensions/Extension.jsm#1418

I blame the review of that bug for not going over the 1000+ new lines of code carefully enough.
We've got a couple of Q1 OKRs depending on this:  Implementing Test Pilot and Tab Split as web extensions.  I don't know how big of a change this fix is, but when there is a patch, we'd like to get it uplifted as far as we can.  Thanks!
bundled experiments only landed in 59 so that's as far as a fix for this could go.  This is on my radar, hoping to do it this week or early next week.
Assignee: nobody → aswan
Priority: -- → P2
Attachment #8947919 - Flags: review?(kmaglione+bmo)
I don't love this solution, but I do like it better than the alternative of having to be careful to always call getAPI() on the right manager instance, that feels fragile and prone to break again in the future.  Which begs the question of how this should be tested, since there are various paths by which apis can be instantiated I don't see a good way to have a reliable test short of having our existing tests run both with and without bundled experiments in the test extensions.  And that doesn't seem at all practical...
Comment on attachment 8947919 [details]
Bug 1434076 Correct calls to getAPI() on wrong api manager

https://reviewboard.mozilla.org/r/217598/#review223374


Static analysis found 1 defect in this patch.
 - 1 defect found by mozlint

You can run this analysis locally with:
 - `./mach lint check path/to/file` (Python/Javascript/wpt)


If you see a problem in this automated review, please report it here: http://bit.ly/2y9N9Vx


::: toolkit/components/extensions/ExtensionCommon.jsm:983
(Diff revision 1)
>     *     "devtools" - A devtools process.
>     *     "proxy" - A proxy script process.
>     * @param {SchemaRoot} schema
> +   * @param {WeakMap} apiMap
>     */
> -  constructor(processType, schema) {
> +  constructor(processType, schema, apiMap=null) {

Error: Infix operators must be spaced. [eslint: space-infix-ops]
I think ddurst asked me to up this to P1 if there was a reason to land in 60 (Notes and Tab Split would both like to use this), so I'm marking P1.  Let me know if I misunderstood...
Priority: P2 → P1
Attachment #8947919 - Flags: review?(kmaglione+bmo) → review?(tomica)
Comment on attachment 8947919 [details]
Bug 1434076 Correct calls to getAPI() on wrong api manager

Clearing this since Kris has opinions about the current approach, and I'm still not up on all the changes from bug 1323845.
Attachment #8947919 - Flags: review?(tomica)
Attachment #8947919 - Flags: review?(tomica) → review?(kmaglione+bmo)
Okay, here's take 2.  I used the Cu.reportError() bit while I was testing and I left it here since I think its cheap and helpful, but I'm also fine ripping it out if you prefer.  I remain stumped about how to test this systematically...
Comment on attachment 8947919 [details]
Bug 1434076 Correct calls to getAPI() on wrong api manager

https://reviewboard.mozilla.org/r/217598/#review223474

I'm leery of this solution. It might solve the immediate problem, but I don't think it's the right fix, and wallpapering over it will probably lead to other problems.

We need to make sure we're always using the correct API manager when we create APIs for extensions. In this case,
Attachment #8947919 - Flags: review?(kmaglione+bmo) → review+
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #11)
> Comment on attachment 8947919 [details]
> Bug 1434076 Correct calls to getAPI() on wrong api manager
> 
> https://reviewboard.mozilla.org/r/217598/#review223474
> 
> I'm leery of this solution. It might solve the immediate problem, but I
> don't think it's the right fix, and wallpapering over it will probably lead
> to other problems.
> 
> We need to make sure we're always using the correct API manager when we
> create APIs for extensions. In this case,

Ignore this. Reviewboard botch. Apparently it failed to submit my initial r- weeks ago and saved the comment... :/
Pushed by aswan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/988b4e245049
Correct calls to getAPI() on wrong api manager r=kmag
https://hg.mozilla.org/mozilla-central/rev/988b4e245049
Status: NEW → RESOLVED
Closed: 5 years ago
status-firefox60: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(aswan)
This isn't covered by automated tests but enough people are using this that I think we're good
Flags: needinfo?(aswan) → qe-verify-
Comment on attachment 8947919 [details]
Bug 1434076 Correct calls to getAPI() on wrong api manager

Approval Request Comment
[Feature/Bug causing the regression]:
This isn't a regression, the bug was always present in this feature which first landed in 59

[User impact if declined]:
This bug doesn't impact users so much as groups within Mozilla like Shield and Test Pilot who want to move to WebExtensions.

[Is this code covered by automated tests?]:
This specific bug is not, but webextensions have a pretty good automated test suite so it is unlikely that this patch introduces other regressions.

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

[Needs manual test from QE? If yes, steps to reproduce]: 
I don't believe additional manual testing is required

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

[Is the change risky?]:
not very

[Why is the change risky/not risky?]:
The change only affects WebExtensions that use bundled experiments.  There is no functional change for regular WebExtensions, and extensions with bundled experiments were already broken enough that it can't really make them any less usable.

[String changes made/needed]:
none
Attachment #8947919 - Flags: approval-mozilla-beta?
Comment on attachment 8947919 [details]
Bug 1434076 Correct calls to getAPI() on wrong api manager

Not going to lie, the lack of test coverage for this issue (both automated and manual) seems sub-optimal for a patch being uplifted directly into the RC build. If our SHIELD and Test Pilot teams both found this issue, is there any possibility they can confirm that it's fixed now?

That said, I don't want to see us ship the new functionality in Fx59 with this bug, so I'm approving the patch for the 59rc1 build on Monday. But I'd *really* like if we could get some sort of confirmation that everything is good now.
Flags: needinfo?(aswan)
Attachment #8947919 - Flags: approval-mozilla-beta? → approval-mozilla-release+
https://hg.mozilla.org/integration/mozilla-inbound/rev/b0eb708c533b05df8c2cfc1b97fc0199e3e84394
Bug 1434076: Follow-up: Add test for conflict between browserAction and bundled experiment. r=zombie a=test-only
(In reply to Ryan VanderMeulen [:RyanVM] from comment #19)
> Not going to lie, the lack of test coverage for this issue (both automated
> and manual) seems sub-optimal for a patch being uplifted directly into the
> RC build.

Sorry about that. I should have asked for tests. Fixed.

https://hg.mozilla.org/releases/mozilla-release/rev/2ce595cf4ff7651a6fa53c5dab4406020eb29773
https://hg.mozilla.org/releases/mozilla-beta/rev/53f9d1e4e584a2414366b34f7dd397b38d4163a4
Flags: qe-verify- → in-testsuite+
Thanks, Kris!
Flags: needinfo?(aswan)
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.