JAR off-main-thread breaks xpcshell-tests

RESOLVED FIXED in Firefox 59

Status

()

Core
Networking: JAR
P2
normal
RESOLVED FIXED
2 months ago
11 days ago

People

(Reporter: xeonchen, Assigned: xeonchen)

Tracking

(Blocks: 1 bug)

unspecified
mozilla59
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: [necko-triaged])

Attachments

(2 attachments, 4 obsolete attachments)

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
OK, the test installs an addon, the removes it before the manifest.json's |OnStartRequest|.
That causes the file deletion failure.
Summary: FAIL | toolkit/components/telemetry/tests/unit/test_TelemetryEnvironment.js → JAR off-main-thread breaks xpcshell-tests on Windows
Depends on: 1425512
Created attachment 8938313 [details] [diff] [review]
Part 1: make sure install/uninstall complete
Attachment #8938313 - Flags: review?(dtownsend)
Created attachment 8938314 [details] [diff] [review]
Part 2: wait for addon.uninstall complete
Attachment #8938314 - Flags: review?(rhelmer)
Created attachment 8938365 [details] [diff] [review]
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?
Attachment #8938365 - Flags: feedback?(kmaglione+bmo)
OS: Windows 7 → Unspecified
Summary: JAR off-main-thread breaks xpcshell-tests on Windows → JAR off-main-thread breaks xpcshell-tests
Attachment #8938314 - Flags: review?(rhelmer) → review?(aswan)
Attachment #8938313 - Flags: review?(dtownsend) → review?(aswan)

Comment 7

a month ago
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()
Attachment #8938313 - Flags: review?(aswan) → review+

Comment 8

a month ago
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?
Attachment #8938314 - Flags: review?(aswan) → review-
(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?
Flags: needinfo?(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).
Attachment #8938313 - Flags: feedback+
Attachment #8938314 - Attachment is obsolete: true
Attachment #8938365 - Attachment is obsolete: true
Attachment #8938365 - Flags: feedback?(kmaglione+bmo)
Created attachment 8940917 [details] [diff] [review]
(v2) make sure install/uninstall complete

fix based on reviewer's comment, and carry r+
Attachment #8940917 - Flags: review+
Attachment #8938313 - Attachment is obsolete: true
Flags: needinfo?(gfritzsche)
Created attachment 8941083 [details] [diff] [review]
Part 2: wait the bg iframe to be loaded in content scripts register unload
Attachment #8941083 - Flags: review?(lgreco)

Updated

13 days ago
Attachment #8941083 - Flags: review?(lgreco) → review+
Created attachment 8941283 [details] [diff] [review]
(v2.1) Part 1: make sure install/uninstall complete

add "part" to log message, carry r+
Attachment #8940917 - Attachment is obsolete: true
Attachment #8941283 - Flags: review+

Comment 15

12 days ago
Pushed by rgurzau@mozilla.com:
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
Keywords: checkin-needed

Comment 16

11 days ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/80022d102ad7
https://hg.mozilla.org/mozilla-central/rev/1adb2c87c080
Status: ASSIGNED → RESOLVED
Last Resolved: 11 days ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59
You need to log in before you can comment on or make changes to this bug.