Right now Telemetry relies on profile-after-change/profile-before-change events to start/stop, but that doesn't work in content processes because they don't have profiles. We should switch to other events instead.

We should use DeferredTask instead of creating a timer ourselves.

This patch takes care of Telemetry startup in the content process by using a separate event (app-startup). Clean-up and sending ping to chrome process on shutdown will come in a different patch.

Comment on attachment 8529245 [details] [diff] [review] Start Telemetry on content process startup (v1) Review of attachment 8529245 [details] [diff] [review]: ----------------------------------------------------------------- ::: toolkit/components/telemetry/TelemetryPing.jsm @@ +914,5 @@ > > /** > + * Perform telemetry initialization for either chrome or content process. > + */ > + enableTelemetry: function enableTelemetry(testing) { Maybe call it "enableTelemetryRecording"? @@ -913,5 @@ > */ > setup: function setup(aTesting) { > // Initialize some probes that are kept in their own modules > this._thirdPartyCookies = new ThirdPartyCookieProbe(); > this._thirdPartyCookies.init(); I think this belongs in the content process @@ +1030,5 @@ > + if (!this.enableTelemetry()) { > + return; > + } > + > + this.observe(null, "sessionstore-windows-restored", null); 1) Shouldn't we set "this._hasWindowRestoredObserver = true" here? 2) I think we should observe "xul-window-visible" in the content process as well.. Does this event not exist in the content process? Is there an equivalent "first-paint" event? @@ +1139,1 @@ > return this.setup(); consider calling it setupParentProcess() or setupParentTelemetry() instead of setup() @@ +1158,5 @@ > } > break; > case "sessionstore-windows-restored": > + if (this._hasWindowRestoredObserver) { > + Services.obs.removeObserver(this, "sessionstore-windows-restored"); why is this removeObserver protected by an if-check now? We wouldn't get the sessionstore-windows-restored event unless an observer was registered, right?

Did you test out these patches in a build?

Addressed comments from the review, and I tested the patches to make sure they work. (In reply to Vladan Djeric (:vladan) from comment #3) > Comment on attachment 8529245 [details] [diff] [review] > Start Telemetry on content process startup (v1) > > Review of attachment 8529245 [details] [diff] [review]: > ----------------------------------------------------------------- > > @@ -913,5 @@ > > */ > > setup: function setup(aTesting) { > > // Initialize some probes that are kept in their own modules > > this._thirdPartyCookies = new ThirdPartyCookieProbe(); > > this._thirdPartyCookies.init(); > > I think this belongs in the content process Isn't networking limited to the chrome process? I did rename setup to setupChromeProcess. > @@ +1030,5 @@ > > + if (!this.enableTelemetry()) { > > + return; > > + } > > + > > + this.observe(null, "sessionstore-windows-restored", null); > > 1) Shouldn't we set "this._hasWindowRestoredObserver = true" here? We actually just simulate "sessionstore-windows-restored", and we don't actually register for it in the content process. I added a comment to explain that. > 2) I think we should observe "xul-window-visible" in the content process as > well.. Does this event not exist in the content process? Is there an > equivalent "first-paint" event? I saw that our "xul-window-visible" handler only records some startup I/O telemetry, which is not relevant to the content process, so I didn't include it. Also added a comment. > @@ +1158,5 @@ > > } > > break; > > case "sessionstore-windows-restored": > > + if (this._hasWindowRestoredObserver) { > > + Services.obs.removeObserver(this, "sessionstore-windows-restored"); > > why is this removeObserver protected by an if-check now? We wouldn't get the > sessionstore-windows-restored event unless an observer was registered, right? Because we're simulating "sessionstore-windows-restored" in the content process and not actually registering for it, we need an additional check here.

(In reply to Jim Chen [:jchen] from comment #5) > Isn't networking limited to the chrome process? I did rename setup to > setupChromeProcess. I took a second look and you're right, these cookie histograms are accumulated in the parent process. > We actually just simulate "sessionstore-windows-restored", and we don't > actually register for it in the content process. I added a comment to > explain that. Gotcha. Still, it's a bit awkward to gather data by faking an event that doesn't even exist in the content process. Also, will the content process even be able to get the I/O counter info it gets sandboxed? I think we should just have setupContentProcess call gatherStartupHistograms() directly instead of sending the fake event. We don't need the child process's I/O counter or debuggerAttached info.

https://hg.mozilla.org/integration/mozilla-inbound/rev/f2c361538514 https://hg.mozilla.org/integration/mozilla-inbound/rev/ffb52826fcfa

https://hg.mozilla.org/mozilla-central/rev/f2c361538514 https://hg.mozilla.org/mozilla-central/rev/ffb52826fcfa