952180 - BrowserUITelemetry is mostly returning empty toolbar data.

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Mike Conley (:mconley) (:⚙️)]

Out of curiosity, I took a look at some of my profile's saved pings to see how the UITelemetry stuff is being recorded.

For almost all of the saved session pings, I'm seeing empty objects attached to UITelemetry, which is not good.

I did a job on an EC2 instance to get pings form yesterday's Aurora's to see if this is typical. It appears to be.

So:

The bad: we've gathered probably no useful UITelemetry data at this point.
The good: we caught this early, and we can fix it.

I think this has to do with the fact that most saved session pings attempt to gather their data during the shutdown phase after the windows have been closed. The BrowserUITelemetry getSimpleMeasures code bails out if it can't find a browser window to read toolbar data from. The solution to this is to try to read this sometime after browser-delayed-startup-finished and cache it.

I'll be working on that this afternoon.

Comment 1

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1 - for non-Australis

My approach here is to gather the measurements on the first window that opens, after it's fired its browser-delayed-startup-finished event, and cache those measurements until the point where a TelemetryPing is gathered.

This means that things like changes to the toolbar contents and collapsed states will not be captured until the next browser session. Customize mode duration and click events on toolbar items will, however, be up to date.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8350254 [details] [diff] [review]
Patch v1 - for non-Australis

Thoughts on this, jaws? I've tested locally that this actually capturing stuff on profile shut down, and it is indeed doing that.

Comment 3

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1 - for Australis

Comment 4

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8350276 [details] [diff] [review]
Patch v1 - for Australis

And the Australis port.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8350254 [details] [diff] [review]
Patch v1 - for non-Australis

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

Looks mostly good, but still need to address the popup issue mentioned below.

::: browser/modules/BrowserUITelemetry.jsm
@@ +318,5 @@
> +    // UITelemetry to ask for our measurements because at that point
> +    // all browser windows have probably been closed, since the vast
> +    // majority of saved-session pings are gathered during shutdown.
> +    if (!this._firstWindowMeasurements) {
> +      this._firstWindowMeasurements = this._getWindowMeasurements(aWindow);

The previous code made sure that the window being used was non-private and non-popup. This now does neither of those two. Over IRC it was said that we are OK with gathering this kind of telemetry from private windows, but I'm pretty sure we still need to make sure that the window is not a popup window.

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8350276 [details] [diff] [review]
Patch v1 - for Australis

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

Same feedback as other patch.

Comment 7

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1 - for non-Australis

Thanks for the review! Updated so that we check for aWindow.toolbar.visible before sampling the window. We'll skip windows until we hit one that has aWindow.toolbar.visible true.

If we somehow never get to a non-popup window, getToolbarMeasurements is ready to handle this._firstWindowMeasurements being null, and just packs the rest of the measurements into an empty object.

Comment 8

[Mike Conley (:mconley) (:⚙️)]

Attachment: Patch v1.1 - for Australis

And the Australis patch.

Comment 9

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8350299 [details] [diff] [review]
Patch v1.1 - for non-Australis

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 

BrowserUITelemetry wasn't reporting things because we failed to account for the fact that most TelemetryPings are gathered when the browser is shutting down and windows are closed. BrowserUITelemetry was returning nothing because it couldn't find a window to sample.


User impact if declined: 

None, but no UITelemetry data for us. :(


Testing completed (on m-c, etc.): 

Tested that shutdown pings contain the UITelemetry data that we require.


Risk to taking this patch (and alternatives if risky): 

Very low.


String or IDL/UUID changes made by this patch:

None.

Comment 10

[Mike Conley (:mconley) (:⚙️)]

Landed non-Australis patch on Holly: https://hg.mozilla.org/projects/holly/rev/106d0a6c8665

Landed Australis patch on fx-team: https://hg.mozilla.org/integration/fx-team/rev/c3239da6de33

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Thanks!

Landed on...

mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/bc261bbedf76

mozilla-beta: https://hg.mozilla.org/releases/mozilla-beta/rev/fa4b83484b56