Closed Bug 1380287 Opened 7 years ago Closed 7 years ago

Perma failure when 56 merges to beta in test_chrome_ext_contentscript_telemetry.html | Data recorded for first extension for histogram: WEBEXT_CONTENT_SCRIPT_INJECTION_MS.

Categories

(WebExtensions :: General, defect, P1)

defect

Tracking

(firefox56+ fixed)

RESOLVED FIXED
mozilla56
Tracking Status
firefox56 + fixed

People

(Reporter: cbook, Assigned: bsilverberg)

References

()

Details

Attachments

(1 file)

found during uplift simulations with trunk as beta

 173 INFO TEST-UNEXPECTED-FAIL | toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_telemetry.html | Data recorded for first extension for histogram: WEBEXT_CONTENT_SCRIPT_INJECTION_MS. 

https://treeherder.mozilla.org/logviewer.html#?job_id=112601724&repo=try

needs to be fixed before we merge 56 to beta to avoid perma failures and so tree closures 

i guess this is a fallout from bug 1356323
Benjamin, Bob: could you take a look ? Thanks !
Flags: needinfo?(bob.silverberg)
Flags: needinfo?(benjamin)
Thanks Tomcat. I will take a look.
Assignee: nobody → bob.silverberg
Priority: -- → P1
Flags: needinfo?(benjamin)
It looks like this is only failing on Android. Matt/Luca, has anything landed on Android between beta and now that would explain why this is passing on Android in Nightly but fails in beta?
Flags: needinfo?(mwein)
Flags: needinfo?(lgreco)
Flags: needinfo?(bob.silverberg)
Matt, do you think you'll get a chance to try reproducing this with the patch from bug 1356323 applied against beta anytime in the next few days? If not, then we should probably just disable the test on Android as this is a blocker to merge this into 56.
Track 56+ as the test might be a blocker to be merged into 56.
Hey Bob,
I've been able to reproduce this issue locally and dig into it a bit:
it seems to be related to the status of the telemetry, which seems to not be enabled when this runs on Android in beta.

I've tried to fix it locally by explicitly forcing the telemetry on in the test (and then reset it to its previous state when the test is completed) and it seems to be enough to fix this perma failure.

I'm attaching the patch that I tried locally to fix the failure on this issue.
Flags: needinfo?(mwein)
Flags: needinfo?(lgreco)
Attachment #8887906 - Flags: review?(alessio.placitelli)
Comment on attachment 8887906 [details]
Bug 1380287 - Turn on telemetry in test_chrome_ext_contentscript_telemetry.html.

https://reviewboard.mozilla.org/r/158820/#review164130

Thank you for your patch and your patience! This looks good with the changes below addressed.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_telemetry.html:20
(Diff revision 1)
>  
>  const HISTOGRAM = "WEBEXT_CONTENT_SCRIPT_INJECTION_MS";
>  
>  add_task(async function test_contentscript_telemetry() {
> +  // Turn on telemetry and reset it to the previous state once the test is completed.
> +  // NOTE: switching the "toolkit.telemetry.enabled" preference

Please also mention that this is only required for Android, which does not properly support unified telemetry: we're always recording opt-out telemetry on desktop.

::: toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_telemetry.html:60
(Diff revision 1)
>  
>    extension.sendMessage();
>    await extension.awaitMessage("content-script-run");
>  
>    let histogramSum = histogram.snapshot().sum;
> +

nit: unintentional blank line?
Attachment #8887906 - Flags: review?(alessio.placitelli) → review+
Thanks so much for taking care of this high priority bug Luca.
Comment on attachment 8887906 [details]
Bug 1380287 - Turn on telemetry in test_chrome_ext_contentscript_telemetry.html.

https://reviewboard.mozilla.org/r/158820/#review164130

> Please also mention that this is only required for Android, which does not properly support unified telemetry: we're always recording opt-out telemetry on desktop.

Sure, added it to the inline comment in the updated patch.

> nit: unintentional blank line?

yep, I removed some additional logging added while debugging the issue and I forgot to remove the blank line.

Thanks! removed in the updated patch.
Luca, won't this be a problem for all of the tests for telemetry pings on Android? This isn't the only test like that.
Flags: needinfo?(lgreco)
Here's a list of all the telemetry tests I added:

browser/components/extensions/test/browser/browser_ext_pageAction_telemetry.js 	
browser/components/extensions/test/browser/browser_ext_browserAction_telemetry.js
 	
toolkit/components/extensions/test/xpcshell/test_ext_extension_startup_telemetry.js 	
toolkit/components/extensions/test/xpcshell/test_ext_background_telemetry.js 	
toolkit/components/extensions/test/xpcshell/test_ext_storage_telemetry.js 	
toolkit/components/extensions/test/xpcshell/test_ext_extension_content_telemetry.js
toolkit/components/extensions/test/mochitest/test_chrome_ext_contentscript_telemetry.html 	

I'm guessing we don't run tests from /browser on Android, so those are likely not a problem, but the others may be. Perhaps the xpcshell tests are okay, and it's only the mochitest that causes an issue?
Thanks for the list Bob!

So, by looking at the list from Comment 13 and the various tests that I did locally, the only one that should have this issue on Android is the mochitest tweaked in the attached patch.

(The tests at the browser level will not be part of the tests executed on android, and I've also executed some of the telemetry xpcshell tests and they seems to work fine on beta, which is expected given that they do not run into a real Firefox for Android instance and so the preferences are not configured in the same way).
Flags: needinfo?(lgreco)
Pushed by luca.greco@alcacoop.it:
https://hg.mozilla.org/integration/autoland/rev/2039119836ea
Turn on telemetry in test_chrome_ext_contentscript_telemetry.html. r=Dexter
https://hg.mozilla.org/mozilla-central/rev/2039119836ea
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.