1277644 - browser_Addons_sample should ensure that it burns CPOW time when asked to.

Status: RESOLVED FIXED

(Toolkit :: Performance Monitoring, defect)

Description

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

browser_Addons_sample.xpi is a test add-on that browser_AddonWatcher.js uses to test the add-on performance monitoring mechanism.

One of the things that browser_Addons_sample.xpi does is listen for an observer notification from browser_AddonWatcher.js that will start doing heavy CPOW operations for about 5 seconds in the hopes that this will be reported.

The code in the add-on that sends the CPOW object up to the parent for it to manipulate doesn't ensure that the CPOW is actually a CPOW. The code just holds on to the last object that the add-on framescript passed up for the test-addonwatcher-cpow:init message. That object might not be a CPOW, especially if the most recent tab to run that framescript wasn't remote.

We should make sure that only a CPOW is passed in this case and used, otherwise this test will time out waiting for CPOW traffic notifications.

Comment 1

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

I noticed this while sorting through test failures for my patch for bug 1261842, which makes the initial browser remote. I think that changes the order for the test-addonwatcher-cpow:init messages, and makes the last one arrive from a non-remote browser, so it's not actually a CPOW.

Comment 2

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

Ahah, so this might explain why I get intermittent errors. Any suggestion how to fix this?

Comment 3

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

(In reply to David Rajchenbach-Teller [:Yoric] (please use "needinfo") from comment #2)
> Ahah, so this might explain why I get intermittent errors. Any suggestion
> how to fix this?

Yeah, I've got a patch. Going to be more deliberate about which browser gets to run the framescript, and add a check to ensure that a CPOW is actually received from the content process framescript, and only let the parent run the test once all of those things have occurred.

Comment 4

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

Attachment: Bug 1277644 - Make sure browser_Addons_sample test add-on tests CPOWs when it is supposed to.

browser_Addons_sample is used by the browser_AddonWatcher.js test to make sure
we can properly detect when an add-on consumes a bunch of CPU or does a lot of
CPOW traffic.

There's a race condition in the add-on where what is supposed to be a CPOW
might not always be a CPOW, so when we try to cause a bunch of CPOW
traffic, we don't get the expected performance warnings.

This makes sure that when we try to simulate CPOW usage, we do it
with an actual CPOW.

Additionally, this commit also includes the unpacked source of the
add-on, which before wasn't in the tree. I've also taken the liberty
of bumping the add-on version and signing it.

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

Comment 5

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

Comment on attachment 8760956 [details]
Bug 1277644 - Make sure browser_Addons_sample test add-on tests CPOWs when it is supposed to.

https://reviewboard.mozilla.org/r/58348/#review55308

Looks good to me, thanks for the fix.

::: toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js:108
(Diff revision 1)
>      let snap1 = histogram.snapshot(ADDON_ID);
>      Assert.equal(snap1.sum, 0, `Histogram ${histogramName} is initially empty for the add-on`);
>  
> +    let timedOut = false;
> +    timer = setTimeout(() => {
> +      Assert.ok(false, "Timed out waiting for histogram to be updated, or detection to occur");

MMmhh, why?

::: toolkit/components/perfmonitoring/tests/browser/browser_AddonWatcher.js:120
(Diff revision 1)
>        yield new Promise(resolve => setTimeout(resolve, 100));
>        Services.obs.notifyObservers(null, topic, "");
>        yield new Promise(resolve => setTimeout(resolve, 100));
>  
>        let snap2 = histogram.snapshot(ADDON_ID);
> +      dump(`\nSnapshot of ${histogramName} for ${ADDON_ID} is: ${JSON.stringify(snap2, null, '\t')}\n`);

Looks like forgotten debugging code, right?

::: toolkit/components/perfmonitoring/tests/browser/browser_Addons_sample/content/framescript.js:10
(Diff revision 1)
> +"use strict";
> +
> +Components.utils.import("resource://gre/modules/Services.jsm");
> +
> +function burnCPOW(msg) {
> +  dump(`Addon: content burnCPU start ${Math.sin(Math.random())}\n`);

Probably my fault, but I imagine that this should print `burnCPOW` rather than `burnCPU` (and below).

Comment 6

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

https://reviewboard.mozilla.org/r/58348/#review55308

> MMmhh, why?

I thought waiting for the 5 minute timeout was a bit extreme, at least on my try pushes. I'll remove this.

> Looks like forgotten debugging code, right?

Good call - I'll remove this.

> Probably my fault, but I imagine that this should print `burnCPOW` rather than `burnCPU` (and below).

No problem, will fix and re-sign.

Comment 7

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

https://reviewboard.mozilla.org/r/58348/#review55308

> No problem, will fix and re-sign.

Actually, this function is used both for burning content CPU, and for causing CPOW traffic. I think I'll just leave this as is.

Comment 8

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

Comment on attachment 8760956 [details]
Bug 1277644 - Make sure browser_Addons_sample test add-on tests CPOWs when it is supposed to.

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

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e1dacbe0001
Make sure browser_Addons_sample test add-on tests CPOWs when it is supposed to. r=Yoric

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7e1dacbe0001