Closed Bug 1222522 Opened 4 years ago Closed 4 years ago

Make dom/devicestorage/ tests work with e10s

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
e10s + ---
firefox45 --- fixed

People

(Reporter: mccr8, Assigned: mccr8)

References

(Blocks 1 open bug)

Details

(Whiteboard: [DeviceStorageTest])

Attachments

(2 files, 1 obsolete file)

These tests all fail. I see two major problems, in devicestorage_common.js. First, they use the directory service to "ensure that the directory we are writing into is empty". Second, it sets boolean preferences using setBoolPref rather than SpecialPowers.pushPrefEnv(). The latter is the immediate cause of the failures.
Blocks: e10s-tests
tracking-e10s: --- → +
Assignee: nobody → continuation
test_dirs.html has already been converted to be e10s-friendly, but the actual test fails. Bug 1063569 is already on file for this, so I'll leave it to that.
Whiteboard: [DeviceStorageTest]
This breaks dom/devicestorage/ipc/test_ipc.html presumably because it seems to set up some weird SpecialPowers shim, and that hasn't been updated to include something I'm using. Maybe with e10s tests enabled test_ipc can be removed?

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d84adfd07694
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.
Dave, can I disable dom/devicestorage/ipc/test_ipc.html if I enable all of the device storage tests under e10s? (Except for the two that are also disabled with test_ipc.html.)
Flags: needinfo?(dhylands)
My changes seem to have broken test_sanity.html on B2G:

495 INFO TEST-START | dom/devicestorage/test/test_sanity.html
[Parent 780] WARNING: waitpid failed pid:877 errno:10: file /builds/slave/b2g_try_emu_dep-00000000000000/build/gecko/ipc/chromium/src/base/process_util_posix.cc, line 267
############ ErrorPage.js
JavaScript error: chrome://specialpowers/content/SpecialPowersObserver.js, line 114: NS_ERROR_NOT_INITIALIZED: Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIMessageSender.sendAsyncMessage]

