1383728 - Add missing tests back into manifests

Status: RESOLVED FIXED

(WebExtensions :: General, enhancement, P2)

Description

[Bob Silverberg [:bsilverberg]]

As per bug #1383221, the following tests are all not included in any active manifests:

toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js
toolkit/components/extensions/test/xpcshell/test_ext_extension_content_telemetry.js
toolkit/components/extensions/test/xpcshell/test_ext_storage_telemetry.js

It looks like they were removed in https://hg.mozilla.org/mozilla-central/rev/9704283b0e3a, but never added back.

When I tried simply adding them to toolkit/components/extensions/test/xpcshell/xpcshell-common.ini, I found that the two telemetry tests were failing with one of the configurations, so I need to spend some time figuring out what the issue is.

Comment 1

[Bob Silverberg [:bsilverberg]]

Luca and I did some debugging of the telemetry tests which are failing so I wanted to record our findings:

- The two telemetry tests listed above fail when run in OOP mode, but pass in in-process mode. They fail when we check that expected telemetry has been recorded.
- I checked the value of Services.telemetry.canRecordBase in both the test and in the frameScript that is injected into the content process and it reports `true` in all cases.
- Based on log output from TelemetryStopwatch.jsm, it does appear as if the telemetry is being recorded as expected during the tests.
- I tried using a ContentTask to check for the telemetry (in addition to checking in the test body) but that still reports that no telemetry has been recorded.

I wondered why the tests passed in the past, but it looks like they were not configured to run in OOP mode until [1] landed and, unfortunately, they were removed from the manifest at that time, so likely have never been run in OOP mode.

[1] https://hg.mozilla.org/mozilla-central/rev/9704283b0e3a

Comment 2

[Bob Silverberg [:bsilverberg]]

Some more information on this:

I initially thought the issue had to do with telemetry related to content scripts, but running test_ext_storage_telemetry.js in OOP mode I discovered that event the call to browser.storage from the background page was failing to register telemetry in the test.

I did a manual test, running Nightly in OOP mode, and I noticed that about:telemetry now lists 3 different processes from which you can look at telemetry. One of the histograms that is causing a problem in the test is WEBEXT_STORAGE_LOCAL_GET_MS, and I notice that it appears not under the "parent" process, but under the "extension" process, so perhaps the problem is that we need to look for it there when we are checking for the telemetry being recorded in the test. I will look at the telemetry modules to see if I can find a way to target the extension process rather than the parent process.

Comment 3

[Bob Silverberg [:bsilverberg]]

Attachment: Bug 1383728 - Add missing tests back into manifests,

The following tests were removed from the test manifest in  https://hg.mozilla.org/mozilla-central/rev/9704283b0e3a and never added back:
toolkit/components/extensions/test/xpcshell/test_ext_browserSettings.js
toolkit/components/extensions/test/xpcshell/test_ext_extension_content_telemetry.js
toolkit/components/extensions/test/xpcshell/test_ext_storage_telemetry.js

A bunch of changes were needed to the telemetry tests to make them work in OOP mode as well.

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

Comment 4

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8890358 [details]
Bug 1383728 - Add missing tests back into manifests,

Sorry for the confusion with this patch the other day. I solved the issues with the telemetry tests so this is now ready to be reviewed.

Comment 5

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8890358 [details]
Bug 1383728 - Add missing tests back into manifests,

https://reviewboard.mozilla.org/r/161472/#review166856

::: toolkit/components/extensions/test/xpcshell/head_telemetry.js:10
(Diff revision 1)
> +/* exported IS_OOP, arraySum, clearHistograms, getSnapshots, promiseTelemetryRecorded */
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "ContentTaskUtils",
> +                                  "resource://testing-common/ContentTaskUtils.jsm");
> +
> +const IS_OOP = Services.prefs.getBoolPref("extensions.webextensions.remote");

It seems to me we could have a default process value here, rather than passing in from the tests.

Comment 6

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8890358 [details]
Bug 1383728 - Add missing tests back into manifests,

https://reviewboard.mozilla.org/r/161472/#review166856

> It seems to me we could have a default process value here, rather than passing in from the tests.

I suppose we could, but in test_ext_storage_telemetry.js we need to switch the value of `process` partway through the test, and it feels strange to me to set a local variable in head_telemetry.js and then override it partway through the test in the test file. Passing it as an argument seems cleaner. Do you disagree?

Comment 7

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8890358 [details]
Bug 1383728 - Add missing tests back into manifests,

https://reviewboard.mozilla.org/r/161472/#review166856

> I suppose we could, but in test_ext_storage_telemetry.js we need to switch the value of `process` partway through the test, and it feels strange to me to set a local variable in head_telemetry.js and then override it partway through the test in the test file. Passing it as an argument seems cleaner. Do you disagree?

oh, I missed that you reversed values for process where you set it.  In any case, it was just a thought rather than a must do.

Comment 8

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8890358 [details]
Bug 1383728 - Add missing tests back into manifests,

https://reviewboard.mozilla.org/r/161472/#review166856

> oh, I missed that you reversed values for process where you set it.  In any case, it was just a thought rather than a must do.

It _was_ a good idea, except for that. ;)  Thanks for the suggestion.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ca779a5aa93f
Add missing tests back into manifests, r=mixedpuppy

Comment 10

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing the xpcshell tests test_ext_storage_telemetry.js and test_ext_extension_content_telemetry.js on Android:

