1031326 - Exception thrown and uncaught in a Task: TypeError: data.set is not a function

Status: RESOLVED FIXED

(Firefox Health Report Graveyard :: Client: Desktop, defect)

Description

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

Came back to a Nightly that I had left running in a terminal window, and saw this:

*************************
A coding exception was thrown and uncaught in a Task.

Full message: TypeError: data.set is not a function
Full stack: Experiments.ExperimentEntry.prototype.initFromCacheData@resource://app/modules/experiments/Experiments.jsm:1456:9
Experiments.Experiments.prototype._populateFromCache@resource://app/modules/experiments/Experiments.jsm:990:12
Experiments.Experiments.prototype._loadFromCache<@resource://app/modules/experiments/Experiments.jsm:971:7
TaskImpl_run@resource://gre/modules/Task.jsm:314:40
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7
exports.Utils.yield@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/ireid/Library/Application%20Support/Firefox/Profiles/sngia9xd.debug/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js -> jar:file:///Users/ireid/Library/Application%20Support/Firefox/Profiles/sngia9xd.debug/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/lib/utils.js:380:12
INIParser.prototype.process@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/ireid/Library/Application%20Support/Firefox/Profiles/sngia9xd.debug/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js -> jar:file:///Users/ireid/Library/Application%20Support/Firefox/Profiles/sngia9xd.debug/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/lib/filterStorage.js:892:7
exports.IO.readFromFile/onProgress@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/ireid/Library/Application%20Support/Firefox/Profiles/sngia9xd.debug/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js -> jar:file:///Users/ireid/Library/Application%20Support/Firefox/Profiles/sngia9xd.debug/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/lib/io.js:104:15
exports.IO.readFromFile/<@resource://gre/modules/addons/XPIProvider.jsm -> jar:file:///Users/ireid/Library/Application%20Support/Firefox/Profiles/sngia9xd.debug/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/bootstrap.js -> jar:file:///Users/ireid/Library/Application%20Support/Firefox/Profiles/sngia9xd.debug/extensions/%7Bd10d0bf8-f5b5-c8b4-a8b2-2b9879e08c5d%7D.xpi!/lib/io.js:213:13
TaskImpl_run@resource://gre/modules/Task.jsm:330:41
Handler.prototype.process@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:866:23
this.PromiseWalker.walkerLoop@resource://gre/modules/Promise.jsm -> resource://gre/modules/Promise-backend.js:745:7

Comment 1

[Benjamin Smedberg]

Crap. I thought it was a Map() but it's just an object. Upgrade code sucks.

Comment 2

[Benjamin Smedberg]

Attachment: experiments-not-map

Comment 3

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8447187 [details] [diff] [review]
experiments-not-map

Review of attachment 8447187 [details] [diff] [review]:
-----------------------------------------------------------------

Can we please add basic test-coverage for this?

Comment 4

[Benjamin Smedberg]

Attachment: experiments-not-map, rev. 2

How does this look? It's very unit-testy, but in this case I think that's better than trying to write a custom manifest and then read it.

Comment 5

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8447229 [details] [diff] [review]
experiments-not-map, rev. 2

Review of attachment 8447229 [details] [diff] [review]:
-----------------------------------------------------------------

I think we have enough coverage of the cache writing/reading path, so directly checking this seems good to me.

::: browser/experiments/test/xpcshell/test_upgrade.js
@@ +4,5 @@
> +"use strict";
> +Cu.import("resource:///modules/experiments/Experiments.jsm");
> +
> +const SEC_IN_ONE_DAY  = 24 * 60 * 60;
> +const MS_IN_ONE_DAY   = SEC_IN_ONE_DAY * 1000;

The constants are unused.