I'll have to figure out exactly what that means.
(In reply to Andrew McCreight [:mccr8] from comment #4)
> Dave, can I disable dom/devicestorage/ipc/test_ipc.html if I enable all of
> the device storage tests under e10s? (Except for the two that are also
> disabled with test_ipc.html.)

Seems like a reasonable tradeoff.

Is there something in particular about the ipc tests which is troublesome?
Flags: needinfo?(dhylands)
(In reply to Dave Hylands [:dhylands] from comment #6)
> Is there something in particular about the ipc tests which is troublesome?

It looks like test_ipc.html shims up the parent process part of the SpecialPowers interface. I added some uses of some additional SpecialPowers methods, so presumably the parent process parts of those would have to be implemented here, too. It isn't impossible, it just doesn't seem like a good use of time if we're already testing multiprocess device storage support on e10s and b2g.
Sure - makes sense. I think the whole ipc stuff was done before b2g supported e10s.
(In reply to Andrew McCreight [:mccr8] from comment #5)
> My changes seem to have broken test_sanity.html on B2G:

In a debug build I see:
###!!! ASSERTION: Security problem: Content process does not have `device-storage:videos-read'.  It will be killed.
'Error', file /builds/slave/b2g_try_emu-d_dep-000000000000/build/gecko/dom/ipc/AppProcessChecker.cpp, line 203

This is happening after all of the tests complete in test_sanity, so I guess there's some issue with how the prefs are cleaned up.
If you set the preference device.storage.prompt.testing then device storage will not try check permissions.

Also, IIRC I think that this file: https://dxr.mozilla.org/mozilla-central/source/testing/mochitest/manifest.webapp may be the one getting used for permissions when not running B2G.

I see that manifest doesn't include videos.
Thanks for the information. Oddly, the failures are B2G only, so the desktop permissions shouldn't be an issue.

My current theory is that this is related to how my patches changes how preferences are cleared so that we actually wait until they are cleared before we finish the test. Maybe we are tearing the page down and destroying some device storage structure, and it asks the parent if it can do something, and we fail if the parent gets that request before the pref clearing has propagated.
For B2G, I think that this manifest.webapp is used:
https://github.com/mozilla-b2g/gaia/blob/master/dev_apps/mochitest/manifest.webapp

which has music, pictures, and sdcard, but no videos.
with the following preferences:

device.storage.enabled  true
device.storage.prompt.testing true

I added some a print to GetRootDirectoryForFile, and tried all combinations of device.storage.testing and e10s and got the following results:

device.storage.testing == false, e10s enabled

> GetRootDirectoryForType (child): mStorageType 'pictures' mStorageName '' f '/home/dhylands/Pictures'
> ##### GetRootDirectoryForType('sdcard', '') failed #####
> GetRootDirectoryForType (child): mStorageType 'videos' mStorageName '' f '/home/dhylands/Videos'
> GetRootDirectoryForType (child): mStorageType 'music' mStorageName '' f '/home/dhylands/Music'
> ##### GetRootDirectoryForType('apps', '') failed #####
> ##### GetRootDirectoryForType('crashes', '') failed #####

device.storage.testing == false, e10s disabled

> GetRootDirectoryForType (parent): mStorageType 'pictures' mStorageName '' f '/home/dhylands/Pictures'
> GetRootDirectoryForType (parent): mStorageType 'sdcard' mStorageName '' f '/home/work/firefox/mozilla-central/obj-x86_64-unknown-linux-gnu/tmp/scratch_user/fake-sdcard'
> GetRootDirectoryForType (parent): mStorageType 'videos' mStorageName '' f '/home/dhylands/Videos'
> GetRootDirectoryForType (parent): mStorageType 'music' mStorageName '' f '/home/dhylands/Music'
> GetRootDirectoryForType (parent): mStorageType 'apps' mStorageName '' f '/home/work/firefox/mozilla-central/obj-x86_64-unknown-linux-gnu/tmp/scratch_user/webapps'
> GetRootDirectoryForType (parent): mStorageType 'crashes' mStorageName '' f '/home/dhylands/.mozilla/firefox/Crash Reports'

device.storage.testing == true, e10s enabled

> GetRootDirectoryForType (child): mStorageType 'pictures' mStorageName '' f '/tmp/device-storage-testing'
> GetRootDirectoryForType (child): mStorageType 'sdcard' mStorageName '' f '/tmp/device-storage-testing'
> GetRootDirectoryForType (child): mStorageType 'videos' mStorageName '' f '/tmp/device-storage-testing'
> GetRootDirectoryForType (child): mStorageType 'music' mStorageName '' f '/tmp/device-storage-testing'
> ##### GetRootDirectoryForType('apps', '') failed #####
> ##### GetRootDirectoryForType('crashes', '') failed #####

device.storage.testing == true, e10s disabled

> GetRootDirectoryForType (parent): mStorageType 'pictures' mStorageName '' f '/tmp/device-storage-testing'
> GetRootDirectoryForType (parent): mStorageType 'sdcard' mStorageName '' f '/tmp/device-storage-testing'
> GetRootDirectoryForType (parent): mStorageType 'videos' mStorageName '' f '/tmp/device-storage-testing'
> GetRootDirectoryForType (parent): mStorageType 'music' mStorageName '' f '/tmp/device-storage-testing'
> GetRootDirectoryForType (parent): mStorageType 'apps' mStorageName '' f '/home/work/firefox/mozilla-central/obj-x86_64-unknown-linux-gnu/tmp/scratch_user/webapps'
> GetRootDirectoryForType (parent): mStorageType 'crashes' mStorageName '' f '/home/dhylands/.mozilla/firefox/Crash Reports'
I actually have this working now, somehow:
  https://treeherder.mozilla.org/#/jobs?repo=try&revision=c09a0f0b59cf
The change I made compared to my earlier versions was to let the test harness do whatever it does to clear the preferences automatically, rather than explicitly doing it myself. I'm guessing that just makes us clear the preferences later, so they are still set when we're running whatever code was causing issues.
These tests use devicestorage_setup() in devicestorage_common() to do
two things:

a) Set up preferences 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 requires wrapping up the
rest of the test inside a giant function.

This patch also removes the non-e10s ipc tests, because the other
changes break them. They should not be needed any more, as with this
patch we will be running devicestorage tests on e10s and b2g.

b2g try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c09a0f0b59cf
L64 try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fda4ac8a7b95
Attachment #8686346 - Attachment is obsolete: true
Attachment #8689599 - Flags: review?(dhylands)
Comment on attachment 8689599 [details] [diff] [review]
part 1 - Make most dom/devicestorage/ tests work with e10s.

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

Very nice. I suspect that this may fix various intermittent failures we've been seeing.
Attachment #8689599 - Flags: review?(dhylands) → review+
Attachment #8689600 - Flags: review?(dhylands) → review+
https://hg.mozilla.org/mozilla-central/rev/90216304ec45
https://hg.mozilla.org/mozilla-central/rev/8095a58a897a
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1232506
You need to log in before you can comment on or make changes to this bug.