Closed Bug 1068838 Opened 6 years ago Closed 6 years ago

[e10s] Make content mochitests stop opening files with the <input type="file"> value setter

Categories

(Core :: DOM: Core & HTML, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla38
Tracking Status
e10s later ---

People

(Reporter: billm, Assigned: jld)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

The <input type=file> code opens the file in the content process before sending it with the POST request. We'll need to fix that to make sandboxing work. At that time, we can remove the code added in bug 1067351.
I've run into something similar in the context of the File/Blob API and I'm not sure whether it's a duplicate of this bug.

Specifically, if I interact normally with the file input element, the resulting File is backed by a BlobChild::RemoteBlobImpl (if I understand the code correctly) and everything is fine; but if the element is manipulated by privileged script setting the value attribute to a path, which we do in a number of mochitests, then the File is such that the content process tries to access it directly.  Also, killing the browser and restoring the session re-creates the input element set to a non-remote File, even if it was a remote File before.

The tests that depend on using SpecialPowers to set |value| are disabled on desktop e10s, but they're being run on B2G, which runs into bug 930258.
Flags: needinfo?(wmccloskey)
I'm trying to remember what I meant by "The <input type=file> code opens the file in the content process before sending it with the POST request." in comment 0. That was true in an early patch in bug 910384, but I guess it's not true anymore.

Regarding comment 1, I guess mozSetFileNameArray is the problematic function? Could we just replace it with a function like mozSetFileBlobs, where you would pass in blobs? Then the test code would obtain a blob through SpecialPowers somehow. The session store code would have to get a blob using the message manager. I'm not sure how that would work, but I could handle fixing that if you take the rest, Jed.

I can also think of a more brute-force approach that would look like this:
1. Have a function that would run in the parent process called exposeFileToChild or something. It would just add a file path to a list stored in ContentParent.
2. Have an IPC call on PContent to ask for a blob given a filename. The parent would allow any path in the path list from #1.
3. When run in the child process, HTMLInputElement::MozSetFileNameArray would use this call to get the blob it needs.

I'd prefer the first approach if we can make it work. What do you think, Jed?
Flags: needinfo?(wmccloskey)
We have PRemoteOpenFile for this purpose already, kinda. We'd just need to make it a little safer and then figure out how to stuff an fd into a Blob object...
(In reply to Bill McCloskey (:billm) from comment #2)
> Regarding comment 1, I guess mozSetFileNameArray is the problematic
> function? Could we just replace it with a function like mozSetFileBlobs,
> where you would pass in blobs? Then the test code would obtain a blob
> through SpecialPowers somehow. The session store code would have to get a
> blob using the message manager.

That looks like the right way to do this.  It seems like a good idea to keep the path used by tests (and crash recovery!) as close to what's used by actual users as possible.  I'll need to go back and stare at the code again to get an idea of how much work this would be….

Also, to answer my own question: POSTing a file appears to work the same way as using the client-side File API — if the file picker is used normally then the file is sent without being opened directly by the content process, which is the desired behavior.
Our current code causes the test in bug 1118504 to blow up on e10s and android. Since that's a use case we want to support (storing a file in IndexedDB) we should make sure we fix this before turning on e10s.

It looks like the necessary plumbing is mostly already in place, we'd just need to expose a new |[ChromeOnly] void mozSetFiles(sequence<File>);| on the HTMLInputElement.webidl interface. And then a SpecialPowers function to get a file-backed File object from a given path...
(In reply to Bill McCloskey (:billm) from comment #2)
> Then the test code would obtain a blob through SpecialPowers somehow.

I tried to see if I could make this work.  I can get the File constructor into the frame script with Cu.importGlobalProperties (h/t :khuey), but when it's sent to the child it winds up as an instance of the File whose global is the ContentFrameMessageManager, not the Window which is the test script's global.  And then the WebIDL binding rejects it for not being an instance of File.
Assignee: nobody → jld
> I tried to see if I could make this work.  I can get the File constructor
> into the frame script with Cu.importGlobalProperties (h/t :khuey), but when
> it's sent to the child it winds up as an instance of the File whose global
> is the ContentFrameMessageManager, not the Window which is the test script's
> global.

Which is not actually the problem.  The problem is that it's a Proxy (specifically, a SpecialPowers chrome wrapper) instead of a File.  WebIDL doesn't actually case which File constructor it is.  This suggests the possibility of having SP.loadChromeScript not auto-wrap return messages.
Blocks: 930258
See Also: → 1121722
Component: DOM: Content Processes → DOM: Core & HTML
Summary: [e10s] Will need to fix file picker code to work with sandboxing → [e10s] Make content mochitests stop opening files with the <input type="file"> value setter
The “Step 2” patch indents a lot of test_fileapi.html to make it the continuation of an async operation; this patch may make that easier to review.
Comment on attachment 8551998 [details] [diff] [review]
Step 2: Fix tests that open files in the content process this way.

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

I mainly skimmed this since the pattern seemed good. If you want a more thorough review, please attach a -w version of this patch.

::: dom/base/test/bug403852_fileOpener.js
@@ +8,5 @@
> +testFile.append("prefs.js");
> +
> +addMessageListener("file.open", function () {
> +  sendAsyncMessage("file.opened", {
> +    file: new File(testFile),

I'm new psyched to use the |new File(nsIFile)| constructor. We're trying to deprecate it since it often results in sync IO happening. Though I'm not sure what else you could use here.

Ideally we'd have some form of asynchronous File factory function. Sadly I don't think we have that currently, so unless you know of one feel free to leave this as-is.

Same applies to a few other places in this patch.
Attachment #8551998 - Flags: review?(jonas) → review+
Comment on attachment 8552713 [details] [diff] [review]
Step 2 with whitespace changes suppressed

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

Looks good
I don't like the main-thread I/O either, but I don't think there's an async way to do that yet.  At least now the main test scripts are async and it's (relatively) easy to change the chrome scripts if an async API is added.
https://hg.mozilla.org/mozilla-central/rev/f9131ff0a89b
https://hg.mozilla.org/mozilla-central/rev/a0f9be26c71e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
See Also: → 1146116
You need to log in before you can comment on or make changes to this bug.