Closed Bug 1575183 Opened 2 years ago Closed 2 years ago

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

Categories

(Thunderbird :: General, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 70.0

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

(Regression)

Details

(Keywords: regression)

Attachments

(3 files, 3 obsolete files)

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.

Attached patch 1575183-extensionStorage.patch (obsolete) — Splinter Review

I should rebuild and try it ;-)

Assignee: nobody → jorgk
Attachment #9086623 - Flags: review?(geoff)
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.
Flags: needinfo?(geoff)
Flags: needinfo?(bdanforth)
Attachment #9086623 - Flags: review?(geoff)
Attached file 1575183.txt (obsolete) —

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

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!

Flags: needinfo?(bdanforth)

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

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

Flags: needinfo?(geoff)

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?

Flags: needinfo?(mkmelin+mozilla)

(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:

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.

Flags: needinfo?(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.

Flags: needinfo?(lgreco)

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?

(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.

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.

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).

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.

Flags: needinfo?(mkmelin+mozilla)

(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.

Attachment #9087654 - Attachment description: Bug 1575183 - Skip two subtests of test_extension_storage_actor.js when run in the main process. → Bug 1575183 - Skip two subtests of test_extension_storage_actor.js when run in non-oop extension mode.
Attachment #9087508 - Attachment is obsolete: true
Attachment #9086623 - Attachment is obsolete: true
Attachment #9086633 - Attachment is obsolete: true
Attached patch 1575183.patchSplinter Review

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

Attachment #9087691 - Flags: review+
Keywords: checkin-needed

Pushed by btara@mozilla.com:
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

Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 70.0
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/dd647ef69680
Port bug 1542035: Add pref devtools.storage.extensionStorage.enabled. r=me

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.

Keywords: regression
You need to log in before you can comment on or make changes to this bug.