Closed Bug 1140558 Opened 5 years ago Closed 5 years ago

Telemetry subsessions should be submitted with their own environment, not the new environment

Categories

(Toolkit :: Telemetry, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox38 --- unaffected
firefox39 --- fixed
firefox40 --- fixed

People

(Reporter: benjamin, Assigned: gfritzsche)

References

(Blocks 1 open bug)

Details

Attachments

(4 files, 16 obsolete files)

1.37 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
66.09 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
14.44 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
23.87 KB, patch
gfritzsche
: review+
Details | Diff | Splinter Review
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.
Attached patch bug1140558.patch (obsolete) — Splinter Review
This patch makes TelemetryPing submit the correct environment data on environment changes.

It also adds environment caching.
Attached patch bug1140558_tests.patch (obsolete) — Splinter Review
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.
Attachment #8574640 - Flags: review?(vdjeric)
Status: NEW → ASSIGNED
Attached patch bug1140558_tests.patch (obsolete) — Splinter Review
Removed a wrong newline.
Attachment #8574642 - Attachment is obsolete: true
Attachment #8574643 - Flags: review?(vdjeric)
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?
Attachment #8574640 - Flags: review?(vdjeric) → review-
Attachment #8574643 - Flags: review?(vdjeric)
Duplicate of this bug: 1140288
Attached patch bug1140558.patch - v2 (obsolete) — Splinter Review
Attachment #8574640 - Attachment is obsolete: true
(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.
Attached patch bug1140558_tests.patch - v2 (obsolete) — Splinter Review
Updates the tests.
Attachment #8575927 - Attachment is obsolete: true
Attachment #8575927 - Attachment is obsolete: false
Attachment #8574643 - Attachment is obsolete: true
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).
Attachment #8575927 - Flags: review?(vdjeric)
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.
Attached patch environment-sync (obsolete) — Splinter Review
Checkpoint of code that's not working yet.
Attached patch environment-sync (obsolete) — Splinter Review
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.
Attachment #8577249 - Attachment is obsolete: true
Attachment #8577438 - Flags: review?(vdjeric)
Attachment #8577438 - Flags: review?(alessio.placitelli)
Attachment #8577438 - Flags: feedback?(dtownsend)
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?
Attachment #8577438 - Flags: review?(alessio.placitelli) → review+
Blocks: 1140132
Attachment #8575927 - Attachment is obsolete: true
Attachment #8575927 - Flags: review?(vdjeric)
Attachment #8575933 - Attachment is obsolete: true
> ::: 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?
Blocks: 1143714
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.
Attachment #8577438 - Flags: feedback?(dtownsend) → feedback+
Attachment #8577438 - Flags: review?(vdjeric)
Assignee: alessio.placitelli → benjamin
Attached patch environment-sync (obsolete) — Splinter Review
Updated patch with review comments.
Attachment #8577438 - Attachment is obsolete: true
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.
Assignee: benjamin → gfritzsche
https://hg.mozilla.org/integration/mozilla-inbound/rev/bd0060c41a3d

Definitely want to save it early, not pass/track the old environment.
Keywords: checkin-needed
(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.
OS: Linux → All
Hardware: x86_64 → All
Version: unspecified → Trunk
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.
Attachment #8580746 - Flags: review?(vdjeric)
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.
Attachment #8580111 - Flags: review+
(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?
Attachment #8580746 - Flags: review?(vdjeric)
(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.
Attachment #8582376 - Flags: review?(dteller)
Updated to use deepEqual instead of JSON string comparisons.
Attachment #8580746 - Attachment is obsolete: true
Attachment #8582385 - Flags: review?(vdjeric)
(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.
Attachment #8583015 - Flags: review?(gfritzsche)
Attachment #8583015 - Flags: review?(gfritzsche) → review+
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.
Attachment #8582376 - Flags: review?(dteller) → review+
Attached patch Part 1 - Cache the environment (obsolete) — Splinter Review
Rebased.
Attachment #8580111 - Attachment is obsolete: true
Attachment #8583027 - Flags: review+
Rebased, addressed review comment.
Attachment #8582385 - Attachment is obsolete: true
Attachment #8582385 - Flags: review?(vdjeric)
Attachment #8583028 - Flags: review?(vdjeric)
Addressed review comments.
Attachment #8582376 - Attachment is obsolete: true
Attachment #8583031 - Flags: review+
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
Attachment #8583031 - Flags: feedback?(gerv)
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.
Attachment #8583027 - Attachment is obsolete: true
Attachment #8583146 - Flags: review+
Fix minor test oversight.
Attachment #8583031 - Attachment is obsolete: true
Attachment #8583031 - Flags: feedback?(gerv)
Attachment #8583147 - Flags: review+
Rebase.
Attachment #8583028 - Attachment is obsolete: true
Attachment #8583028 - Flags: review?(vdjeric)
Attachment #8583148 - Flags: review?(vdjeric)
(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?
Flags: needinfo?(gerv)
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
Attachment #8583148 - Flags: review?(vdjeric) → review+
(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.
Blocks: 1147828
(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.
Attachment #8583147 - Attachment is obsolete: true
Attachment #8583806 - Flags: review+
Flags: needinfo?(gerv)
Addressed review comment.
Attachment #8583148 - Attachment is obsolete: true
Attachment #8583807 - Flags: review+
Blocks: 1147901
Being handled in bug 1139460.
Flags: needinfo?(gfritzsche)
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.
See bug 1139460.
Flags: needinfo?(gfritzsche)
(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
Depends on: 1157644
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.
Attachment #8583146 - Flags: approval-mozilla-aurora+
Attachment #8583806 - Flags: approval-mozilla-aurora+
Attachment #8583807 - Flags: approval-mozilla-aurora+
Attachment #8583015 - Flags: approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.