1434076 - browserAction breaks in extensions that use bundled experiments

Status: RESOLVED FIXED

(WebExtensions :: Experiments, defect, P1)

Description

[Andrew Swan [:aswan]]

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.

Comment 1

[Wil Clouser [:clouserw]]

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!

Comment 2

[Andrew Swan [:aswan]]

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.

Comment 3

[Andrew Swan [:aswan]]

Attachment: Bug 1434076 Correct calls to getAPI() on wrong api manager

Review commit: https://reviewboard.mozilla.org/r/217598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/217598/

Comment 4

[Andrew Swan [:aswan]]

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 5

[Code Review Bot [:reviewbot]]

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]

Comment 6

[Wil Clouser [:clouserw]]

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...

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8947919 [details]
Bug 1434076 Correct calls to getAPI() on wrong api manager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217598/diff/1-2/

Comment 8

[Tomislav Jovanovic :zombie]

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.

Comment 9

[Andrew Swan [:aswan]]

Comment on attachment 8947919 [details]
Bug 1434076 Correct calls to getAPI() on wrong api manager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217598/diff/2-3/

Comment 10

[Andrew Swan [:aswan]]

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 11

[Kris Maglione [:kmag]]

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,

Comment 12

[Kris Maglione [:kmag]]

(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... :/

Comment 13

[Andrew Swan [:aswan]]

Comment on attachment 8947919 [details]
Bug 1434076 Correct calls to getAPI() on wrong api manager

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/217598/diff/3-4/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/988b4e245049
Correct calls to getAPI() on wrong api manager r=kmag

Comment 15

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/988b4e245049

Comment 16

[marius.santa]

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.

Comment 17

[Andrew Swan [:aswan]]

This isn't covered by automated tests but enough people are using this that I think we're good

Comment 18

[Andrew Swan [:aswan]]

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

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/1f84de3779e2 (FIREFOX_59b_RELBRANCH)
https://hg.mozilla.org/releases/mozilla-release/rev/e43c383fe7da

Comment 21

[Kris Maglione [:kmag]]

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

Comment 22

[Kris Maglione [:kmag]]

(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

Comment 23

[Ryan VanderMeulen [:RyanVM]]

Thanks, Kris!

Comment 24

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/b0eb708c533b