975000 - Telemetry experiments: ensure that experiment addons are compatible, don't update automatically

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Benjamin Smedberg]

Any updates/uninstalls of experiment XPIs should be managed by the experiment system and should not go through normal update.rdf checking.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Hm, why? Seems if you bypass that you'll need to re-invent the wheel - and the existing wheel is already well-tested and solves all the various security issues (and everything else) involved.

Comment 2

[Benjamin Smedberg]

The experiment system needs to be in charge of the active experiment, and has the condition-based uninstall or upgrade system.

Comment 3

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

Ben, 

Suggested Fix:  Social Agreement by Experiment Writers.

In V.0, maybe we can make a SOCIAL AGREEMENT, that experiment writers won't update via mechanism.  Using Jetpack (as most experiments have been) actually requires extra work to get updates going!

Comment 4

[Blair McBride [:Unfocused] (UNAVAILABLE)]

It'd be pretty easy to enforce this. And, in fact, we'll probably want something regardless - because if a add-on doesn't have an update URL, we ping AMO instead. (Think if someone uploaded an add-on with the same ID.)

Just needs a few special cases for the experiment add-on type in XPIProvider.jsm:
* loadManifestFromRDF() - disallow usage of updateURL (possibly a couple of other properties while we're at it) - http://hg.mozilla.org/mozilla-central/file/78646f067fd3/toolkit/mozapps/extensions/internal/XPIProvider.jsm#l676
* AddonWrapper.permissions - don't add PERM_CAN_UPGRADE - http://hg.mozilla.org/mozilla-central/file/78646f067fd3/toolkit/mozapps/extensions/internal/XPIProvider.jsm#l6446
* AddonWrapper.findUpdates() - immediately return no updates, copy implementation from http://hg.mozilla.org/mozilla-central/file/78646f067fd3/toolkit/mozapps/extensions/LightweightThemeManager.jsm#l511  - http://hg.mozilla.org/mozilla-central/file/78646f067fd3/toolkit/mozapps/extensions/internal/XPIProvider.jsm#l6549

Comment 5

[Gregory Szorc [:gps]]

Creeping scope to include compatibility checking changes so I don't have to file a new bug.

Taking bug.

Comment 6

[Gregory Szorc [:gps]]

Attachment: Change behavior of experiments in add-ons manager

See RB for review questions.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 8394363 [details]
Change behavior of experiments in add-ons manager

New patch on RB.

Comment 8

[Gregory Szorc [:gps]]

I've landed the blockers on this patch and this one is next in line to land.

However, there appear to be failures. https://tbpl.mozilla.org/?tree=Try&rev=2ab9b6510de8

Will look into these tomorrow. I guess I'll prioritize this over bug 989137 since it comes first in the sequence and changes in behavior from this and bug 985084 are necessary to test bug 989137.

Comment 9

[Georg Fritzsche [:gfritzsche]]

(In reply to Gregory Szorc [:gps] from comment #8)
> However, there appear to be failures.
> https://tbpl.mozilla.org/?tree=Try&rev=2ab9b6510de8

Going from e.g. this log:
[1] https://tbpl.mozilla.org/php/getParsedLog.php?id=36838759&tree=Try&full=1#error0

... for browser_bug557956.js, we open the compability window and it shows a page:
> 16:37:40     INFO -  TEST-INFO | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug557956.js | Compatibility dialog opened
> 16:37:40     INFO -  TEST-INFO | chrome://mochitests/content/browser/toolkit/mozapps/extensions/test/browser/browser_bug557956.js | Page versioninfo shown

... a little bit later there an exception in a safeCall() callback:

> 16:37:40     INFO -  1395963460586	addons.manager	WARN	Exception calling callback

... and we apparently never get the "pageshow" event for the "mismatch" page here:
http://hg.mozilla.org/mozilla-central/annotate/6fa163ff81a3/toolkit/mozapps/extensions/test/browser/browser_bug557956.js#l193

I'm running these for checking for stale addons and actually logging details for the callback exception:
https://tbpl.mozilla.org/?tree=Try&rev=89387ff3261e
https://tbpl.mozilla.org/?tree=Try&rev=008d0957c67d

Comment 10

[Georg Fritzsche [:gfritzsche]]

So, we have a callstack (scroll up a little):
https://tbpl.mozilla.org/php/getParsedLog.php?id=36886980&tree=Try&full=1#error1

Apparently the issue is using safeCall here:
https://hg.mozilla.org/try/rev/9d894be72598#l1.23
... as it always passes null as thisArg when .apply()ing.

Comment 11

[:Irving Reid (No longer working on Firefox)]

I really need to get bug 966674 fixed to get those error details back into the logs.

Anyhow, yes, you always need bind() your methods to 'this' before you pass them to safeCall() (or add an object-aware version of safeCall())

Comment 12

[Georg Fritzsche [:gfritzsche]]

(In reply to :Irving Reid from comment #11)
> I really need to get bug 966674 fixed to get those error details back into
> the logs.

That would be great, i already ran into this in two different bugs in the last two days.

Try push with bound listeners:
https://tbpl.mozilla.org/?tree=Try&rev=5b4ae19ade4e

Comment 13

[Gregory Szorc [:gps]]

Georg: It looks like your approach of using .bind() is sane. But nowhere else in Addons Manager code does safeCall() do a .bind(): the .bind() needs to be performed on the caller side. I'll make this small change, verify I don't locally get the leaked property window locally, then push this.

Comment 14

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/5fb093da5090

Comment 15

[Gregory Szorc [:gps]]

https://hg.mozilla.org/integration/fx-team/rev/dca962be6487

Backed out because tests still failed.

And I thought we had a green Try run and I pushed an equivalent patch to that. Oh well :/

Comment 16

[Georg Fritzsche [:gfritzsche]]

The failing push: https://tbpl.mozilla.org/?tree=Fx-Team&rev=4834a3833639

I don't think that patch is equivalent to my try push.
You changed how the "onNo*" callbacks are called on the listeners and it's now passing |null| instead of |listener| as |this| to them. Why not just keep the previous behavior here via |.bind(listener)|?

Comment 17

[Georg Fritzsche [:gfritzsche]]

(In reply to Gregory Szorc [:gps] from comment #13)
> I'll make this small change, verify I
> don't locally get the leaked property window locally, then push this.

As mentioned in IRC, that was just me (missing let in one of the debugging patches).

Comment 18

[Georg Fritzsche [:gfritzsche]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> I don't think that patch is equivalent to my try push.
> You changed how the "onNo*" callbacks are called on the listeners and it's
> now passing |null| instead of |listener| as |this| to them. Why not just
> keep the previous behavior here via |.bind(listener)|?

To add context: dxr says that at least social code is using these (and who knows which addons might?). Changing that behavior seems at least out of scope for this bug.

Comment 19

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5fb093da5090

Comment 20

[Gregory Szorc [:gps]]

I guess mcMerge didn't like my backout commit.

Comment 21

[Gregory Szorc [:gps]]

(In reply to Georg Fritzsche [:gfritzsche] from comment #16)
> The failing push: https://tbpl.mozilla.org/?tree=Fx-Team&rev=4834a3833639
> 
> I don't think that patch is equivalent to my try push.
> You changed how the "onNo*" callbacks are called on the listeners and it's
> now passing |null| instead of |listener| as |this| to them. Why not just
> keep the previous behavior here via |.bind(listener)|?

The previous behavior never included a .bind(): that was something you added. Nowhere else in Addon Manager code AFAICT does a listener get .bind() treatment. I'll go back to ReviewBoard for further comments.

Comment 22

[Gregory Szorc [:gps]]

Alright. Looking at this with a fresh mind, the .bind() from Georg's patch is necessary to maintain backwards compatibility.

I've relanded with Georg's changes factored in. This didn't technically get r+ from Blair. But I'm a Toolkit peer and this is all dealing with internal APIs on the AddonManager. I don't see any harm or breach of process.

https://hg.mozilla.org/integration/fx-team/rev/ac486fb972ca

Comment 23

[Phil Ringnalda (:philor)]

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