When the telemetry environment changes, the old subsession is submitted with the new environment data. This is incorrect: what we care about is associating the data in that ping with the environment that was active at the time the data was collected. If it's easy to do, it's ok to *also* submit the new environment, so that we can compare what changed. But that is not a requirement.

This patch makes TelemetryPing submit the correct environment data on environment changes. It also adds environment caching.

This patch adjusts TelemetrySession tests to check for the correct environment data on environment changes. It also adds test coverage for cache hits in TelemetryEnvironment.

Removed a wrong newline.

Comment on attachment 8574640 [details] [diff] [review] bug1140558.patch Review of attachment 8574640 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +253,4 @@ > /** > * Initialize TelemetryEnvironment. > */ > + init: Task.async(function* () { TelemetryEnvironment.init() won't get called until 1 minute after browser startup. Does that mean we miss environment changes during the first minute? @@ +263,5 @@ > this._log.trace("init"); > this._shutdown = false; > this._startWatchingPrefs(); > this._startWatchingAddons(); > + this._cachedEnvironment = yield this.getEnvironmentData(); what value of _cachedEnvironment would be reported in environment-changed pings when there are multiple consecutive environment changes? @@ +993,5 @@ > } > > this._log.trace("getEnvironmentData"); > if (this._collectTask) { > return this._collectTask; 1. which environment (or mixture of environments) is reported when there are multiple consecutive environment changes? 2. what if successive calls to getEnvironment data have different values of aNeedPreviousEnvironment? ::: toolkit/components/telemetry/TelemetrySession.jsm @@ +1651,5 @@ > let options = { > retentionDays: RETENTION_DAYS, > addClientId: true, > addEnvironment: true, > + useOldEnvironment: true, we've really stuffed a lot of logic not related to sending pings into TelemetryPing.send(), we'll have to fix this later.. @@ +1656,2 @@ > }; > let promise = TelemetryPing.send(getPingType(payload), payload, options); isn't it possible for the environment to change again, between the time this line is executed and the assemblePing actually reads the oldEnvironment?

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #4) > Comment on attachment 8574640 [details] [diff] [review] > bug1140558.patch > > Review of attachment 8574640 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/telemetry/TelemetryEnvironment.jsm > @@ +253,4 @@ > > /** > > * Initialize TelemetryEnvironment. > > */ > > + init: Task.async(function* () { > > TelemetryEnvironment.init() won't get called until 1 minute after browser > startup. Does that mean we miss environment changes during the first minute? Thanks for reviewing this! Yes, that means we miss environment change notifications during the first minute. But since TelemetrySession is not initialised as well, would it make sense to get changes notification before? > @@ +263,5 @@ > > this._log.trace("init"); > > this._shutdown = false; > > this._startWatchingPrefs(); > > this._startWatchingAddons(); > > + this._cachedEnvironment = yield this.getEnvironmentData(); > > what value of _cachedEnvironment would be reported in environment-changed > pings when there are multiple consecutive environment changes? The data from the environment before the first change happens. > @@ +993,5 @@ > > } > > > > this._log.trace("getEnvironmentData"); > > if (this._collectTask) { > > return this._collectTask; > > 1. which environment (or mixture of environments) is reported when there are > multiple consecutive environment changes? That's a very good point. If a change takes place while TelemetryEnvironment is still collecting, then a mixed environment gets collected. We could guard against this case by forcing cache invalidation in |_doGetEnvironmentData| if multiple calls are detected? > 2. what if successive calls to getEnvironment data have different values of > aNeedPreviousEnvironment? This is addressed in this new patch. > ::: toolkit/components/telemetry/TelemetrySession.jsm > @@ +1651,5 @@ > > let options = { > > retentionDays: RETENTION_DAYS, > > addClientId: true, > > addEnvironment: true, > > + useOldEnvironment: true, > > we've really stuffed a lot of logic not related to sending pings into > TelemetryPing.send(), we'll have to fix this later.. > > @@ +1656,2 @@ > > }; > > let promise = TelemetryPing.send(getPingType(payload), payload, options); > > isn't it possible for the environment to change again, between the time this > line is executed and the assemblePing actually reads the oldEnvironment? In that case, the environment from before the first change is returned by TelemetryEnvironment.

Updates the tests.

Comment on attachment 8575927 [details] [diff] [review] bug1140558.patch - v2 This patch has been changed to address your input from last review (please check comment 7).

I think TelemetryEnvironment is still too complex here. It shouldn't be necessary to have an asynchronous getter for the environment at any point, and that will make downstream code simpler as well. I think the API for TelemetryEnvironment should be as follows: TelemetryEnvironment.getEnvironment() - sync getter can be called at any time. If the environment is not fully initialized, the environment data may not be complete (e.g. the addons manager isn't done yet so we don't have a list of active addons) TelemetryEnvironment.onInitialized() - returns Promise<environment> which is resolved when all the environment data is available TelemetryEnvironment.registerChangeListener(function(environment)) - the change listener is passed the new environment synchronously And the TelemetrySession code should use the environment in this way: At startup, call .getEnvironment to cache the environment for the first subsession. Call .onInitialized and update the first subsession environment once we have the full data. From here, call .registerChangeListener. Any call to the change listener starts a new subsession. I can write the TelemetryEnvironment.jsm changes today, if this sounds like a reasonable plan.

if we can make getEnvironment synchronous, then your proposal is better. which current or future environment getters return Promises? is it only the addonManager?

The search provider will also be async-init. It doesn't look like any of the other data is async.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #12) > The search provider will also be async-init. It doesn't look like any of the > other data is async. if you're ok with some pings missing addonManager & search provider info, then this would work. e.g. pings from short sessions

(In reply to Benjamin Smedberg [:bsmedberg] from comment #10) > And the TelemetrySession code should use the environment in this way: > > At startup, call .getEnvironment to cache the environment for the first > subsession. > Call .onInitialized and update the first subsession environment once we have > the full data. From here, call .registerChangeListener. > Any call to the change listener starts a new subsession. My only concern is with which object is owning the Environment data: TelemetryPing owns the Environment and is responsible for composing the common ping format (which contains the Environment data itself). @vladan: other than the addonManager, |ProfileAge| is also returning promises.

We have two options. If we're missing data, we could either wait for it (block shutdown) or save the ping without it. Given my experience with the addon manager missing callbacks, I suspect that we'll have a more reliable system if we don't try to block on getting the data.

Checkpoint of code that's not working yet.

This implements the environment caching and passes xpcshell tests at least. I'm a little bit worried about this because of performance concerns with getting the addons list. My understanding of the APIs is that they force us to load the addons cache, and this may be a performance problem. There even appears in the log of one of the tests this warning: 0:01.46 PROCESS_OUTPUT: Thread-1 (pid:32292) "1426279053903 addons.xpi-utils WARN Synchronous load of XPI database due to getAddonsByType(theme)" I don't know why this is synchronous, since .getAddonsByType is async. But I'm not the expert about this and so I'm looking for advice on the proper way to get the list of active addons near startup without janking the browser.

Comment on attachment 8577438 [details] [diff] [review] environment-sync Review of attachment 8577438 [details] [diff] [review]: ----------------------------------------------------------------- Looks good, just a couple of comments on the addons part. ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +371,5 @@ > + onDisabled: function() { > + this._onChange(); > + }, > + onInstalled: function() { > + this._onChange(); Unfortunately, we have to be more careful here. Even the addon was installed it doesn't mean it became active. Check addon.isActive too (onInstalled comes with an "addon" argument). Please see bug 1122050 comment 38 for more info. @@ +374,5 @@ > + onInstalled: function() { > + this._onChange(); > + }, > + onUninstalling: function() { > + this._onChange(); Same here, we should be reporting changes only if the addon is active and doesn't require a restart: onUninstalling: function(addon, requiresRestart) { if (!addon.isActive || requiresRestart) return; this._onChange(); } More info in bug 1122050 comment 47! @@ +772,3 @@ > > for (let pref in this._watchedPrefs) { > + Preferences.observe(pref, observerFunc); I think |observerFunc| should be a member of |EnvironmentCache| (like the previous |_onPrefChanged|), otherwise you would not be able to stop watching the prefs in |_stopWatchingPrefs|. @@ +784,2 @@ > for (let pref in this._watchedPrefs) { > Preferences.ignore(pref, this._onPrefChanged, this); There's no |_onPrefChanged| function anymore, so this should be changed to match whatever function |Preferences.observe| is using as a callback. @@ +855,5 @@ > try { > updateChannel = UpdateChannel.get(); > } catch (e) {} > > + this._currentEnvironment.settings = { Since nothing is being returned anymore, the |@return Object| comment part can be removed. @@ +890,1 @@ > } Same here. @@ +1065,5 @@ > this._log.trace("_onEnvironmentChange - Already shut down."); > return; > } > > + Extra blank line here. ::: toolkit/components/telemetry/TelemetrySession.jsm @@ +1647,1 @@ > this._log.trace("_onEnvironmentChange"); Since we have a |reason| now, is it worth logging? ::: toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js @@ +53,5 @@ > + line = "done"; > + } catch(e) { > + do_print("test_firstRun exception: " + e + " at phase: " + line); > + throw(e); > + } Not an error or anything, just curious: is this change intended to be here or that's debug code that made it into the patch?

> ::: toolkit/components/telemetry/TelemetryEnvironment.jsm > @@ +371,5 @@ > > + onDisabled: function() { > > + this._onChange(); > > + }, > > + onInstalled: function() { > > + this._onChange(); > > Unfortunately, we have to be more careful here. > Even the addon was installed it doesn't mean it became active. Check > addon.isActive too (onInstalled comes with an "addon" argument). Please see > bug 1122050 comment 38 for more info. I don't think this is correct. _onChange gets the list of active addons and only fires notifications if the list has changed. So I don't think we need any of the more complex logic that was here originally and I removed it intentionally. > ::: toolkit/components/telemetry/tests/unit/test_TelemetryPingBuildID.js > @@ +53,5 @@ > > + line = "done"; > > + } catch(e) { > > + do_print("test_firstRun exception: " + e + " at phase: " + line); > > + throw(e); > > + } > > Not an error or anything, just curious: is this change intended to be here > or that's debug code that made it into the patch? Yes this is debugging code that I can revert. We were rejecting a promise with `undefined` which ended up horking the stack trace.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #17) > I'm not the expert about this and so I'm looking for advice on the proper > way to get the list of active addons near startup without janking the > browser. AddonManager already calls setAddons during its startup with the list of active extensions (unchanged from classic Telemetry). Would this basic list be enough when the async request for detailed info hasn't completed yet?

Comment on attachment 8577438 [details] [diff] [review] environment-sync Review of attachment 8577438 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +384,1 @@ > return; what happens when multiple addon changes happen while waiting for _pendingTask to complete? Telemetry just skips generating environment-changed pings for all changes except the first one? Isn't there a risk of the returned addon state being stale?

Yes, we do risk that. And we have throttling issues also; I'd like to postpone dealing with that into bug 1143714.

Comment on attachment 8577438 [details] [diff] [review] environment-sync Review of attachment 8577438 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +66,5 @@ > + }, > +}; > + > +const DEFAULT_ENVIRONMENT_PREFS = new Map([ > + ["app.feedback.baseURL", TelemetryEnvironment.RECORD_PREF_VALUE], Can we move adding the prefs to bug 1131138 (or at least close that bug over this one)? @@ +346,5 @@ > > + this._pendingTask = this._updateAddons().then( > + () => { this._pendingTask = null; }, > + (err) => { > + this._environment._log.error("Exception in _updateAddons", err); The logger is prefixed with "TelemetryEnvironment::" so that using a message like "functionName - foo" results in a message like "TelemetryEnvironment::functionName - foo". Can we keep that pattern here? Dito below. @@ +414,5 @@ > + } > + }, > + (err) => { > + this._pendingTask = null; > + this._log.error("Error collecting addons", err); this._environment._log ::: toolkit/components/telemetry/docs/environment.rst @@ +44,3 @@ > }, > }, > profile: { // This section is not available on Android. Let's document that this may be empty for pings that are submitted early. Dito that addons may be missing before first collection? ::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js @@ +720,1 @@ > Nit: can we remove that trailing space while we're here?

(In reply to Benjamin Smedberg [:bsmedberg] from comment #17) > Created attachment 8577438 [details] [diff] [review] > environment-sync > > This implements the environment caching and passes xpcshell tests at least. > > I'm a little bit worried about this because of performance concerns with > getting the addons list. My understanding of the APIs is that they force us > to load the addons cache, and this may be a performance problem. There even > appears in the log of one of the tests this warning: > > 0:01.46 PROCESS_OUTPUT: Thread-1 (pid:32292) "1426279053903 > addons.xpi-utils WARN Synchronous load of XPI database due to > getAddonsByType(theme)" This is complaining in particular that something has changed the active theme before the add-ons database has been loaded, either a lightweight theme was applied or the theme pref changed basically. It shouldn't be the norm and likely only happens during testing. > I don't know why this is synchronous, since .getAddonsByType is async. But > I'm not the expert about this and so I'm looking for advice on the proper > way to get the list of active addons near startup without janking the > browser. Loading the database is meant to be asynchronous so I think it should be ok. Note that if all you care about is the extension IDs that are active then the pref extensions.enabledAddons holds that. It's a bit hacky to use that though.

Updated patch with review comments.

Benjamin pointed out that his patch only caches the environment, but doesn't actually fix the main problem here. We should save the environment when we start a new subsession, not after a change. Alternatively maybe an environment-change event could pass the old environment along.

https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0060c41a3d Definitely want to save it early, not pass/track the old environment.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #28) > Definitely want to save it early, not pass/track the old environment. Why? It actually seems better to have the environment take care of it instead of >= environment clients. Also, there is the issue with addons & profile not being filled out initially, having the environment pass the old data seems simpler.

sorry had to back this out for test failures like https://treeherder.mozilla.org/logviewer.html#?job_id=7845613&repo=mozilla-inbound

This passes the old environment data along to change listeners. To get this into the ping, we allow TelemetryPing clients to pass an environment data override. Note: This includes minor readability & deduplication changes.

Primarily because of the throttling for bug 1143714. But also it's going to be simpler to manage to have an environment with the current subsession, rather than gathering it at the end.

Comment on attachment 8580111 [details] [diff] [review] environment-sync This was already reviewed.

(In reply to Benjamin Smedberg [:bsmedberg] from comment #33) > Primarily because of the throttling for bug 1143714. But also it's going to > be simpler to manage to have an environment with the current subsession, > rather than gathering it at the end. Per IRC, the current behavior in the patch (passing the old environment just before the change to the change listeners) is fine.

Comment on attachment 8580746 [details] [diff] [review] Part 2 - Pass the old environment data to event listeners on environment changes. Review of attachment 8580746 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +391,2 @@ > > + _checkForChanges: function(changeReason) { _checkForAddonChanges? @@ +391,3 @@ > > + _checkForChanges: function(changeReason) { > + if (this._pendingTask) { So we're still gonna have issues with with stale data and missed environment-changed pings? This issue isn't addressed in the batching patch, the batching patch operates from _onEnvironmentChange and this if-check will prevent _onEnvironmentChange from even being called for each change @@ +446,5 @@ > }; > > + let result = { > + changed: (JSON.stringify(addons) != > + JSON.stringify(this._environment._currentEnvironment.addons)), I realize this was not added in this patch, but isn't this comparison affected by differences in the order of subfields?

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #36) > @@ +391,3 @@ > > > > + _checkForChanges: function(changeReason) { > > + if (this._pendingTask) { > > So we're still gonna have issues with with stale data and missed > environment-changed pings? > This issue isn't addressed in the batching patch, the batching patch > operates from _onEnvironmentChange and this if-check will prevent > _onEnvironmentChange from even being called for each change We are throttling environment changes in bug 1143714 anyway, so i am not worried about this part. > @@ +446,5 @@ > > }; > > > > + let result = { > > + changed: (JSON.stringify(addons) != > > + JSON.stringify(this._environment._currentEnvironment.addons)), > > I realize this was not added in this patch, but isn't this comparison > affected by differences in the order of subfields? Yeah, good point.

The Assert.jsm implementation looks solid and tested, moving this over to a shared location.

Updated to use deepEqual instead of JSON string comparisons.

(In reply to Georg Fritzsche [:gfritzsche] from comment #37) > We are throttling environment changes in bug 1143714 anyway, so i am not > worried about this part. In bug 1143714 there's a threshold for dropping environment changes and we log the drops, in this patch, we're dropping the changes almost silently. Add the changeReason to the log message + document in _onEnvironmentChange that throttling also happens further upstream in _checkChanges

On Windows, under some circumstances, gfx RAM was being reported as NaN and not as the expected null. This fixes the problem.

Comment on attachment 8582376 [details] [diff] [review] Part 2 - Move the Assert.deepEqual implementation to a shared module Review of attachment 8582376 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the stuff below ::: toolkit/modules/ObjectUtils.jsm @@ +1,2 @@ > +this.EXPORTED_SYMBOLS = ["ObjectUtils"]; > + Please add the copyright and "use strict". Oh, and have you checked the copyright for this source, btw? Is it our implementation or did we pull it from somewhere? And finally, please add a comment.

Rebased.

Rebased, addressed review comment.

Addressed review comments.

Comment on attachment 8583031 [details] [diff] [review] Part 2 - Move the Assert.deepEqual implementation to a shared module Gerv, this has an open license question, can you help on that? This moves the deepEqual implementation from Assert.jsm [0] to a new ObjectUtils.jsm. That implementation originally comes from Narwhaljs under the MIT license [1]. Are we fine with the top-level comment on ObjectUtils.jsm as done here? [0]: https://hg.mozilla.org/mozilla-central/annotate/cc0950b7a369/testing/modules/Assert.jsm#l260 [1]: https://hg.mozilla.org/mozilla-central/annotate/cc0950b7a369/testing/modules/Assert.jsm#l9

gfritzsche: MIT stuff can be stuck in an MPLed file, as long as you reproduce the MIT license text in the file and say what code it applies/applied to. So how it's done in Assert.jsm seems a little vague (unless the entire file was originally under MIT) but that's the basic idea. Does that help? Gerv

Rebase.

Fix minor test oversight.

Rebase.

(In reply to Gervase Markham [:gerv] from comment #48) > gfritzsche: MIT stuff can be stuck in an MPLed file, as long as you > reproduce the MIT license text in the file and say what code it > applies/applied to. So how it's done in Assert.jsm seems a little vague > (unless the entire file was originally under MIT) but that's the basic idea. So, any suggestions on improving it? Do you think it's better to move the comment right to the main function that was moved over? Or maybe it's better to keep it in a separate file?

Also note that the question is about ObjectUtils.jsm, Assert.jsm is already in-tree with those comments.

If the MIT license reference applies only to a single section or function, stick the reference just above that section/function, and put an "End of previously MIT-licensed code" comment at the end, or something like that. Or at the top, you can say "Portions of this file come from <source> and were licensed under the following license: <terms> Basically, make it clear that the entire file isn't ex-MIT. The more specific the better, but there's no hard legal standard I know of. Gerv

Comment on attachment 8583148 [details] [diff] [review] Part 3 - Pass the old environment data to event listeners on environment changes. Review of attachment 8583148 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryEnvironment.jsm @@ +386,5 @@ > > // nsIObserver > observe: function (aSubject, aTopic, aData) { > this._environment._log.trace("observe - Topic " + aTopic); > + this._checkForChanges("experiment-changed"); Hmm, we could miss a subsession's participation in an en experiment. At least it's not possible for a user to activate 2 experiments at the same time. ::: toolkit/components/telemetry/docs/environment.rst @@ +11,5 @@ > > +Some parts of the environment must be fetched asynchronously at startup. If a session is very short or terminates early, the following items may be missing from the environment: > + > +- profile > +- addons Explain why environment.addons might be incorrect in rare cases

(In reply to Vladan Djeric (:vladan) -- please needinfo! from comment #55) > Comment on attachment 8583148 [details] [diff] [review] > Part 3 - Pass the old environment data to event listeners on environment > changes. > > Review of attachment 8583148 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: toolkit/components/telemetry/TelemetryEnvironment.jsm > @@ +386,5 @@ > > > > // nsIObserver > > observe: function (aSubject, aTopic, aData) { > > this._environment._log.trace("observe - Topic " + aTopic); > > + this._checkForChanges("experiment-changed"); > > Hmm, we could miss a subsession's participation in an en experiment. At > least it's not possible for a user to activate 2 experiments at the same > time. Hm, ok, there is a race potential, but with the current implementation we happen to not yield after this._getActiveExperiment() in _updateAddons(). So, if there is a _pendingTask running in _checkChanges() we should always pick up the current (last active) experiment. Suggestion: * i'll handle the issue i see there with the throttling in bug 1143714 over there * i'll file a follow-up on either removing that race potential or explicitly factoring experiments out so we don't accidentally break that

Clearing ni?gerv per comment 54.

(In reply to Georg Fritzsche [:gfritzsche] from comment #56) > > ::: toolkit/components/telemetry/TelemetryEnvironment.jsm > > @@ +386,5 @@ > > > > > > // nsIObserver > > > observe: function (aSubject, aTopic, aData) { > > > this._environment._log.trace("observe - Topic " + aTopic); > > > + this._checkForChanges("experiment-changed"); > > > > Hmm, we could miss a subsession's participation in an en experiment. At > > least it's not possible for a user to activate 2 experiments at the same > > time. > > Hm, ok, there is a race potential, but with the current implementation we > happen to not yield after this._getActiveExperiment() in _updateAddons(). > So, if there is a _pendingTask running in _checkChanges() we should always > pick up the current (last active) experiment. > > Suggestion: > * i'll handle the issue i see there with the throttling in bug 1143714 over > there Nevermind that part - we always update the environment and just throttle firing the change events, so that part is fine. > * i'll file a follow-up on either removing that race potential or explicitly > factoring experiments out so we don't accidentally break that Filed bug 1147828.

Addressed license concerns per comment 54.

Addressed review comment.

https://hg.mozilla.org/integration/fx-team/rev/d7a94bbfa3ef https://hg.mozilla.org/integration/fx-team/rev/9623fa2b2e16 https://hg.mozilla.org/integration/fx-team/rev/a54f84a3d265 https://hg.mozilla.org/integration/fx-team/rev/d3512bb40d24

Backed out for shutdown crashes: https://hg.mozilla.org/integration/fx-team/rev/832a874a7ccc https://hg.mozilla.org/integration/fx-team/rev/83e259349f52 https://hg.mozilla.org/integration/fx-team/rev/43235a6667fd https://hg.mozilla.org/integration/fx-team/rev/b4ad5033ab88

Being handled in bug 1139460.

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=256df1f0f2f6 fxteam push: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=d2567d89cda3 https://hg.mozilla.org/integration/fx-team/rev/a082c774db0a https://hg.mozilla.org/integration/fx-team/rev/b2e92f548736 https://hg.mozilla.org/integration/fx-team/rev/a4f545b715ae https://hg.mozilla.org/integration/fx-team/rev/18989d1ebd43

This bug seems to have added a new test: toolkit/modules/tests/xpcshell/test_ObjectUtils.js Currently, Windows XP pgo is still building for this particular push: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=d2567d89cda3 But a commit that landed a few commits after has the test failing: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=895cf25d11fd log: https://treeherder.mozilla.org/logviewer.html#?job_id=2520123&repo=fx-team 18:54:22 WARNING - TEST-UNEXPECTED-FAIL | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | xpcshell return code: 0

The test appears to be passing on non-pgo builds. Digging a bit more into the test: it's failing on the very last check for circular refs: 18:54:22 INFO - TEST-PASS | toolkit/modules/tests/xpcshell/test_ObjectUtils.js | test_deepEqual - [test_deepEqual : 81] true == true 18:54:22 INFO - (xpcshell/head.js) | test run_next_test 0 finished (2) 18:54:22 INFO - Unexpected exception InternalError: too much recursion at resource://gre/modules/ObjectUtils.jsm:124 18:54:22 INFO - objEquiv@resource://gre/modules/ObjectUtils.jsm:124:9 18:54:22 INFO - _deepEqual@resource://gre/modules/ObjectUtils.jsm:72:12 https://hg.mozilla.org/integration/fx-team/file/b2e92f548736/toolkit/modules/tests/xpcshell/test_ObjectUtils.js#l80 80 // String literal + object 81 Assert.ok(!deepEqual("a", {})); 82 83 // Make sure deepEqual doesn't loop forever on circular refs 84 85 let b = {}; 86 b.b = b; 87 88 let c = {}; 89 c.b = c; 90 91 Assert.ok(!deepEqual(b, c));

Comment on attachment 8583806 [details] [diff] [review] Part 2 - Move the Assert.deepEqual implementation to a shared module >+++ b/testing/modules/tests/xpcshell/test_assert.js >- // Make sure deepEqual doesn't loop forever on circular refs >- >- let b = {}; >- b.b = b; >- >- let c = {}; >- c.b = c; >- >- let gotError = false; >- try { >- assert.deepEqual(b, c); >- } catch (e) { >- gotError = true; >- } >- >- dump("All OK\n"); >- assert.ok(gotError); The original test makes sure there /is/ an exception.

This and everything else from that push is backed out in https://hg.mozilla.org/integration/fx-team/rev/6901a267f856 for Windows PGO XPCShell failures: https://treeherder.mozilla.org/logviewer.html#?job_id=2520751&repo=fx-team

See bug 1139460.

(In reply to Georg Fritzsche [:gfritzsche] from comment #38) > Hmpf, #3 of "things that never showed up on try". > > PGO try push with fix: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cb8ce2e0436

fx-team push: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=cda9f6c087a6 pgo try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9cb8ce2e0436 previous full try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=256df1f0f2f6 https://hg.mozilla.org/integration/fx-team/rev/f5b390ddd517 https://hg.mozilla.org/integration/fx-team/rev/61809fa9751f https://hg.mozilla.org/integration/fx-team/rev/4520038d8907 https://hg.mozilla.org/integration/fx-team/rev/ac401813266d

https://hg.mozilla.org/mozilla-central/rev/f5b390ddd517 https://hg.mozilla.org/mozilla-central/rev/61809fa9751f https://hg.mozilla.org/mozilla-central/rev/4520038d8907 https://hg.mozilla.org/mozilla-central/rev/ac401813266d

Comment on attachment 8583146 [details] [diff] [review] Part 1 - Cache the environment Approved for Aurora. For approval request see bug 1139460#c42. For approval comments see bug 1139460#c43.

https://hg.mozilla.org/releases/mozilla-aurora/rev/79582fe6d87e https://hg.mozilla.org/releases/mozilla-aurora/rev/aab52686db29 https://hg.mozilla.org/releases/mozilla-aurora/rev/e289c20fd8ee https://hg.mozilla.org/releases/mozilla-aurora/rev/9b2cd00d2ddc