[e10s] dom/devicestorage/test/test_dirs.html are disabled

RESOLVED FIXED in Firefox 46

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: drno, Assigned: mccr8)

Tracking

(Blocks: 2 bugs)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox46 fixed, firefox47 fixed)

Details

(Whiteboard: [DeviceStorageTest])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

4 years ago
As part of bug 1057558 we disabled the IPC test dom/devicestorage/test/test_dirs.html as it was causing perma-orange.

Someone needs to investigate why this test if failing and enable it once it is fixed.
(Reporter)

Comment 1

4 years ago
Jim as you seem to be the author of this test, could you please have a look why this test is failing as part of IPC?
Flags: needinfo?(nchen)
Sorry, I'm not knowledgeable about how device storage works differently with IPC or how to debug IPC tests. My guess would be SpecialPowers.pushPermissions is not doing what it should. Maybe somebody from Ateam can have a look.
Flags: needinfo?(nchen)
(Reporter)

Comment 3

4 years ago
Jonathan do you know someone who could help fixing this?
Flags: needinfo?(jgriffin)
Running with ./mach mochitest-plain --e10s dom/devicestorage/test/test_dirs.html , I get the same failures as with test_ipc.html:
9 INFO TEST-UNEXPECTED-FAIL | dom/devicestorage/test/test_dirs.html | Should have apps storage with permission - expected PASS
##### GetRootDirectoryForType('sdcard', '') failed #####
10 INFO TEST-UNEXPECTED-FAIL | dom/devicestorage/test/test_dirs.html | Should have sdcard storage - expected PASS
##### GetRootDirectoryForType('crashes', '') failed #####
11 INFO TEST-UNEXPECTED-FAIL | dom/devicestorage/test/test_dirs.html | Should have crashes storage - expected PASS
Created attachment 8589692 [details] [diff] [review]
1063569.diff

I tried something like this, but doesn't work.
This makes it at least clear that pushPermissions seems to work correctly, as testPermission shows that the 'webapps-manage' permission is correctly set.
Dave, could there be something wrong with navigator.getDeviceStorage() that it doesn't work in e10s?
Flags: needinfo?(dhylands)
I don't think this is a harness issue.  We have other e10s tests that work fine with SpecialPowers.pushPermissions, and Martijn's patch seems to exonerate that as well.  Will wait for Dave's reply before looking further.
Flags: needinfo?(jgriffin)
Hmmm. It looks like the tests are running in a child process, and then it calls:

  dirService->Get(NS_APP_USER_PROFILE_50_DIR, NS_GET_IID(nsIFile),
                  getter_AddRefs(sDirs->apps));

or

    NS_GetSpecialDirectory("UAppData", getter_AddRefs(sDirs->crashes));

sDirs->apps (or sDirs->crashes) comes back NULL.

B2G runs with e10s but takes different code paths, and it doesn't call either of those 2 functions.

In B2G we pass a list of volumes from the parent to the child when the child app starts. This happens through: ContentParent::ForwardKnownInfo

It calls SendVolumes (on B2G). For non-B2G we could perhaps have it send the profile directory and UAppData directory.
Flags: needinfo?(dhylands)
Component: Mochitest → DOM: Device Interfaces
Product: Testing → Core
Version: unspecified → Trunk
Whiteboard: [DeviceStorageTest]
(Assignee)

Updated

3 years ago
Blocks: 1218958
tracking-e10s: --- → ?
Summary: dom/devicestorage/test/test_dirs.html are disabled → [e10s] dom/devicestorage/test/test_dirs.html are disabled
(Assignee)

Comment 9

3 years ago
Created attachment 8686344 [details] [diff] [review]
Make most dom/devicestorage/ tests work with e10s.

These tests use devicestorage_setup() in devicestorage_common() to do
two things:

a) Set up some preferences. This just requires using pushPrefEnv().

b) Delete a special directory for testing device storage things, to
make sure it is empty. This requires running a chrome script and
waiting for it to finish.

Actually converting the individual tests just requires wrapping up the
rest of the test inside a giant function.
(Assignee)

Comment 10

3 years ago
Comment on attachment 8686344 [details] [diff] [review]
Make most dom/devicestorage/ tests work with e10s.

Sorry, wrong bug.
Attachment #8686344 - Attachment is obsolete: true

Updated

3 years ago
Blocks: 984139
tracking-e10s: ? → +
(Assignee)

Comment 11

3 years ago
This seems like it just works now.
Assignee: nobody → continuation
(Assignee)

Comment 12

3 years ago
Created attachment 8713811 [details] [diff] [review]
Enable devicestorage test_dirs.html.

Dave, does it sound reasonable to enable this? It looks like the concern that led this to be disabled involved test_ipc.html, which is gone now, and various storage testing things have been fixed.

This try run is a horror show but it looks like test_dirs is passing:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=85c4362ab798&selectedJob=16059206
Attachment #8713811 - Flags: review?(dhylands)
Comment on attachment 8713811 [details] [diff] [review]
Enable devicestorage test_dirs.html.

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

Seems fine to me.
Attachment #8713811 - Flags: review?(dhylands) → review+
(Assignee)

Updated

3 years ago
Attachment #8589692 - Attachment is obsolete: true

Comment 15

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2f144331a4bb
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47

Comment 16

3 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/1111cbd52899
status-firefox46: --- → fixed
You need to log in before you can comment on or make changes to this bug.