1575183 - TEST-UNEXPECTED-FAIL | devtools/server/tests/unit/test_extension_storage_actor.js when run in Thunderbird environment

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Jorg K (CEST = GMT+2)]

TEST-UNEXPECTED-FAIL | devtools/server/tests/unit/test_extension_storage_actor.js | xpcshell return code: 0
TEST-UNEXPECTED-FAIL | devtools/server/tests/unit/test_extension_storage_actor.js | test_extension_origin_matches_debugger_target - [test_extension_origin_matches_debugger_target : 262] Should have the expected extension host in the extensionStorage store - false == true
TEST-UNEXPECTED-FAIL | devtools/server/tests/unit/test_extension_storage_actor.js | test_extension_origin_matches_debugger_target - [test_extension_origin_matches_debugger_target : 326] Extension left running at test shutdown - "running" == "unloaded"

First seen:
https://treeherder.mozilla.org/#/jobs?repo=comm-central&selectedJob=262442694&revision=fc6df7c5e3dabb93283c233f20d06ed4e6aa072b

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=14bd73fd1e01022663fe79675d1273defe&tochange=e7e658ec1e98c3868b98195c2ee4b9fe38

That's a new test introduced here:
Bug 1542035 - Add read-only support for extension storage.local in addon debugger r=miker,rpl
https://hg.mozilla.org/mozilla-central/rev/d7bfcc967c3e2cefd9179772c89a489e44ff0918

Looks like we need to do this

+// Disable extensionStorage storage actor by default
+pref("devtools.storage.extensionStorage.enabled", false);

as well.

Comment 1

[Jorg K (CEST = GMT+2)]

Attachment: 1575183-extensionStorage.patch

I should rebuild and try it ;-)

Comment 2

[Jorg K (CEST = GMT+2)]

Comment on attachment 9086623 [details] [diff] [review]
1575183-extensionStorage.patch

Actually, that doesn't help. So what's next? Disable the test for TB? Bianca, maybe you have a suggestion. I'll attach a log in the next comment.

Comment 3

[Jorg K (CEST = GMT+2)]

Attachment: 1575183.txt

Look for the string "JavaScript", there are lots of errors.

Comment 4

[Bianca Danforth [:bdanforth]]

This test is for a new "extensionStorage" actor in the add-on toolbox's Storage panel to inspect extension storage from the browser.storage.local WebExtensions API.

The test failure means that, when the feature is enabled via the preference, the storage actor is not being listed in the tree on the left sidebar in the add-on toolbox’s Storage panel. I'm not familiar enough with the state of extensions and DevTools in Thunderbird to have ideas why the test might be failing in Thunderbird.

I recommend skipping this test for now, and if you would like this feature to work in Thunderbird, to file a follow-up issue to fix the test (and anything else). We set the "devtools.storage.extensionStorage.enabled" preference on the browser level, so it should not be enabled by default on Thunderbird even if we eventually enable it by default for Firefox.

Hope that helps!

Comment 5

[Jorg K (CEST = GMT+2)]

Thanks, Bianca. I'll see what Geoff says, then we'll provide a patch to disable the test for TB.

Comment 6

[Geoff Lankow (:darktrojan)]

Well this is weird, the UI works but the test does not.

Comment 7

[Geoff Lankow (:darktrojan)]

Attachment: Screenshot from 2019-08-21 21-22-07.png

Comment 8

[Jorg K (CEST = GMT+2)]

Geoff told me on IRC to install any WE that uses the storage API, like for example, Dropbox FileLink. So I installed that I also see the Extension Storage header. In fact, the inbuilt WeTransfer shows it too. I'm not sure whether that is was Bianca referred to as "storage actor".

Magnus, care to make a decision on how to proceed here? The test would need to be disabled in M-C, so that's more involved than just switching something off temporarily in C-C. Or can you find a resource to debug the failure?

Comment 9

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

