Closed
Bug 1143934
Opened 10 years ago
Closed 10 years ago
[e10s] Disallow mozSetFileNameArray (and the file input value setter) in content-process privileged scripts
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla40
People
(Reporter: darktrojan, Assigned: jld)
References
(Blocks 2 open bugs)
Details
Attachments
(4 files)
144 bytes,
text/javascript
|
Details | |
23.60 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
1.88 KB,
patch
|
ttaubert
:
review+
|
Details | Diff | Splinter Review |
5.00 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
STR:
Load a form with a file input control.
Load the attached frame script.
Call messageManager.sendAsyncMessage("setfiles", ["/path/to/a/file"]). The value of the control should change.
Try to submit the form.
In a non-e10s browser, the form submits. In an e10s browser, a request appears in the console, but it is immediately aborted.
Updated•10 years ago
|
tracking-e10s:
--- → ?
Jed, did this change recently for sandboxing?
Flags: needinfo?(jld)
Assignee | ||
Comment 2•10 years ago
|
||
I haven't restricted mozSetFileNameArray (or the |value| setter, which should be more or less the same thing) — even in the absence of add-ons, there are still things we can't break written around the idea of the control having a “value” that's a filesystem path. (This includes SessionStore: bug 1122855.) The closest I got to it was bug 1068838, where I added a new method for directly setting the underlying sequence of File objects, and converted a bunch of tests to use that API and create their File objects in the chrome process instead.
Linux desktop e10s sandboxing is currently nonexistent, so it's not that either.
In any case, calling mozSetFileNameArray (and/or setting |value|) means reading an arbitrary file, so we should carefully consider whether we want to support it in e10s frame scripts.
Flags: needinfo?(jld)
Flags: needinfo?(wmccloskey)
Hmm, I'm not sure why this is failing then.
Ben, is there a way to send a DOM File object using the message manager? That would be the right way to do this. Then they could call mozSetFileArray in the frame script.
Flags: needinfo?(wmccloskey) → needinfo?(bent.mozilla)
Yeah, message manager should be able to send blobs/files already. We have tests for this kind of thing (http://mxr.mozilla.org/mozilla-central/source/dom/base/test/test_ipc_messagemanager_blob.html).
Flags: needinfo?(bent.mozilla)
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Bill McCloskey (:billm) from comment #3)
> Ben, is there a way to send a DOM File object using the message manager?
> That would be the right way to do this. Then they could call mozSetFileArray
> in the frame script.
Note that mozSetFileArray landed in 38 (see bug 1068838), so code that needs to support older versions would need to check for it and fall back to setting the filename.
Also, I ran into something that's probably the same bug: trying to use an e10s file submission form that was restored from SessionStore (see, as mentioned above, bug 1122855). The child process thinks it has a File, but this happens when trying to submit:
[Child 84786] WARNING: This file has not been opened (or could not be opened). Sending an invalid file descriptor to the other process!: file /home/jld/src/gecko-dev/netwerk/base/nsFileStreams.cpp, line 571
[Parent 84749] WARNING: Received a bad file descriptor index!: file /home/jld/src/gecko-dev/netwerk/base/nsFileStreams.cpp, line 611
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Jed Davis [:jld] from comment #5)
> [Child 84786] WARNING: This file has not been opened (or could not be
> opened). Sending an invalid file descriptor to the other process!: file
> /home/jld/src/gecko-dev/netwerk/base/nsFileStreams.cpp, line 571
Some debug-by-printf suggests that it's the "has not been opened" case.
Yeah, session restore will have to be smarter and re-send those FDs if this is a case we care about.
Assignee | ||
Comment 8•10 years ago
|
||
So it looks as if this might never have worked — using mozSetFileNameArray in a content process yields a File object that can be read with a FileReader but not submitted (or stored in IndexedDB). There are tests that would notice this, but they're marked as disabled for e10s, probably because they'd fail.
It might be better to just make mozSetFileNameArray throw if it's called in a content process, rather than doing something that doesn't really work.
Assignee | ||
Comment 9•10 years ago
|
||
(In reply to Jed Davis [:jld] {UTC-7} from comment #8)
> It might be better to just make mozSetFileNameArray throw if it's called in
> a content process, rather than doing something that doesn't really work.
Tests that break:
dom/html/test/test_bug143220.html — testing the file value getter; just needs a File with a path
dom/base/test/test_bug345339.html — tests that form value persists over reload; could use Blob
dom/html/test/forms/test_{max,min,pattern,required,step}_attribute.html — probably can use Blob?
So this looks doable.
Assignee | ||
Comment 10•10 years ago
|
||
Because mozSetFileNameArray has always been confusingly broken in frame scripts, and fixing it is hard to justify given that we'd want to remove it at some point (as a special case of removing frame script APIs that directly access the filesystem) anyway, I'm going to repurpose this bug for disallowing it in that context.
mozSetFileArray effectively replaces it, but it needs to be documented first, and the question raised by bug 1146116 about how mozSetFileArray interacts with cross-origin wrappers should be answered.
Assignee: nobody → jld
Depends on: 1146116
Summary: [e10s] Calling mozSetFileNameArray from frame script prevents form submission → [e10s] Disallow mozSetFileNameArray (and the file input value setter) in frame scripts
![]() |
||
Updated•10 years ago
|
Blocks: e10s-tests
Assignee | ||
Comment 11•10 years ago
|
||
(In reply to Jed Davis [:jld] {UTC-7} from comment #9)
> dom/html/test/test_bug143220.html — testing the file value getter; just
> needs a File with a path
Slight problem: trying to read the full path of a remote File/Blob on a MOZ_CHILD_PERMISSIONS build (i.e., on B2G) will kill the child process and assertion-fail the parent. This includes reading the value property of a file input element from a privileged context. See BlobParent::RecvGetFilePath (which does the check) and BlobChild::RemoteBlobImpl::GetMozFullPathInternal (which doesn't check before calling SendGetFilePath).
I'm not sure what *should* happen in that case. I can see why we'd want to apply the principle of least privilege to giving the child process the full path. But… it seems strange to have the file input's value getter throw an exception (or return an empty string) in chrome context when it would return something useful (the leaf name) in content context. On the other hand, having it return the full path in the chrome process and the leaf name in the content process strikes me as a way to get annoying bugs.
Assignee | ||
Comment 12•10 years ago
|
||
This also removes `skip-if = e10s` from the five tests that were so afflicted, including test_formSubmission.html.
Attachment #8586552 -
Flags: review?(bugs)
Assignee | ||
Comment 13•10 years ago
|
||
Attachment #8586553 -
Flags: review?(ttaubert)
Assignee | ||
Comment 14•10 years ago
|
||
I wonder if it would be worth making this post an error to the Web Console, with advice about what to do instead, in addition to throwing an exception. Right now it's just the exception, and that might not be sufficiently illuminating for add-on developers.
I'll update MDN appropriately before this lands in any case.
Attachment #8586563 -
Flags: review?(ehsan)
Comment 15•10 years ago
|
||
Comment on attachment 8586563 [details] [diff] [review]
Step 3: Disallow mozSetFileNameArray in content processes.
Review of attachment 8586563 [details] [diff] [review]:
-----------------------------------------------------------------
If you want to warn, you can use nsIDocument::WarnOnceAbout.
Attachment #8586563 -
Flags: review?(ehsan) → review+
Comment 16•10 years ago
|
||
Comment on attachment 8586563 [details] [diff] [review]
Step 3: Disallow mozSetFileNameArray in content processes.
This is odd. Frame script in inprocesstabchildglobal could use
mozSetFileNameArray, but not in tabchildglobal.
Comment 17•10 years ago
|
||
But perhaps that isn't too bad.
Comment 18•10 years ago
|
||
Comment on attachment 8586552 [details] [diff] [review]
Step 1: Fix assorted forms mochitests for e10s-compatibility.
rs+
Attachment #8586552 -
Flags: review?(bugs) → review+
Updated•10 years ago
|
Attachment #8586553 -
Flags: review?(ttaubert) → review+
Assignee | ||
Comment 19•10 years ago
|
||
I did a try run of this last week, with these three patches plus bug 1146116 rolled into one changeset: https://treeherder.mozilla.org/#/jobs?repo=try&revision=02a24d7d0d4e
Keywords: checkin-needed
Assignee | ||
Comment 20•10 years ago
|
||
(In reply to Olli Pettay [:smaug] (New review requests after Easter, please, Friday and Monday are holidays in Finland) from comment #16)
> This is odd. Frame script in inprocesstabchildglobal could use
> mozSetFileNameArray, but not in tabchildglobal.
That's a good point — the API isn't broken for the in-process case the way it is when out-of-process, but to minimize the confusion of add-on authors there should be at least a warning for the in-process case. I think this can be a followup, and it might belong in (or related to) bug 1150395.
Assignee | ||
Updated•10 years ago
|
Summary: [e10s] Disallow mozSetFileNameArray (and the file input value setter) in frame scripts → [e10s] Disallow mozSetFileNameArray (and the file input value setter) in content-process privileged scripts
Comment 21•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/d1f61a3f4fc3
https://hg.mozilla.org/integration/mozilla-inbound/rev/b82726a9882e
https://hg.mozilla.org/integration/mozilla-inbound/rev/2cd7fb610499
I won't lie, https://treeherder.mozilla.org/logviewer.html#?job_id=6027122&repo=try makes me nervous, though.
Flags: in-testsuite+
Keywords: checkin-needed
Comment 22•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/d1f61a3f4fc3
https://hg.mozilla.org/mozilla-central/rev/b82726a9882e
https://hg.mozilla.org/mozilla-central/rev/2cd7fb610499
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox40:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
Updated•6 years ago
|
Component: HTML: Form Submission → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•