@@ +6,5 @@
> +
> +const SEC_IN_ONE_DAY  = 24 * 60 * 60;
> +const MS_IN_ONE_DAY   = SEC_IN_ONE_DAY * 1000;
> +
> +let cd = {

|let cacheData = ...|?

Comment 6

[Benjamin Smedberg]

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

Comment 7

[Benjamin Smedberg]

Comment on attachment 8447229 [details] [diff] [review]
experiments-not-map, rev. 2

Approval Request Comment
[Feature/regressing bug #]: regressed with bug 1017806
[User impact if declined]: Users who used Firefox prior to bug 1017806 landing will not be able to run any experiments until this bug is fixed.
[Describe test coverage new/current, TBPL]: basic automated tests
[Risks and why]: all risk localized to the experiment system
[String/UUID change made/needed]: none

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/529a9b954b72

Comment 10

[:Felipe Gomes (needinfo for replies!)]

I wonder if we could get an expedited approval for this patch? We are about to release two experiments this week (translation on aurora and search on beta), and experiments don't work on builds affected by this bug.

Comment 11

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> [User impact if declined]: Users who used Firefox prior to bug 1017806
> landing will not be able to run any experiments until this bug is fixed.

Does this mean any user on a build older than June 26 (when bug 1017806 landed on Aurora and Beta) cannot participate in the experiment? Any user who was on a build from before bug 1017806 landed and updated to a build with bug 1017806 can not participate in experiments?

Comment 12

[Lawrence Mandel [:lmandel] (use needinfo)]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #7)
> [Describe test coverage new/current, TBPL]: basic automated tests

Given that this just landed on m-c, have you verified that experiments function as expected with this fix? Are you confident that further last minute (for the two experiments that Felipe listed) changes won't be required?

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Felipe, beta 6 will gtb tomorrow. Could you answer to Lawrence's questions?
Thanks

Comment 14

[:Felipe Gomes (needinfo for replies!)]

I had replied on an e-mail, pasting here:

> 1. Does this mean any user on a build older than June 26 (when bug 1017806
> landed on Aurora and Beta) cannot participate in the experiment? Any user
> who was on a build from before bug 1017806 landed and updated to a build
> with bug 1017806 can not participate in experiments?

Users could get the experiment before June 26 (i.e, the translation experiment doesn't require bug 1017806). But if, after having receiving the experiment, they update to a build after June 26, the experiment system won't initialize properly and the translation experiment will be broken until they update again to a build with a fix for bug 1031326.

Users who had not yet received the experiment won't receive them until they are on a build with 1031326 fixed.

>
> Felipe asked for some uplift approvals last week in order to have adequate
> time for people to update before the experiment began. Does bug 1031326 also
> require that people update to the latest build in order for the experiment
> to function correctly? If so, do we need to push out the start date for the
> experiment in order to get enough uptake of suitable Aurora and beta builds?

Very likely. But we do not need to push the start date for deploying it, as the system supports setting a "minimum build date" in which the experiment should be activated. Previously I had set it to the build from 2014-06-21 which is the date when the approvals I requested last week landed. But we will probably need to bump it to 2014-06-30 (assuming I push it today) and then as users gradually update to this build they will start receiving the experiment.


>
> 2. Given that this just landed on m-c, have you verified that experiments
> function as expected with this fix? Are you confident that further last
> minute (for the two experiments that Felipe listed) changes won't be
> required?
>
> I don't think our typical process of bake time on Nightly is going to buy us
> anything if experiments are not enabled. Has the experiment been tested by
> yourselves or QA to ensure that it functions as expected? Can we shake out
> any other bugs before this hits our trial audience?

Right, there's not much point in letting it bake on Nightly untested. I have verified that the translation experiment works properly with the fix. I haven't checked the search experiment yet but I have no reason to believe it would be any different.

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8447229 [details] [diff] [review]
experiments-not-map, rev. 2

While later in beta than I would usually like to see a non critical change, given the nature of the change (one line and contained to experiments) and the desire to kick off an experiment in this cycle, I'm going to plus this one. As we need to lock down beta, if there are further changes required, we should consider whether we can push the search experiment to the next cycle.

Aurora+ as well. I look forward to seeing the results of these experiments.

Comment 16

[Lawrence Mandel [:lmandel] (use needinfo)]

Benjamin, Felipe - Can one of you please handle the landing to ensure this gets in a build ASAP?

Comment 17

[:Felipe Gomes (needinfo for replies!)]

Done! Thanks for the approvals.

https://hg.mozilla.org/releases/mozilla-aurora/rev/44d31566a3a6
https://hg.mozilla.org/releases/mozilla-beta/rev/c06faa2cf62b