1251469 - Add a telemetry probe to measure time to sanitize a loaded or unloaded Flash

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Marco Bonardo [:mak]]

We take a lot of time to clear plugins in some cases, but it's unclear if the main culprit is Flash and whether the problem is loading it, or it's just slow clearing data.

We can add probes for that.

Comment 1

[Marco Bonardo [:mak]]

Attachment: MozReview Request: Bug 1251469 - Add telemetry probes for the time needed to sanitize a loaded or unloaded Flash. r=yoric

Review commit: https://reviewboard.mozilla.org/r/36837/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/36837/

Comment 2

[Marco Bonardo [:mak]]

I must recall to add bug_numbers to the new probes, since they were made mandatory 5 minutes ago!

Comment 3

[Marco Bonardo [:mak]]

Comment on attachment 8724051 [details]
MozReview Request: Bug 1251469 - Add telemetry probes for the time needed to sanitize a loaded or unloaded Flash. r=yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36837/diff/1-2/

Comment 4

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8724051 [details]
MozReview Request: Bug 1251469 - Add telemetry probes for the time needed to sanitize a loaded or unloaded Flash. r=yoric

https://reviewboard.mozilla.org/r/36837/#review33429

::: browser/base/content/sanitize.js:294
(Diff revision 1)
> +        let perPluginTelemetryProbe = (tag) => {

This function is called a single time. Moving this out of line makes the code a bit harder to read.

::: browser/base/content/sanitize.js:296
(Diff revision 1)
> +            return `FX_SANITIZE_${tag.loaded ? "LOADED" : "UNLOADED"}_FLASH`;

I'd prefer if you did
```js
return tag.loaded ?
   "FX_SANITIZER_LOADED_FLASH"
 : "FX_SANITIZER_UNLOADED_FLASH";

```

It makes it easier to both spot typos and search in the code.

::: browser/base/content/sanitize.js:308
(Diff revision 1)
> +            let probe = perPluginTelemetryProbe(tag);

Why don't we make this a keyed histogram, with key ```js
`${tag.name}: ${tag.loaded}`
```? This will help us pinpoint offenders more easily.

Since the Telemetry Ping already gives us the list of plugins (I believe), this doesn't cause any privacy risk.

::: browser/base/content/sanitize.js:323
(Diff revision 1)
> +                TelemetryStopwatch.finish(probe, refObj);

If you just want to know the time spent clearing site data by a plugin (rather than the time spent waiting for the event loop), you should rather put the calls to `TelemetryStopwatch` inside the calls to `new Promise`.

Comment 5

[Marco Bonardo [:mak]]

https://reviewboard.mozilla.org/r/36837/#review33429

> Why don't we make this a keyed histogram, with key ```js
> `${tag.name}: ${tag.loaded}`
> ```? This will help us pinpoint offenders more easily.
> 
> Since the Telemetry Ping already gives us the list of plugins (I believe), this doesn't cause any privacy risk.

I don't want to complicate the data more than we need. It's pretty clear Flash is the thing we care most, also because we are going to deprecate all NPAPI plugins bug Flash by end of the year. Plus it's likely Flash is one of the very few plugins supporting clearing data, cause IIRC it was added to NPAPI exactly for Flash.
Plus Flash is the only plugin that we will instantiate on sanitize if it's not yet loaded (single item whitelist).
So, I don't think it's worth measuring anything outside of flash.

> If you just want to know the time spent clearing site data by a plugin (rather than the time spent waiting for the event loop), you should rather put the calls to `TelemetryStopwatch` inside the calls to `new Promise`.

What affects the user is the whole thing, and that's what we want to measure. Moreover with lots of data having some skewed results due to a busy events loop doesn't really matter.
Thus I think the code complication to move this inside the promise is not worth it, cause then I should change all the callbacks, or I'd just measure the cost of the call.

Comment 6

[David Teller [:Yoric] - still alive but not very active]

https://reviewboard.mozilla.org/r/36837/#review33429

> I don't want to complicate the data more than we need. It's pretty clear Flash is the thing we care most, also because we are going to deprecate all NPAPI plugins bug Flash by end of the year. Plus it's likely Flash is one of the very few plugins supporting clearing data, cause IIRC it was added to NPAPI exactly for Flash.
> Plus Flash is the only plugin that we will instantiate on sanitize if it's not yet loaded (single item whitelist).
> So, I don't think it's worth measuring anything outside of flash.

Ok, fair enough.

> What affects the user is the whole thing, and that's what we want to measure. Moreover with lots of data having some skewed results due to a busy events loop doesn't really matter.
> Thus I think the code complication to move this inside the promise is not worth it, cause then I should change all the callbacks, or I'd just measure the cost of the call.

Ok, fair enough.

Comment 7

[David Teller [:Yoric] - still alive but not very active]

Comment on attachment 8724051 [details]
MozReview Request: Bug 1251469 - Add telemetry probes for the time needed to sanitize a loaded or unloaded Flash. r=yoric

https://reviewboard.mozilla.org/r/36837/#review33453

In that case, ship it with minor changes.

Comment 8

[Marco Bonardo [:mak]]

Comment on attachment 8724051 [details]
MozReview Request: Bug 1251469 - Add telemetry probes for the time needed to sanitize a loaded or unloaded Flash. r=yoric

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/36837/diff/2-3/

Comment 9

[Pulsebot]

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

Comment 10

[Carsten Book [:Tomcat]]

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

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Should we uplift this to 46?

Comment 12

[Marco Bonardo [:mak]]

we could, but it could also ride the trains. honestly there isn't much we can do in Aurora/Beta for plugins without taking risks.

Comment 13

[Bob Silverberg [:bsilverberg]]

Hi Marco, I am implementing a function similar to what promiseClearPluginCookies [1] does for the browsingData API. Should I include telemetry probes in my code as well, in the same locations, and if so, should I use the same probe names, so our data can be added to your data, or create new probe names of our own?

[1] http://searchfox.org/mozilla-central/source/browser/base/content/sanitize.js#314

Comment 14

[Marco Bonardo [:mak]]

(In reply to Bob Silverberg [:bsilverberg] from comment #13)
> Hi Marco, I am implementing a function similar to what
> promiseClearPluginCookies [1] does for the browsingData API. Should I
> include telemetry probes in my code as well, in the same locations, and if
> so, should I use the same probe names, so our data can be added to your
> data, or create new probe names of our own?

I think you should create a FX_SANITIZE_YOURDATANAME probe, for coherency, if your code ends up in the sanitizer