(In reply to Jorg K (GMT+2) from comment #8)

Geoff told me on IRC to install any WE that uses the storage API, like for example, Dropbox FileLink. So I installed that I also see the Extension Storage header. In fact, the inbuilt WeTransfer shows it too. I'm not sure whether that is was Bianca referred to as "storage actor".

The "storage actor" mentioned by Bianca is the "Remote Debugging actor" related to the extension storage, which is defined here:

• https://searchfox.org/mozilla-central/rev/597a69c70a5cce6f42f159eb54ad1ef6745f5432/devtools/server/actors/storage.js#1582-1879

Comment 10

[Jorg K (CEST = GMT+2)]

Thanks for the comment. Can you tell from the screenshot in comment #7 whether the UI looks like it should? IOW, is that "storage actor" listed as it should be? If so, where's a good point to start to debug this? Of course, if the UI is already wrong, we'd switch off the test for now, but if it's there it, might be worth getting the test to work.

Comment 11

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

(In reply to Jorg K (GMT+2) from comment #10)

Thanks for the comment. Can you tell from the screenshot in comment #7 whether the UI looks like it should? IOW, is that "storage actor" listed as it should be?

The screenshot shows the expected UI (and the expected content), and so it seems to be working as expected.

If so, where's a good point to start to debug this? Of course, if the UI is already wrong, we'd switch off the test for now, but if it's there it, might be worth getting the test to work.

Sure, I'll be happy to help you debugging what is the underlying issue so that we can possibly keep the test enabled also for Thunderbird builds.

I would suggest to start by comparing (e.g. using a temporary console.log) which are the urls of the windows in this.windows from inside the StorageActors.default's form method, "while running the xpcshell test" vs. "while actually using the addon debugger toolbox on an extension in a real Thunderbird instance" (just opening the devtools storage panel as shown in the screenshot is enough).

The available hosts (which is what the failing assertion is checking) are derived from that set of windows, and we would expect to find at least a window with a "moz-extension://.../_generated_background_page.html" url in it (the one related to the target extension being inspected).

I think that (for reasons to be determined) the expected "moz-extension://..." window is there as expected when the devtools storage panel is being used interactively in a real Thunderbird instance (given that the screenshot shows a perfectly working storage panel), but it is not there while running these xpcshell unit tests.

Comment 12

[Jorg K (CEST = GMT+2)]

Hmm, I'll look into it. I'm a bit surprised about windows in an xpcshell-test. Aren't they all "text only", no windows?

Comment 13

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

(In reply to Jorg K (GMT+2) from comment #12)

Hmm, I'll look into it. I'm a bit surprised about windows in an xpcshell-test. Aren't they all "text only", no windows?

xpcshell doesn't provide any visible UI (besides its "text only" UI), nevertheless a window, a document etc. can still be created in an xpcshell test.

it is basically an headless test environment (but the xpcshell provides a "reduced XPCOM environment", it is not a complete "browser" as in mach mochitest --headless).

As a additional example, as part of the WebExtensions APIs test suites we have a number of xpcshell tests that are testing the WebExtensions content scripts (even if in the xpcshell environment there isn't the concept of tabs or anything related to the Firefox UI):
https://searchfox.org/mozilla-central/search?q=&path=toolkit%2Fcomponents%2Fextensions%2Ftest%2Fxpcshell%2Ftest_ext_content.

Comment 14

[Jorg K (CEST = GMT+2)]

Attachment: this-is-what-needs-to-be-switched-off-for-test-to-pass.patch

OK, with these two checks disabled, the whole thing runs through.
No idea why the pref doesn't work and what the thing about the extension-origin is.
I think it worth fixing and not switching off completely, or at least only switching off the parts that fail.

Further comments are welcome.

Comment 15

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

Thanks!
The fact that these are the only two tests that are failing on Thunderbird is pointing in a single clear direction:
the extensions run in the main process on Thunderbird builds.

Personally I'm ok with disabling these two test cases when the extensions runs in the main process, by passing a skip_if callback to the add_task call:

add_task({
  // ... (See Bug 1575183 for a rationale).
  skip_if: () => !WebExtensionPolicy.useRemoteWebExtensions,
}, async function test_name() {
  ...
});

Make sure to try to run the test file with --verify, I would expect that some of the other tests may fail intermittently because the timing of some of the steps is going to be different when the test extension runs in the main process.
(e.g. I suspect that test_panel_live_reload may be one of those that could more likely fail intermittently, if that it is the case lets skip it with the same skip_if callback).

---

Some additional details about why these two test cases fails when the extensions runs in the main process (non-oop mode):

test_extensionStorage_store_disabled_on_pref is not going to pass in a build that runs the extensions in the main process
(once the actor has been define, it is going to be there even if we flip the preference), lets just skip it with the skip_if callback described above.

As I suspected test_extension_origin_matches_debugger_target fails because the background page is not yet in the "storageActor.windows" (webExtensionTargetPrototype._searchForExtensionWindow 1 has an inline comment that explicitly mention that when the extensions are running in the main process the background page will not be immediately detected).

Everything work as expected in a real Thunderbird instance because the background page is detected from webExtensionTargetPrototype._shouldAddNewGlobalAsDebuggee when the threadActor is being attached by the toolbox
(this test is a unit test restricted to the "storage actors" and so it doesn't trigger that behavior).

As an alternative strategy we could likely make this xpcshell test file to trigger _shouldAddNewGlobalAsDebugee by attaching the threadActor,
which should make test_extension_origin_matches_debugger_target to pass
(but it is going to require some additional changes to a couple of the test helper methods defined in this test file,
I can give you more details about this if you prefer to look into that instead of skipping the test case).

Comment 16

[Jorg K (CEST = GMT+2)]

Hi Luca, thank's so much for your long explanation. If you don't mind, I'll go with the path of least resistance of disabling those tests.

Comment 17

[Jorg K (CEST = GMT+2)]

Attachment: Bug 1575183 - Skip two subtests of test_extension_storage_actor.js when run in non-oop extension mode.

Comment 18

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

(In reply to Jorg K (GMT+2) from comment #16)

Hi Luca, thank's so much for your long explanation. If you don't mind, I'll go with the path of least resistance of disabling those tests.

Sure, sounds good to me.

Comment 19

[Jorg K (CEST = GMT+2)]

Attachment: 1575183.patch

Same patch as in Phabibricator. Please land the "old fashioned" way. Commit message:
Bug 1575183 - Skip two subtests of test_extension_storage_actor.js when run in non-oop extension mode. r=rpl

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa7a7506f941
Skip two subtests of test_extension_storage_actor.js when run in non-oop extension mode. r=rpl

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/fa7a7506f941

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/dd647ef69680
Port bug 1542035: Add pref devtools.storage.extensionStorage.enabled. r=me

Comment 23

[Jorg K (CEST = GMT+2)]

I've added the pref for TB as well since FF has it here:
https://hg.mozilla.org/mozilla-central/rev/d7bfcc967c3e2cefd9179772c89a489e44ff0918#l1.12
That's better than having to go looking for it later.

Comment 24

[Intermittent Failures Robot]

1 failures in 5073 pushes (0.0 failures/push) were associated with this bug in the last 7 days.

Repository breakdown:

• mozilla-central: 1

Platform breakdown:

• windows10-64-ccov: 1

For more details, see:
https://treeherder.mozilla.org/intermittent-failures.html#/bugdetails?bug=1575183&startday=2019-08-19&endday=2019-08-25&tree=all