https://hg.mozilla.org/integration/autoland/rev/eeb8b3ad1b548d635b63bc0af9cb805af192604b

Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ca779a5aa93fecf65a56647492208f3a17fd215c&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=118426256&repo=autoland

[task 2017-07-27T09:49:01.355304Z] 09:49:01  WARNING -  TEST-UNEXPECTED-FAIL | xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_storage_telemetry.js | xpcshell return code: 0
[task 2017-07-27T09:49:01.356740Z] 09:49:01     INFO -  TEST-INFO took 7010ms
[task 2017-07-27T09:49:01.356838Z] 09:49:01     INFO -  >>>>>>>
[task 2017-07-27T09:49:01.356972Z] 09:49:01     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_storage_telemetry.js | xpcw: cd /storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell
[task 2017-07-27T09:49:01.357783Z] 09:49:01     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_storage_telemetry.js | xpcw: xpcshell -r /storage/sdcard/tests/xpc/c/httpd.manifest --greomni /data/local/xpcb/target.apk -m -s -e const _HEAD_JS_PATH = "/storage/sdcard/tests/xpc/head.js"; -e const _MOZINFO_JS_PATH = "/storage/sdcard/tests/xpc/p/mozinfo.json"; -e const _TESTING_MODULES_DIR = "/storage/sdcard/tests/xpc/m"; -f /storage/sdcard/tests/xpc/head.js -e const _SERVER_ADDR = "localhost" -e const _HEAD_FILES = ["/storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell/head.js", "/storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell/head_telemetry.js"]; -e const _JSDEBUGGER_PORT = 0; -e const _TEST_FILE = ["test_ext_storage_telemetry.js"]; -e const _TEST_NAME = "xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_storage_telemetry.js" -e _execute_test(); quit(0);
[task 2017-07-27T09:49:01.360880Z] 09:49:01     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2017-07-27T09:49:01.361642Z] 09:49:01     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2017-07-27T09:49:01.362370Z] 09:49:01     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2017-07-27T09:49:01.362464Z] 09:49:01     INFO -  running event loop
[task 2017-07-27T09:49:01.363832Z] 09:49:01     INFO -  xpcshell.ini:toolkit/components/extensions/test/xpcshell/test_ext_storage_telemetry.js | Starting test_telemetry_background
[task 2017-07-27T09:49:01.363949Z] 09:49:01     INFO -  (xpcshell/head.js) | test test_telemetry_background pending (2)
[task 2017-07-27T09:49:01.364393Z] 09:49:01     INFO -  "Extension attached"
[task 2017-07-27T09:49:01.364426Z] 09:49:01     INFO -  "Extension attached"
[task 2017-07-27T09:49:01.364456Z] 09:49:01     INFO -  (xpcshell/head.js) | test run_next_test 0 finished (2)
[task 2017-07-27T09:49:01.364545Z] 09:49:01     INFO -  Unexpected exception TypeError: Services.telemetry.snapshotSubsessionHistograms(...) is undefined at /storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell/head_telemetry.js:21
[task 2017-07-27T09:49:01.364625Z] 09:49:01     INFO -  getSnapshots@/storage/sdcard/tests/xpc/toolkit/components/extensions/test/xpcshell/head_telemetry.js:21:10
[task 2017-07-27T09:49:01.364715Z] 09:49:01     INFO -  test_telemetry_background@test_ext_storage_telemetry.js:51:19
[task 2017-07-27T09:49:01.364777Z] 09:49:01     INFO -  asyncFunction@resource://gre/modules/Task.jsm:241:18
[task 2017-07-27T09:49:01.364836Z] 09:49:01     INFO -  Task_spawn@resource://gre/modules/Task.jsm:166:12
[task 2017-07-27T09:49:01.364888Z] 09:49:01     INFO -  _run_next_test@/storage/sdcard/tests/xpc/head.js:1488:9
[task 2017-07-27T09:49:01.364923Z] 09:49:01     INFO -  run@/storage/sdcard/tests/xpc/head.js:701:9
[task 2017-07-27T09:49:01.364950Z] 09:49:01     INFO -  _do_main@/storage/sdcard/tests/xpc/head.js:221:3
[task 2017-07-27T09:49:01.364981Z] 09:49:01     INFO -  _execute_test@/storage/sdcard/tests/xpc/head.js:544:5
[task 2017-07-27T09:49:01.365004Z] 09:49:01     INFO -  @-e:1:1
[task 2017-07-27T09:49:01.365022Z] 09:49:01     INFO -  exiting test
[task 2017-07-27T09:49:01.365039Z] 09:49:01     INFO -  <<<<<<<

Comment 11

[Bob Silverberg [:bsilverberg]]

Luca, I wonder if this is a similar problem to what we saw on Android before, or if it may have to do with the changes I needed to make to get this working with OOP. Would you be able to try these tests with your Android build?

Comment 12

[Bob Silverberg [:bsilverberg]]

Comment on attachment 8890358 [details]
Bug 1383728 - Add missing tests back into manifests,

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

Comment 13

[Bob Silverberg [:bsilverberg]]

Nevermind, Luca. I figured it out. The code I added to support OOP extensions won't work on Android.

I've disabled the tests on Android for now and will try to re-land. I've opened bug 1384923 to re-enable the tests.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/95e19ac11ce9
Add missing tests back into manifests, r=mixedpuppy

Comment 15

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

https://hg.mozilla.org/mozilla-central/rev/95e19ac11ce9