If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Remote open() calls via IPDL for MediaCache

RESOLVED FIXED in mozilla33

Status

()

Core
Security
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: arroway, Assigned: jld)

Tracking

(Blocks: 1 bug)

unspecified
mozilla33
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

4 years ago
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

3 years ago
Created attachment 8442564 [details] [diff] [review]
bug965724-remote-tempfiles-hg0.diff

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

3 years ago
Created attachment 8444559 [details] [diff] [review]
bug965724-remote-tempfiles-hg1.diff

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

3 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

3 years ago
Flags: needinfo?(roc)
(Assignee)

Comment 7

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f9793892fc81
https://hg.mozilla.org/mozilla-central/rev/f9793892fc81
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla33
(Assignee)

Updated

3 years ago
Duplicate of this bug: 995071
(Assignee)

Updated

3 years ago
Blocks: 1149313
Depends on: 1146416
You need to log in before you can comment on or make changes to this bug.