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)
WebExtensions
General
Tracking
(firefox56+ fixed)
RESOLVED
FIXED
mozilla56
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
Reporter | ||
Comment 1•7 years ago
|
||
Benjamin, Bob: could you take a look ? Thanks !
status-firefox56:
--- → affected
tracking-firefox56:
--- → ?
Flags: needinfo?(bob.silverberg)
Flags: needinfo?(benjamin)
Assignee | ||
Comment 2•7 years ago
|
||
Thanks Tomcat. I will take a look.
Assignee: nobody → bob.silverberg
Priority: -- → P1
Updated•7 years ago
|
Flags: needinfo?(benjamin)
Assignee | ||
Comment 3•7 years ago
|
||
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)
Assignee | ||
Comment 4•7 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment 7•7 years ago
|
||
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)
Updated•7 years ago
|
Attachment #8887906 -
Flags: review?(alessio.placitelli)
Comment 8•7 years ago
|
||
mozreview-review |
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+
Assignee | ||
Comment 9•7 years ago
|
||
Thanks so much for taking care of this high priority bug Luca.
Comment hidden (mozreview-request) |
Comment 11•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Comment 12•7 years ago
|
||
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)
Assignee | ||
Comment 13•7 years ago
|
||
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?
Comment 14•7 years ago
|
||
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)
Comment hidden (Intermittent Failures Robot) |
Comment 16•7 years ago
|
||
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
Comment 17•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/2039119836ea
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Updated•6 years ago
|
Product: Toolkit → WebExtensions
You need to log in
before you can comment on or make changes to this bug.
Description
•