1422496 - JAR off-main-thread breaks xpcshell-tests

Status: RESOLVED FIXED

(Core :: Networking: JAR, enhancement, P2)

Description

[Gary Chen [:xeonchen]]

After applying patches of bug 1373708, xpcshell-test failed on win32 platform.

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff1839d893f36d838c6317d8c9e0d6a85a53fc9c

Comment 1

[Gary Chen [:xeonchen]]

TEST-UNEXPECTED-FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js | xpcshell return code: 0

Error: Test cleanup: path c:\\users\\genericworker\\appdata\\local\\temp\\xpc-profile-qiryw8\\extensions\\trash exists when it should not at resource://testing-common/AddonTestUtils.jsm:298 ([1, 2])

[1] https://treeherder.mozilla.org/logviewer.html#?job_id=149131940&repo=try&lineNumber=17375
[2] https://dxr.mozilla.org/mozilla-central/rev/709f355a7a8c4ae426d1824841a71ffdb5ce0137/toolkit/mozapps/extensions/internal/AddonTestUtils.jsm#298

Comment 2

[Gary Chen [:xeonchen]]

OK, the test installs an addon, the removes it before the manifest.json's |OnStartRequest|.
That causes the file deletion failure.

Comment 3

[Gary Chen [:xeonchen]]

Attachment: Part 1: make sure install/uninstall complete

Comment 4

[Gary Chen [:xeonchen]]

Attachment: Part 2: wait for addon.uninstall complete

Comment 5

[Gary Chen [:xeonchen]]

OMG, still long long way to go

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c3780dadae68992847ae550ee97f7e3608513f89

Comment 6

[Gary Chen [:xeonchen]]

Attachment: 0001-Bug-1422496-FIX-wait-for-applying-CSS.patch

Hi Kris,

In |test_contentscripts_unregister_on_context_unload|, sometimes |check_applied_styles| will be called before |registered_ext_style.css| is loaded.

This patch reduces the failure rate, but it still happens (about 1/10).
Do you have any idea?

Comment 7

[Andrew Swan [:aswan]]

Comment on attachment 8938313 [details] [diff] [review]
Part 1: make sure install/uninstall complete

Review of attachment 8938313 [details] [diff] [review]:
-----------------------------------------------------------------

uninstalls are synchronous, there's no need to wait for them, and if you have addon objects you can also just call addon.uninstall() instead of going through AddonManagerTesting.uninstallAddonByID()
I'm not sure what the intention is with that test (ie why it doesn't already have a matching uninstall for each install), please get a Telemetry peer to sign off on that change.

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +1187,5 @@
>              "Dictionaries should not appear in active addons.");
>  
>    TelemetryEnvironment.unregisterChangeListener("testNotInteresting");
> +
> +  await dictionaryAddon.startupPromise;

This is a no-op, dictionaries are loaded synchronously.

@@ +1188,5 @@
>  
>    TelemetryEnvironment.unregisterChangeListener("testNotInteresting");
> +
> +  await dictionaryAddon.startupPromise;
> +  await AddonManagerTesting.uninstallAddonByID(DICTIONARY_ADDON_ID);

This can also just be dictionaryAddon.uninstall()

Comment 8

[Andrew Swan [:aswan]]

Comment on attachment 8938314 [details] [diff] [review]
Part 2: wait for addon.uninstall complete

Review of attachment 8938314 [details] [diff] [review]:
-----------------------------------------------------------------

addon.uninstall() does not return a Promise.  Does this patch actually make a difference in tests?

Comment 9

[Gary Chen [:xeonchen]]

(In reply to Andrew Swan [:aswan] from comment #7)
> Comment on attachment 8938313 [details] [diff] [review]
> Part 1: make sure install/uninstall complete
> 
> Review of attachment 8938313 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> uninstalls are synchronous, there's no need to wait for them, and if you
> have addon objects you can also just call addon.uninstall() instead of going
> through AddonManagerTesting.uninstallAddonByID()
> I'm not sure what the intention is with that test (ie why it doesn't already
> have a matching uninstall for each install), please get a Telemetry peer to
> sign off on that change.
> 

Hi Georg, do you have any idea why the addons are not uninstalled?

Comment 10

[Georg Fritzsche [:gfritzsche]]

Comment on attachment 8938313 [details] [diff] [review]
Part 1: make sure install/uninstall complete

Review of attachment 8938313 [details] [diff] [review]:
-----------------------------------------------------------------

::: toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js
@@ +1188,5 @@
>  
>    TelemetryEnvironment.unregisterChangeListener("testNotInteresting");
> +
> +  await dictionaryAddon.startupPromise;
> +  await AddonManagerTesting.uninstallAddonByID(DICTIONARY_ADDON_ID);

From looking through the test, this just happens to work. E.g.:
(1) test_addonsWatch_NotInterestingChange() installs restartless.xpi
(2) checks that restartless.xpi shows up in the telemetry environment data
(3) (addon is not uninstalled)
(4) test_addonsAndPlugins() re-installs (?) restartless.xpi
(5) checks that restartless.xpi shows up in the telemetry environment data

This works with or without (3).

Comment 11

[Gary Chen [:xeonchen]]

Attachment: (v2) make sure install/uninstall complete

fix based on reviewer's comment, and carry r+

Comment 12

[Gary Chen [:xeonchen]]

Attachment: Part 2: wait the bg iframe to be loaded in content scripts register unload

Comment 13

[Gary Chen [:xeonchen]]

Attachment: (v2.1) Part 1: make sure install/uninstall complete

add "part" to log message, carry r+

Comment 14

[Gary Chen [:xeonchen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9fafd12416b332311571fc664355acfd950cd26e

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/80022d102ad7
Part 1: make sure install/uninstall complete; r=aswan
https://hg.mozilla.org/integration/mozilla-inbound/rev/1adb2c87c080
Part 2: wait the bg iframe to be loaded in content scripts register unload; r=rpl

Comment 16

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/80022d102ad7
https://hg.mozilla.org/mozilla-central/rev/1adb2c87c080