1422823 - figure out why test_ext_contentScripts_register.js fails after fixing long shutdown delay in bug 1420594

Status: RESOLVED DUPLICATE of bug 1477688

(WebExtensions :: Android, enhancement, P3)

Description

[Ben Kelly [:bkelly, not reviewing]]

I fixed a shutdown delay in bug 1420594.  This seems to have caused test_ext_contentScripts_register.js to start failing.  I'm disabling for now since we really need to correct the shutdown delay to avoid more leaks, errors being added.

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

AFAICT this test was added while the shutdown delay was in place and probably just doesn't handle shutdown correctly.

Comment 2

[Ben Kelly [:bkelly, not reviewing]]

Luca, can you investigate why this fails when shutdown is not delayed?  I see some other tests in this directory are disabled on android debug.  Is this just similar to those reasons?

Comment 3

[Luca Greco [:rpl] [:luca] [:lgreco]]

(In reply to Ben Kelly [:bkelly] from comment #2)
> Luca, can you investigate why this fails when shutdown is not delayed?

Yes, absolutely, I'm looking into it.
(and I think that it is likely to be related to the "unregistering of programmatically registered content script when the extension is shutting down", which we could and we should avoid if the extension is shutting down because the entire browser
is exiting).

> I see some other tests in this directory are disabled on android debug.  Is
> this just similar to those reasons?

The reasons behind the WebExtensions tests that are currently disabled only on android debug are usually related to the fact that the arm emulator is pretty slow and it is even slower when we are running on a debug build, and so these tests have an higher rate of failures on android debug (on the contrary the tests that are disabled on all the android targets are usually related to unimplemented APIs on Android, and in lower number of cases because of a testing strategy that works on Desktop but not on Android).

Comment 4

[Luca Greco [:rpl] [:luca] [:lgreco]]

Attachment: Bug 1422823 - Re-enable test_ext_contentScripts_register.js xpcshell-test on Firefox for Android.

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

Comment 5

[Luca Greco [:rpl] [:luca] [:lgreco]]

Follows a try push related to the attached patch:

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

The attach patch is re-enabling the test file and skip a single test case, test_contentscripts_register_js (https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/toolkit/components/extensions/test/xpcshell/test_ext_contentScripts_register.js#248), as a proof that even if the crash is happening in the test case that follows (test_contentscripts_register_all_options, https://searchfox.org/mozilla-central/rev/f5f1c3f294f89cfd242c3af9eb2c40d19d5e04e7/toolkit/components/extensions/test/xpcshell/test_ext_contentScripts_register.js#414), it is actually triggered by the test that runs before it.

It is also worth to mention that a crash with the same error messages is also happening on Android debug on another of the WebExtensions xpcshell tests, test_ext_redirects.js (filed as Bug 1423475).

The following error messages and assertion errors are in common between the crashes happened on test_ext_contentScripts_register.js and test_ext_redirects.js:

JavaScript error: jar:jar:file:///data/local/xpcb/target.apk!/assets/omni.ja!/components/nsAsyncShutdown.js, line 119: Error: We have already registered a distinct blocker with the same name: ClientManagerService: start destroying IPC actors early
WARNING: '!mClientSource', file /builds/worker/workspace/build/src/dom/base/nsGlobalWindowInner.cpp, line 1804
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /builds/worker/workspace/build/src/dom/base/nsGlobalWindowInner.cpp, line 1856
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /builds/worker/workspace/build/src/dom/base/nsGlobalWindowOuter.cpp, line 2015
WARNING: ContentViewer Initialization failed: file /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp, line 9702
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp, line 7517
WARNING: NS_ENSURE_SUCCESS(rv, rv) failed with result 0x80004005: file /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp, line 8399
WARNING: NS_ENSURE_TRUE(mContentViewer) failed: file /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp, line 8255
WARNING: NS_ENSURE_SUCCESS(EnsureContentViewer(), nullptr) failed with result 0x8000FFFF: file /builds/worker/workspace/build/src/docshell/base/nsDocShell.cpp, line 4759
Segmentation fault

Follows links to the failures logs on treeherder:

- test_ext_contentScripts_register.js xpcshell-test crash: https://treeherder.mozilla.org/logviewer.html#?job_id=149165468&repo=mozilla-inbound&lineNumber=1875
- test_ext_redirects.js xpcshell-test crash: https://treeherder.mozilla.org/logviewer.html#?job_id=150077116&repo=mozilla-inbound&lineNumber=1775

Based on a brief chat over IRC with bkelly, it seems that the logged errors can be fixed by the patch attached to Bug 1423913 Comment 12.

I'm going to try to trigger the issue again in a push to try on top of the changes that https://bugzilla.mozilla.org/show_bug.cgi?id=1423913#c12 is going to introduce.

Comment 6

[Luca Greco [:rpl] [:luca] [:lgreco]]

The push to try with the patches from Bug 1423913:

- https://treeherder.mozilla.org/#/jobs?repo=try&revision=45cc3a1d7424161fa2054b6d357f64e96b0d2637

it doesn't trigger the xpcshell-test crash anymore on this test file.

I'm adding Bug 1423913 as a blocker for this bug.

Comment 7

[Luca Greco [:rpl] [:luca] [:lgreco]]

Comment on attachment 8935445 [details]
Bug 1422823 - Re-enable test_ext_contentScripts_register.js xpcshell-test on Firefox for Android.

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