Closed Bug 1232506 Opened 4 years ago Closed 4 years ago

Make dom/devicestorage really work with e10s

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
e10s + ---
firefox46 --- fixed

People

(Reporter: dhylands, Assigned: dhylands)

References

Details

Attachments

(2 files, 1 obsolete file)

Bug 12222522 made the tests work with e10s.

This bug makes non-tests (i.e. real usage) work with e10s.
To test, set the following preferences (in firefox about:config)

> device.storage.enabled true
> device.storage.testing false
> device.storage.prompt.testing true

Under linux, the directories ~/Pictures, ./obj-x86_64-unknown-linux-gnu/tmp/scratch_user/fake-sdcard, ~/Videos, ~/Music should be enumerated (assuming you're using ./mach run)

If you also set

> device.storage.overrideRootDir abs-path-to-some-dir

then all of the storage areas should come from the same tree.

Both of the above should work with and without e10s enabled.
Attachment #8698305 - Flags: review?(alchen)
Comment on attachment 8698305 [details] [diff] [review]
Forward various directories to child when using e10s

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

It looks fine.
Attachment #8698305 - Flags: review?(alchen) → review+
we get failures in xperf which measures files accessed during startup.  We have a series of files which were accessed and we were not expecting them:

TEST-UNEXPECTED-FAIL : xperf: File 'c:\users\public\desktop.ini' was accessed and we were not expecting it. DiskReadCount: 24, DiskWriteCount: 0, DiskReadBytes: 4176, DiskWriteBytes: 0
TEST-UNEXPECTED-FAIL : xperf: File 'c:\users\cltbld\music\desktop.ini' was accessed and we were not expecting it. DiskReadCount: 2, DiskWriteCount: 0, DiskReadBytes: 1012, DiskWriteBytes: 0
TEST-UNEXPECTED-FAIL : xperf: File 'c:\users\cltbld\pictures\desktop.ini' was accessed and we were not expecting it. DiskReadCount: 2, DiskWriteCount: 0, DiskReadBytes: 1012, DiskWriteBytes: 0
TEST-UNEXPECTED-FAIL : xperf: File 'c:\users\cltbld\appdata\roaming\microsoft\windows\libraries\pictures.library-ms' was accessed and we were not expecting it. DiskReadCount: 2, DiskWriteCount: 0, DiskReadBytes: 8192, DiskWriteBytes: 0
TEST-UNEXPECTED-FAIL : xperf: File 'c:\users\cltbld\appdata\roaming\microsoft\windows\libraries\videos.library-ms' was accessed and we were not expecting it. DiskReadCount: 2, DiskWriteCount: 0, DiskReadBytes: 8192, DiskWriteBytes: 0
TEST-UNEXPECTED-FAIL : xperf: File 'c:\users\public\pictures\desktop.ini' was accessed and we were not expecting it. DiskReadCount: 2, DiskWriteCount: 0, DiskReadBytes: 764, DiskWriteBytes: 0
TEST-UNEXPECTED-FAIL : xperf: File 'c:\users\cltbld\videos\desktop.ini' was accessed and we were not expecting it. DiskReadCount: 2, DiskWriteCount: 0, DiskReadBytes: 1012, DiskWriteBytes: 0
TEST-UNEXPECTED-FAIL : xperf: File 'c:\users\public\videos\desktop.ini' was accessed and we were not expecting it. DiskReadCount: 2, DiskWriteCount: 0, DiskReadBytes: 764, DiskWriteBytes: 0
TEST-UNEXPECTED-FAIL : xperf: File 'c:\users\public\music\desktop.ini' was accessed and we were not expecting it. DiskReadCount: 2, DiskWriteCount: 0, DiskReadBytes: 764, DiskWriteBytes: 0
TEST-UNEXPECTED-FAIL : xperf: File 'c:\users\cltbld\appdata\roaming\microsoft\windows\libraries\desktop.ini' was accessed and we were not expecting it. DiskReadCount: 2, DiskWriteCount: 0, DiskReadBytes: 552, DiskWriteBytes: 0
TEST-UNEXPECTED-FAIL : xperf: File 'c:\users\cltbld\appdata\roaming\microsoft\windows\libraries\music.library-ms' was accessed and we were not expecting it. DiskReadCount: 2, DiskWriteCount: 0, DiskReadBytes: 8192, DiskWriteBytes: 0


the patch that landed looks to access videos/pictures/music on the home directory- which matches what is seen here.  Is this intended to access these files?
Flags: needinfo?(dhylands)
(In reply to Joel Maher (:jmaher) from comment #5)
> we get failures in xperf which measures files accessed during startup.  We
> have a series of files which were accessed and we were not expecting them:
>
...snip... 
> the patch that landed looks to access videos/pictures/music on the home
> directory- which matches what is seen here.  Is this intended to access
> these files?

It needs to access them sometime between startup and before the first e10s process is launched. The e10s child processes can't access the directory service (or at least can't resolve all of the needed directories).

So what the patch does is to get the root directories that device storage needs in the parent and passes this information to child processes so that the child process doesn't need to go through directory services.

The files could be resolved anytime before the first child process is launched. So if there is a more appropriate place that this can be done, I'd be happy to move it there.
Flags: needinfo?(dhylands)
this sounds like we are expecting to access these files, if that is the case we can always add them to the whitelist:
https://dxr.mozilla.org/mozilla-central/source/testing/talos/talos/xtalos/xperf_whitelist.json

I would like to ask :aklotz to chime in here in case there is a better place to do this work, or something else I am overlooking.
Flags: needinfo?(aklotz)
See Also: → 1234109
backed this out for perma failures bug 1234109
Can we do this on a background thread and then force a sync up when we're ready to launch the content process?
Flags: needinfo?(aklotz)
(In reply to Aaron Klotz [:aklotz] (please use needinfo) from comment #11)
> Can we do this on a background thread and then force a sync up when we're
> ready to launch the content process?

I need to determine if the accesses are happening on the parent side or the child side.

My suspicion is that they're happening on the child side, since I'm pretty sure that the accesses on the parent side haven't changed.

If its the case that the accesses are happening on the child side, then I'm pretty sure I can defer them until the child side actually creates a device storage object.

The parent passes the paths to the child, and right now the child is turning those paths into file objects really early, and I think I can change it to just store the paths, and defer the file accesses.
As a warning, this also seems to regress ts_paint by about 2.5% on e10s (just did a bunch of retriggers to confirm):

https://treeherder.allizom.org/perf.html#/compare?originalProject=mozilla-inbound&originalRevision=9e1c3b1f311c&newProject=mozilla-inbound&newRevision=0f2a62a45b00&framework=1&filter=ts_paint%20windows7&showOnlyConfident=1

We need to either fix this issue or decide to accept the regression before this lands.

If you need help running the Talos test, feel free to ask questions in #perf. :)
more random data for the ts_paint regression, this is windows7 only and here is the try syntax:
try: -b o -p win32 -u none -t other-e10s
Carrying r+

I reworked this so that instead of passing the file info at process start (which penalizes all child processes), when a child uses devices storage it will make a sync IPC call to the parent to get the needed path information. It needs to be synchronous since navigator.getDeviceStorage is synchronous.

With this patch, only device storage users will be slowed down for this single synchronous IPC call (it will only be made once for a given child process).

This is the try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=48031db5bb00

I manually confirmed that devices storage seems to be working properly by running the steps from comment 1. I realized that there was one step missing from comment 1, namely that you need to create a fake-sdcard directory in the profile (for me, using mach run, this was /home/work/firefox/mozilla-inbound/obj-x86_64-unknown-linux-gnu/tmp/scratch_user/fake-sdcard)
Attachment #8698305 - Attachment is obsolete: true
Attachment #8703224 - Flags: review+
Attachment #8698304 - Attachment description: Test file used for verifying devicestorage is working with e10s → Test file used for verifying devicestorage is working with e10s (not intended to be checked in)
https://hg.mozilla.org/mozilla-central/rev/471e252b7b30
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.