Closed
Bug 965724
Opened 10 years ago
Closed 10 years ago
Remote open() calls via IPDL for MediaCache
Categories
(Core :: Security, defect)
Tracking
()
RESOLVED
FIXED
mozilla33
People
(Reporter: arroway, Assigned: jld)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 1 obsolete file)
6.92 KB,
patch
|
roc
:
review+
|
Details | Diff | Splinter Review |
MediaCache and EncodedBufferCache open temporary cache files by calling NS_OpenAnonymousTemporaryFile(). When called from a content process, it should remote open() and unlink() operations to a parent process via IPDL and pass back the file descriptor to the content process (as suggested in bug 906996 comment 9 and bug 940863).
Assignee | ||
Comment 1•10 years ago
|
||
This is the hasty implementation of bug 906996 comment #9 that I wrote while investigating bug 930258. I'm not sure if it's doing things in the right places code-wise, but it seemed to work in my local testing. Try run: https://tbpl.mozilla.org/?tree=Try&rev=57db509ebc14
Attachment #8442564 -
Flags: feedback?(roc)
Comment on attachment 8442564 [details] [diff] [review] bug965724-remote-tempfiles-hg0.diff Review of attachment 8442564 [details] [diff] [review]: ----------------------------------------------------------------- This looks fine, but I think it would be a bit nicer to have NS_OpenAnonymousTemporaryFile be the universal entry point and have it implement the IPC stuff internally if there is a ContentChild.
Attachment #8442564 -
Flags: feedback?(roc) → feedback+
Assignee | ||
Comment 3•10 years ago
|
||
Now with the e10s-awareness moved into xpcom/io. And I think at this point it makes sense for me to steal this bug; hopefully that's not a problem. https://tbpl.mozilla.org/?tree=Try&rev=08bb04d973ce
Assignee: stephouillon → jld
Attachment #8442564 -
Attachment is obsolete: true
Attachment #8444559 -
Flags: review?(roc)
Comment on attachment 8444559 [details] [diff] [review] bug965724-remote-tempfiles-hg1.diff Review of attachment 8444559 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/ipc/ContentParent.cpp @@ +3649,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return false; > + } > + *aFD = FileDescriptor::PlatformHandleType(PR_FileDesc2NativeHandle(prfd)); > + PR_Close(prfd); Doesn't this close *aFD as well? I don't understand how we ensure the FD is kept open long enough to be received by the child but not leaked.
Attachment #8444559 -
Flags: review?(roc) → review-
Assignee | ||
Comment 5•10 years ago
|
||
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #4) > > + *aFD = FileDescriptor::PlatformHandleType(PR_FileDesc2NativeHandle(prfd)); > > + PR_Close(prfd); > > Doesn't this close *aFD as well? > > I don't understand how we ensure the FD is kept open long enough to be > received by the child but not leaked. The ipc::FileDescriptor constructor, which is called implicitly on the first line, duplicates the file descriptor/handle. So the PR_Close closes the one that was opened, but the FileDescriptor has its own copy (which it owns and closes in its dtor, after duplicating it again over IPC). I can add a comment and/or an explicit constructor call to make all that clearer. Also, because I'm feeling more paranoid now than I was before I asked for review: are there any existing tests that use MediaCache in a remote browser? If not, is there one that could be adapted?
Flags: needinfo?(roc)
Comment on attachment 8444559 [details] [diff] [review] bug965724-remote-tempfiles-hg1.diff Review of attachment 8444559 [details] [diff] [review]: ----------------------------------------------------------------- I'm pretty sure B2G tests catch this. ::: dom/ipc/ContentParent.cpp @@ +3649,5 @@ > + if (NS_WARN_IF(NS_FAILED(rv))) { > + return false; > + } > + *aFD = FileDescriptor::PlatformHandleType(PR_FileDesc2NativeHandle(prfd)); > + PR_Close(prfd); Alright, please add a comment explaining how this works.
Attachment #8444559 -
Flags: review- → review+
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(roc)
Assignee | ||
Comment 7•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9793892fc81
https://hg.mozilla.org/mozilla-central/rev/f9793892fc81
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
You need to log in
before you can comment on or make changes to this bug.
Description
•