Closed
Bug 1034143
Opened 10 years ago
Closed 10 years ago
Make jar:http(s) url loads sandoxable — electrolyze child process use of nsDownloader
Categories
(Core :: Networking, defect)
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: blassey, Assigned: jld)
References
Details
Attachments
(4 files, 6 obsolete files)
13.71 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
6.28 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
4.53 KB,
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
16.33 KB,
patch
|
jld
:
review+
|
Details | Diff | Splinter Review |
This is causing trouble for our sandboxing efforts.
Fabrice, Doug said you may have some desire to keep this support around. Is that true?
Flags: needinfo?(fabrice)
Comment 1•10 years ago
|
||
Yes... We're working on making packaged apps better web citizens, using something like:
https://host.com/path/application.zip!//js/app.js to access resources. So what we do is that we create a channel for jar:https://host.com/path/application.zip!/js/app.js while keeping the original uri for uri resolution.
Flags: needinfo?(fabrice)
Comment 2•10 years ago
|
||
Actually I may change my mind because using jar:https may lead to additional complexity in the apps code.
ni? on me again and I'll come with a clear answer next week.
Flags: needinfo?(fabrice)
Comment 3•10 years ago
|
||
The final decision is that we need to keep support for these jar:http(s) urls. Should I morph this bug into "support proper remoting for jar:http" or do you prefer filing a new one?
Flags: needinfo?(fabrice)
Reporter | ||
Comment 4•10 years ago
|
||
go ahead and morph it
Comment 5•10 years ago
|
||
Jason, dougt told me you will work on this one ;)
Summary: remove support for jar:http:// → Make jar:http(s) url loads sandoxable
Comment 6•10 years ago
|
||
> The final decision is that we need to keep support for these jar:http(s) urls.
Fabrice: just out of curiosity, why do we need to keep jar:http?
> This is causing trouble for our sandboxing efforts.
OK, so what's the issue exactly? IIRC for jar:http URIs we 1) download the jar file, then 2) open it like a regular jar file. For app:// URIs (which are basically jar:// under the cover) we accomplished step #2 for e10s by having the parent open the jar file and then pass a file descriptor over IPDL to the child, which can then use the regular jar code with it. I assume we want the same thing here? That's definitely achievable. I'm not sure if there are any complications with cleanup (I don't know exactly how/when we delete the downloaded jar file).
Also, is there any reason why regular "jar://filename" works under e10s but "jar:http://" doesn't? I would assume that sandboxing is broken for the entire jar:// protocol (as long as child processes can't open files).
Flags: needinfo?(blassey.bugs)
Comment 7•10 years ago
|
||
(In reply to Jason Duell (:jduell) from comment #6)
> > The final decision is that we need to keep support for these jar:http(s) urls.
>
> Fabrice: just out of curiosity, why do we need to keep jar:http?
I'm using them in bug 1036275. If there's another, better solution instead of rewriting url to jar:http that would work too!
> > This is causing trouble for our sandboxing efforts.
>
> OK, so what's the issue exactly? IIRC for jar:http URIs we 1) download the
> jar file, then 2) open it like a regular jar file. For app:// URIs (which
> are basically jar:// under the cover) we accomplished step #2 for e10s by
> having the parent open the jar file and then pass a file descriptor over
> IPDL to the child, which can then use the regular jar code with it. I
> assume we want the same thing here? That's definitely achievable. I'm not
> sure if there are any complications with cleanup (I don't know exactly
> how/when we delete the downloaded jar file).
>
> Also, is there any reason why regular "jar://filename" works under e10s but
> "jar:http://" doesn't? I would assume that sandboxing is broken for the
> entire jar:// protocol (as long as child processes can't open files).
The issue with jar:http is that we create the http channel in the child, and then create a temp file for the zip in the child. But the sandbox prevents child process from creating files so that will fail. So it looks like we want jarChannel to remote the creation of the http channel, create the temp zip file in the parent and send back the descriptor to the child.
Assignee | ||
Comment 8•10 years ago
|
||
(In reply to Fabrice Desré [:fabrice] from comment #7)
> The issue with jar:http is that we create the http channel in the child, and
> then create a temp file for the zip in the child. But the sandbox prevents
> child process from creating files so that will fail. So it looks like we
> want jarChannel to remote the creation of the http channel, create the temp
> zip file in the parent and send back the descriptor to the child.
This is also my understanding. One of the complications was that, before comment #1, it wasn't clear if there were real use cases to justify this work.
(In reply to Jason Duell (:jduell) from comment #6)
> Also, is there any reason why regular "jar://filename" works under e10s but
> "jar:http://" doesn't? I would assume that sandboxing is broken for the
> entire jar:// protocol (as long as child processes can't open files).
jar:remoteopenfile:// should work, judging by modules/libjar/test/unit/head_ipc.js and the special case in nsJARChannel::LookupFile, from bug 815523. The rest might well be problems, but they didn't show up in a B2G `-u all` try run with unlink() disallowed. Also, supporting jars in existing local files should be relatively simple given that PRemoteOpenFile already handles it.
I think PRemoteOpenFile could be reused here, to provide the nsIFile wrapper (maybe managed by a different protocol with a name like PDownloadToTempFile and its own PRemoteOpenFile constructor), but: RemoteOpenFileChild handles only one call to OpenNSPRFileDesc, so if the zip/jar code would open the file multiple times, this would need some work — either duplicating the fd/handle in the child (which means breaking NSPR's abstractions a bit) or adjusting the IPC interface to allow multiple open requests.
Flags: needinfo?(blassey.bugs)
Assignee | ||
Updated•10 years ago
|
Assignee | ||
Updated•10 years ago
|
Assignee: nobody → jld
Assignee | ||
Comment 10•10 years ago
|
||
This almost works by using the existing PContent::OpenAnonymousTemporaryFile RPC, wrapping the result in an nsIFile (with a UUID instead of the "native path"), and having the nsDownloader run as normal in the child process.
But, similarly to bug 988816 comment #47, duplicating the fd in the child means that all the duplicates share a single file offset (which, per POSIX, is part of the “file description”). So the offset needs to be reset to 0 after downloading the file and before accessing it. Which is "fixed" by lseek'ing in the OpenNSPRFileDesc implementation… but this runs into the same problems as PRemoteOpenFile in terms of allowing races on the shared file offset.
Also, we'll have the same problem on Windows with DuplicateHandle — the documentation even specifically mentions file offset as an example of shared mutable state in handle-able objects.
Assignee | ||
Comment 11•10 years ago
|
||
…except there's no guarantee that libjar will always be the only content-process client of nsDownloader, so handing out silently nonconformant nsIFiles is not the best idea, even if it happens to be good enough for JarCache.
Assignee | ||
Comment 12•10 years ago
|
||
(In reply to Jed Davis [:jld] from comment #10)
> So the offset needs to be reset to 0 after downloading the file and before accessing it.
But only because libjar uses PR_Available64, which returns the distance from the current offset to the end of the file, to determine the file size. After that, it uses only memory-mapped I/O and never PR_Read, as far as I can tell from browsing the source.
The only problem is: we don't have a good way to assert that this fact about libjar will remain true (or even that's it's currently true in the first place).
(In reply to Jed Davis [:jld] from comment #11)
> …except there's no guarantee that libjar will always be the only
> content-process client of nsDownloader, so handing out silently
> nonconformant nsIFiles is not the best idea, even if it happens to be good
> enough for JarCache.
In that case, leave nsDownloader alone (or make it error out if it would create a temp file in a content process, to catch regressions in a more polite way than the sandbox), and have libjar supply the nsIFile.
Comment 13•10 years ago
|
||
Yeah I need to figure all this out in order to make bug 1036275 work, which is on my plate right now.
Medium-term, it might make sense to add a new IDL (nsIChildFile?) that implements only a subset of nsIFile (including providing a single-use open() only) and maybe have nsIFile inherit from it. That'd be cleaner logically than pretending we implement nsIFile things that we don't exactly.
Comment 14•10 years ago
|
||
Fabrice,
So the only jar:http:// case you care about is for http://...!// URIs, right? I think I may handle that case specially in a way that doesn't fix jar:http in general. Would that be OK?
Flags: needinfo?(fabrice)
Comment 15•10 years ago
|
||
(In reply to Jason Duell (:jduell) from comment #14)
> Fabrice,
>
> So the only jar:http:// case you care about is for http://...!// URIs,
> right? I think I may handle that case specially in a way that doesn't fix
> jar:http in general. Would that be OK?
Yes, that would be fine for me.
Flags: needinfo?(fabrice)
Assignee | ||
Comment 16•10 years ago
|
||
This patch seems to take care of jar:http: URLs (using PContent::OpenAnonymousTemporaryFile and the dup/DuplicateHandle approach to multiple opens).
https://tbpl.mozilla.org/?tree=Try&rev=9f4e7e07c79f
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=9f4e7e07c79f
It probably needs more comments, at least, before it's ready for a real review.
Given comment #14, do we still need it?
Attachment #8476727 -
Flags: feedback?(jduell.mcbugs)
Comment 17•10 years ago
|
||
Comment on attachment 8476727 [details] [diff] [review]
bug1034143-libjar-anon-wip0.diff
It's still not clear if we need this (or how soon) for desktop e10s (or B2G: desktop would seem more likely). The http://..!// stuff (bug 1036275) is looking like it will solve this issue differently.
Given bug 1043470 it looks like we've got some test cases on the child for jar:http://. But it's not clear if those are uses cases that happen in actual user browsing.
Honza, since you're doing bug 1036275 and that also touches JarChannel and nsDownloader, could you keep an eye on this and see if it makes sense to land this patch?
Attachment #8476727 -
Flags: feedback?(jduell.mcbugs) → feedback?(honzab.moz)
Comment 18•10 years ago
|
||
Comment on attachment 8476727 [details] [diff] [review]
bug1034143-libjar-anon-wip0.diff
Review of attachment 8476727 [details] [diff] [review]:
-----------------------------------------------------------------
I don't like the sync approach of opening the FD. You are in AsyncOpen, you can go async.
And something tells me this is crossing the path we walk in bug 1034143.
Have to think...
::: dom/ipc/RemoteAnonymousTemporaryFile.cpp
@@ +54,5 @@
> + }
> + rv = uuidgen->GenerateUUIDInPlace(&mUUID);
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + bool opened = aChild->SendOpenAnonymousTemporaryFile(&receivedFD);
hmm.. this is sync, bad...
@@ +121,5 @@
> +RemoteAnonymousTemporaryFile::Clone(nsIFile** file)
> +{
> + MOZ_ASSERT(mInited);
> + // This class is intended to be thread-safe.
> + NS_ADDREF((*file = this));
this is TODO, right?
Attachment #8476727 -
Flags: feedback?(honzab.moz) → feedback-
Assignee | ||
Comment 19•10 years ago
|
||
This is similar to bug 1036275, but the result doesn't have to pretend to be an HTTP request, which seems to simplify a lot of things. Also, any additional requirements or alternate formats or capabilities that might come out of the !// standardization process wouldn't apply here.
If the only potential jar:http: users we're concerned about are desktop extensions (or non-Firefox Gecko embedding?) it would be possible to disallow them / disable tests / etc. on B2G only, but that just pushes the problem down the road a little.
(In reply to Honza Bambas (:mayhemer) from comment #18)
> I don't like the sync approach of opening the FD. You are in AsyncOpen, you
> can go async.
I'll have to write more IPDL for that, but it's not hard. (The existing IPDL will remain, because the other uses of NS_OpenAnonymousTemporaryFile, in MediaCache, can't easily be made async.)
> @@ +121,5 @@
> > +RemoteAnonymousTemporaryFile::Clone(nsIFile** file)
> > +{
> > + MOZ_ASSERT(mInited);
> > + // This class is intended to be thread-safe.
> > + NS_ADDREF((*file = this));
>
> this is TODO, right?
It's been long enough since I wrote that that I'm not sure, but the idea is that the object doesn't really do anything that changes its state, so it can basically just use threadsafe refcounting and make Clone a no-op (+ addref).
Assignee | ||
Comment 20•10 years ago
|
||
Judging by bug 1036275 comment #49, that bug isn't going to be helpful here, but it's also not going to be a consumer of jar: URLs.
Which brings us back to the earlier question: do we actually use those URLs in content processes?
Assignee | ||
Comment 21•10 years ago
|
||
(In reply to Jed Davis [:jld] from comment #20)
> Which brings us back to the earlier question: do we actually use those URLs
> in content processes?
My answer: Maybe, but at this point it would be more effort to determine whether (or how much) that's the case outside the testsuite — and to fix the affected test cases to do something else — than to just make it work.
Assignee | ||
Comment 22•10 years ago
|
||
Now with a new IPC protocol for asynchronously opening an anonymous temporary file. I'm not as certain as I'd like to be about the rearrangement of nsJARChannel::AsyncOpen, but it passes try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ecbfc0cdd59
Attachment #8476727 -
Attachment is obsolete: true
Attachment #8553197 -
Flags: review?(honzab.moz)
Attachment #8553197 -
Flags: review?(bent.mozilla)
Assignee | ||
Comment 23•10 years ago
|
||
The changes to nsJARChannel might be clearer with the indentation changes ignored.
Comment 25•10 years ago
|
||
It's a good question, and I don't know the answer. Nothing has broken yet without it, but e10s desktop might at some point?
I can see the benefits of landing something while we've got people thinking about it, and I can also understand holding off on landing this unless/until we actually see a need for it. I'll leave that up to the reviewer (Honza) since you'll have a better idea how nice the code/solution is here.
Flags: needinfo?(jduell.mcbugs)
Comment 26•10 years ago
|
||
Brad seemed to think this was actively causing issues with sandboxing, which makes sense. Though we've got a lot of other things that are probably blocking that too. Brad, do you have a time frame for how soon (or if) this needs to get fixed?
Flags: needinfo?(blassey.bugs)
Comment on attachment 8553197 [details] [diff] [review]
bug1034143-libjar-anon-hg0.diff
Review of attachment 8553197 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/ipc/ContentChild.h
@@ +205,5 @@
> + virtual bool
> + DeallocPOpenAnonymousTemporaryFileChild(POpenAnonymousTemporaryFileChild* aActor) MOZ_OVERRIDE;
> +
> + nsresult
> + AsyncOpenAnonymousTemporaryFile(FileDescriptor* aFileDescPtr,
The lifetime constraints on aFileDescPtr are really not clear form this signature... Basically you have to pass in a pointer that is valid for the life of this async call, and the function will do nothing to help (e.g. holding aContinuation alive, for instance).
I think making a new interface for this is probably much better than expecting callers to get this right:
class AnonymousTemporaryFileCallback
{
NS_IMETHOD_(MozExternalRefCountType) AddRef() = 0;
NS_IMETHOD_(MozExternalRefCountType) Release() = 0;
virtual void Callback(FileDescriptor* aFd) = 0;
};
Then you can lose the complicated RunnableMethod stuff you need now.
::: dom/ipc/ContentParent.cpp
@@ +3413,5 @@
> +{
> + FileDescriptor fd;
> + // This is just an async version of OpenAnonymousTemporaryFile, so
> + // reuse that code instead of duplicating it:
> + return RecvOpenAnonymousTemporaryFile(&fd)
Oh, yeesh. This does main thread I/O. We can't add more of this :(
Attachment #8553197 -
Flags: review?(bent.mozilla) → review-
Reporter | ||
Comment 29•10 years ago
|
||
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #25)
> It's a good question, and I don't know the answer. Nothing has broken yet
> without it, but e10s desktop might at some point?
>
> I can see the benefits of landing something while we've got people thinking
> about it, and I can also understand holding off on landing this unless/until
> we actually see a need for it. I'll leave that up to the reviewer (Honza)
> since you'll have a better idea how nice the code/solution is here.
My understanding from Jed is that this is blocking the sandbox work for Linux, which means we'd like to get it sorted out in 38.
Flags: needinfo?(blassey.bugs)
Assignee | ||
Comment 30•10 years ago
|
||
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #29)
> My understanding from Jed is that this is blocking the sandbox work for
> Linux, which means we'd like to get it sorted out in 38.
Somewhat. If necessary I can always treat Necko as if it were a third-party library (e.g., http://keithp.com/blogs/chromium-dri3/ → https://crbug.com/415681), intercept the open() and unlink() syscalls, and try to emulate them in a way that works. Which is basically the same idea as this patch, but pushed down the stack a few layers (and doesn't help Windows or OS X if they want to tighten their sandboxes correspondingly).
My hope was that, since I basically already had a patch for this, I could just finish it instead of having to: write and land telemetry probes, wait to get data, figure out what to do about the existing test cases, and live with this strange corner case (jar URL *and* not-file inner URL *and* content process).
Comment 31•10 years ago
|
||
Comment on attachment 8553197 [details] [diff] [review]
bug1034143-libjar-anon-hg0.diff
Review of attachment 8553197 [details] [diff] [review]:
-----------------------------------------------------------------
r-
And concur the bent's comments...
::: dom/ipc/ContentChild.cpp
@@ +903,5 @@
>
> +POpenAnonymousTemporaryFileChild*
> +ContentChild::AllocPOpenAnonymousTemporaryFileChild()
> +{
> + MOZ_CRASH("Don't use this; allocate OpenAnonymousTemporaryFileChild directly.");
hm... weird.
::: dom/ipc/ContentParent.cpp
@@ +3413,5 @@
> +{
> + FileDescriptor fd;
> + // This is just an async version of OpenAnonymousTemporaryFile, so
> + // reuse that code instead of duplicating it:
> + return RecvOpenAnonymousTemporaryFile(&fd)
Yep, MT IO, no no...
::: dom/ipc/RemoteAnonymousTemporaryFile.cpp
@@ +28,5 @@
> + nsIRunnable* aOnReady)
> +{
> + MOZ_ASSERT(NS_IsMainThread());
> + MOZ_ASSERT(!mInited);
> + nsCOMPtr<nsIRunnable> onReady = aOnReady;
why exactly this dance via local comptr? for an argument?
@@ +29,5 @@
> +{
> + MOZ_ASSERT(NS_IsMainThread());
> + MOZ_ASSERT(!mInited);
> + nsCOMPtr<nsIRunnable> onReady = aOnReady;
> + nsresult rv = NS_ERROR_FAILURE;
why preinit?
@@ +36,5 @@
> + MOZ_ASSERT(uuidgen);
> + if (!uuidgen) {
> + return rv;
> + }
> + rv = uuidgen->GenerateUUIDInPlace(&mUUID);
Could we do this lazily?
@@ +41,5 @@
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + mOnReady.swap(onReady);
> + return aActor->AsyncOpenAnonymousTemporaryFile(&mFileDesc,
> + NS_NewRunnableMethod(this, &RemoteAnonymousTemporaryFile::Ready));
please, put the result of NS_NewRunnableMethod to a local comptr. in case of AsyncOpenAnonymousTemporaryFile failure it will leak otherwise.
::: modules/libjar/nsJARChannel.cpp
@@ +878,3 @@
> }
> + } else {
> + rv = NS_DispatchToMainThread(next);
why not just Run() ?
::: netwerk/base/src/nsDownloader.cpp
@@ +45,5 @@
> nsDownloader::OnStartRequest(nsIRequest *request, nsISupports *ctxt)
> {
> nsresult rv;
> if (!mLocation) {
> + MOZ_ASSERT(XRE_GetProcessType() != GeckoProcessType_Content);
== default?
Comment 33•10 years ago
|
||
Comment on attachment 8553197 [details] [diff] [review]
bug1034143-libjar-anon-hg0.diff
Review of attachment 8553197 [details] [diff] [review]:
-----------------------------------------------------------------
r-
And concur the bent's comments...
::: dom/ipc/ContentChild.cpp
@@ +903,5 @@
>
> +POpenAnonymousTemporaryFileChild*
> +ContentChild::AllocPOpenAnonymousTemporaryFileChild()
> +{
> + MOZ_CRASH("Don't use this; allocate OpenAnonymousTemporaryFileChild directly.");
hm... weird.
::: dom/ipc/ContentParent.cpp
@@ +3413,5 @@
> +{
> + FileDescriptor fd;
> + // This is just an async version of OpenAnonymousTemporaryFile, so
> + // reuse that code instead of duplicating it:
> + return RecvOpenAnonymousTemporaryFile(&fd)
Yep, MT IO, no no...
::: dom/ipc/RemoteAnonymousTemporaryFile.cpp
@@ +28,5 @@
> + nsIRunnable* aOnReady)
> +{
> + MOZ_ASSERT(NS_IsMainThread());
> + MOZ_ASSERT(!mInited);
> + nsCOMPtr<nsIRunnable> onReady = aOnReady;
why exactly this dance via local comptr? for an argument?
@@ +29,5 @@
> +{
> + MOZ_ASSERT(NS_IsMainThread());
> + MOZ_ASSERT(!mInited);
> + nsCOMPtr<nsIRunnable> onReady = aOnReady;
> + nsresult rv = NS_ERROR_FAILURE;
why preinit?
@@ +36,5 @@
> + MOZ_ASSERT(uuidgen);
> + if (!uuidgen) {
> + return rv;
> + }
> + rv = uuidgen->GenerateUUIDInPlace(&mUUID);
Could we do this lazily?
@@ +41,5 @@
> + NS_ENSURE_SUCCESS(rv, rv);
> +
> + mOnReady.swap(onReady);
> + return aActor->AsyncOpenAnonymousTemporaryFile(&mFileDesc,
> + NS_NewRunnableMethod(this, &RemoteAnonymousTemporaryFile::Ready));
please, put the result of NS_NewRunnableMethod to a local comptr. in case of AsyncOpenAnonymousTemporaryFile failure it will leak otherwise.
::: modules/libjar/nsJARChannel.cpp
@@ +878,3 @@
> }
> + } else {
> + rv = NS_DispatchToMainThread(next);
why not just Run() ?
::: netwerk/base/src/nsDownloader.cpp
@@ +45,5 @@
> nsDownloader::OnStartRequest(nsIRequest *request, nsISupports *ctxt)
> {
> nsresult rv;
> if (!mLocation) {
> + MOZ_ASSERT(XRE_GetProcessType() != GeckoProcessType_Content);
== default?
Attachment #8553197 -
Flags: review?(honzab.moz) → review-
Assignee | ||
Comment 34•10 years ago
|
||
After spending some quality time staring into the depths of libjar, I have a better idea: download remote jars into a memory buffer instead of a temporary file. And it's not just an idea: I've written a patch to implement it which mostly seems to work (and needs some cleanup).
Useful facts: the current nsJARChannel→nsDownloader path doesn't use a JarCache, so there's no need to pretend there's a file for its sake, and nsZipHandle already abstracts over jar files and nested jars unpacked/mapped in memory, so the support for an arbitrary chunk of memory is mostly already there.
But there is one thing that cares whether the nsJARChannel has a real file: XMLHttpRequest, which opens the file and memory-maps it a second time to implement bug 945152. The test cases for bug945152 and bug 1008126 currently require that that works for jar:http: (or, at least, that the application/mem-mapped pseudo-content-type is set). Those tests should probably be changed to use jar:file: or jar:remoteopenfile:, but I'd need to figure out how to make that work in the confines of the test automation.
Assignee | ||
Updated•10 years ago
|
Attachment #8553197 -
Attachment is obsolete: true
Assignee | ||
Updated•10 years ago
|
Attachment #8553199 -
Attachment is obsolete: true
Assignee | ||
Comment 35•10 years ago
|
||
Attachment #8568824 -
Flags: review?(bugs)
Assignee | ||
Comment 36•10 years ago
|
||
Attachment #8568826 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 37•10 years ago
|
||
Attachment #8568828 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 38•10 years ago
|
||
Attachment #8568829 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 39•10 years ago
|
||
Missing from these patches: reporting the memory consumed here. I could keep a global list of all of the buffers, but there's probably a better way — or maybe we don't need to care until/unless it comes back to me as a MemShrink bug.
Assignee | ||
Comment 40•10 years ago
|
||
Comment on attachment 8568824 [details] [diff] [review]
Step 1: Fix tests for bug 945152 and bug 1008126.
Review of attachment 8568824 [details] [diff] [review]:
-----------------------------------------------------------------
::: dom/base/test/mochitest.ini
@@ +157,5 @@
> file_bug869432.eventsource^headers^
> file_bug902350.html
> file_bug902350_frame.html
> file_bug907892.html
> file_bug945152.jar
The jar stays here in addition to being added in chrome.ini, because at least one other plain mochitest depends on it (from another directory), and I'm adding another such use in a later patch. Note to self: mention this in the commit comment to reduce confusion.
Assignee | ||
Comment 41•10 years ago
|
||
Comment on attachment 8568828 [details] [diff] [review]
Step 2b: Add the ability to read jar files from arbitrary memory.
Review of attachment 8568828 [details] [diff] [review]:
-----------------------------------------------------------------
::: modules/libjar/nsIZipReader.idl
@@ +78,5 @@
> */
> void openInner(in nsIZipReader zipReader, in AUTF8String zipEntry);
>
> /**
> + * Opens a zip file stored in memory; the file attribute will be null.
Stored in memory which is required to outlive the ZipReader and which should not be modified while the ZipReader exists; this should be stated explicitly, since we're not using a language whose type system can enforce it. (There may also be a better way to use XPIDL here.)
::: modules/libjar/nsZipArchive.cpp
@@ +247,5 @@
> +nsresult nsZipHandle::Init(const uint8_t* aData, uint32_t aLen,
> + nsZipHandle **aRet)
> +{
> + nsRefPtr<nsZipHandle> handle = new nsZipHandle();
> + if (!handle) {
This is copied from the existing nsZipHandle::Init()s, but… do we actually need it? Isn't this kind of allocation infallible?
Assignee | ||
Comment 42•10 years ago
|
||
Comment on attachment 8568826 [details] [diff] [review]
Step 2a: Add a class like nsDownloader but using memory instead of a file.
Review of attachment 8568826 [details] [diff] [review]:
-----------------------------------------------------------------
::: netwerk/base/MemoryDownloader.h
@@ +29,5 @@
> + NS_DECL_ISUPPORTS
> + NS_DECL_NSIREQUESTOBSERVER
> + NS_DECL_NSISTREAMLISTENER
> +
> + typedef mozilla::UniquePtr<FallibleTArray<uint8_t>> Data;
This is a pointer to a pointer, and it doesn't really need to be, although it does make it more difficult to accidentally copy (or alias/retain) the memory. (Also, there may be another type that works better than TArray here.)
Comment 43•10 years ago
|
||
Comment on attachment 8568826 [details] [diff] [review]
Step 2a: Add a class like nsDownloader but using memory instead of a file.
Review of attachment 8568826 [details] [diff] [review]:
-----------------------------------------------------------------
generally looks good
style for new files is 2 columns indent
so, on low memory devices this may actually open a gate to just kill the device with OOM. you need a review from some memory-shrink guy.
::: netwerk/base/MemoryDownloader.cpp
@@ +39,5 @@
> + Data data;
> + data.swap(mData);
> + mObserver->OnDownloadComplete(this, aRequest, aCtxt, aStatus,
> + mozilla::Move(data));
> + mObserver = nullptr;
maybe also swap mObserver before the callback call
maybe release the partial data on a failure?
@@ +53,5 @@
> + uint32_t* aWriteCount)
> +{
> + MemoryDownloader* self = static_cast<MemoryDownloader*>(aClosure);
> + if (!self->mData->AppendElements(aFromRawSegment, aCount)) {
> + return NS_ERROR_OUT_OF_MEMORY;
aToOffset must be added to aFromRawSegment (it's a pointer to the buffer where you actually are)
this error is not propagated to result of aInStr->ReadSegments. you need to set the error on the downloader and return it from OnDataAvailable. it will then propagate to aStatus in OnStopRequest and also stop the download.
I also think you should release the memory here ASAP on this error.
Attachment #8568826 -
Flags: review?(honzab.moz) → review-
Comment 44•10 years ago
|
||
Comment on attachment 8568828 [details] [diff] [review]
Step 2b: Add the ability to read jar files from arbitrary memory.
Review of attachment 8568828 [details] [diff] [review]:
-----------------------------------------------------------------
looks good, but I'm no expert to nsJAR.cpp. not sure I should review this. jduell might be a better choice here.
::: modules/libjar/nsIZipReader.idl
@@ +78,5 @@
> */
> void openInner(in nsIZipReader zipReader, in AUTF8String zipEntry);
>
> /**
> + * Opens a zip file stored in memory; the file attribute will be null.
write the comment you added to splinter into the IDL comment
memory lifetime is the most important information to express
::: modules/libjar/nsZipArchive.cpp
@@ +248,5 @@
> + nsZipHandle **aRet)
> +{
> + nsRefPtr<nsZipHandle> handle = new nsZipHandle();
> + if (!handle) {
> + return NS_ERROR_OUT_OF_MEMORY;
no need for this check
Attachment #8568828 -
Flags: review?(honzab.moz) → review+
Comment 45•10 years ago
|
||
Comment on attachment 8568829 [details] [diff] [review]
Step 3: Convert nsJARChannel from temporary files to temporary memory.
Review of attachment 8568829 [details] [diff] [review]:
-----------------------------------------------------------------
So, as I understand, currently we 1a) download to a temp file, 1b) map the complete file to memory via mmap 2) process it with the zip archiver to get a single entry 3) throw the temp file away (no caching)
With this patch we 1) download whole zip to memory (with chunk-size increments) 2) process 3) free (no caching)
So, not much of a difference... If the http caching is set well for the file we may go from the cache. And this just reduces to memory copying of a file between processes. Not optimal, but not worse than before.
r=honzab
::: modules/libjar/nsJARChannel.cpp
@@ +894,2 @@
> }
> + channel->AsyncOpen(downloader, nullptr);
we must check the result here..
Attachment #8568829 -
Flags: review?(honzab.moz) → review+
Updated•10 years ago
|
Attachment #8568824 -
Flags: review?(bugs) → review+
Assignee | ||
Comment 46•10 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #43)
> so, on low memory devices this may actually open a gate to just kill the
> device with OOM. you need a review from some memory-shrink guy.
It's allocated in the content process, so on B2G (or other e10s) that's what should get killed. In general this doesn't seem very different from content XHR'ing the jar file directly, or just allocating a large string or ArrayBuffer.
I also brought it up on #memshrink and discussed it briefly with njn; we've determined that, while it would be nice to have a memory reporter for this, it's not necessary.
Assignee | ||
Comment 47•10 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #43)
> Comment on attachment 8568826 [details] [diff] [review]
> > + if (!self->mData->AppendElements(aFromRawSegment, aCount)) {
> > + return NS_ERROR_OUT_OF_MEMORY;
>
> aToOffset must be added to aFromRawSegment (it's a pointer to the buffer
> where you actually are)
nsDownloader doesn't use aToOffset at all:
> return self->mSink->Write(fromRawSegment, count, writeCount);
Is that a bug in nsDownloader?
Flags: needinfo?(honzab.moz)
Comment 48•10 years ago
|
||
(In reply to Jed Davis [:jld] from comment #47)
> (In reply to Honza Bambas (:mayhemer) from comment #43)
> > Comment on attachment 8568826 [details] [diff] [review]
> > > + if (!self->mData->AppendElements(aFromRawSegment, aCount)) {
> > > + return NS_ERROR_OUT_OF_MEMORY;
> >
> > aToOffset must be added to aFromRawSegment (it's a pointer to the buffer
> > where you actually are)
>
> nsDownloader doesn't use aToOffset at all:
>
> > return self->mSink->Write(fromRawSegment, count, writeCount);
>
> Is that a bug in nsDownloader?
My bad! This is alright. aToOffset is offset to your buffer when you are using a static allocated array. It actually reflects the number of bytes so far read during the call of ReadSegments (writer can be called multiple times).
Hence, you are OK to go with the code as you have!
Flags: needinfo?(honzab.moz)
Assignee | ||
Comment 49•10 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #45)
> ::: modules/libjar/nsJARChannel.cpp
> @@ +894,2 @@
> > }
> > + channel->AsyncOpen(downloader, nullptr);
>
> we must check the result here..
It was ignoring the result like that before I touched it (my change to that line is just the indent level and variable name), but I agree that it looks like a potential bug and should be fixed.
Assignee | ||
Comment 50•10 years ago
|
||
Attachment #8568826 -
Attachment is obsolete: true
Attachment #8570813 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 51•10 years ago
|
||
With additional r? as suggested in comment #44.
Attachment #8568828 -
Attachment is obsolete: true
Attachment #8570816 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Comment 52•10 years ago
|
||
Carrying over r+ from comment #45.
Attachment #8568829 -
Attachment is obsolete: true
Attachment #8570819 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Attachment #8570819 -
Attachment description: Step 3: Convert nsJARChannel from temporary files to temporary memory. → Step 3: Convert nsJARChannel from temporary files to temporary memory. [v2]
Comment 53•10 years ago
|
||
Comment on attachment 8570813 [details] [diff] [review]
Step 2a: Add a class like nsDownloader but using memory instead of a file. [v2]
Review of attachment 8570813 [details] [diff] [review]:
-----------------------------------------------------------------
::: netwerk/base/MemoryDownloader.h
@@ +27,5 @@
> +{
> + public:
> + NS_DECL_ISUPPORTS
> + NS_DECL_NSIREQUESTOBSERVER
> + NS_DECL_NSISTREAMLISTENER
indent's weird
public: and private: has to be on column 0
no tabs (0x09, double check that)
Attachment #8570813 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 54•10 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #53)
> Comment on attachment 8570813 [details] [diff] [review]
> > + public:
> > + NS_DECL_ISUPPORTS
> > + NS_DECL_NSIREQUESTOBSERVER
> > + NS_DECL_NSISTREAMLISTENER
>
> indent's weird
> public: and private: has to be on column 0
> no tabs (0x09, double check that)
Absence of tabs confirmed. The weird indentation is what emacs did with the lack of semicolons after the NS_DECL macros (and which I fixed by hand while typing it the first time); thanks for catching it.
Comment 55•10 years ago
|
||
Comment on attachment 8570816 [details] [diff] [review]
Step 2b: Add the ability to read jar files from arbitrary memory. [v2]
Review of attachment 8570816 [details] [diff] [review]:
-----------------------------------------------------------------
::: modules/libjar/nsIZipReader.idl
@@ +92,2 @@
> * The file that represents the zip with which this zip reader was
> + * initialized. This may be null if there is no underlying file.
s/may be/will be/
Attachment #8570816 -
Flags: review?(jduell.mcbugs) → review+
Assignee | ||
Comment 56•10 years ago
|
||
Assignee | ||
Comment 57•10 years ago
|
||
(In reply to Jed Davis [:jld] from comment #56)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/879c78ef29ec
I forgot to amend the r= onto this one before pushing, and also managed to typo the bug number; sorry about that, and hopefully that doesn't break anything.
Comment 58•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/ddf4960cda9a
https://hg.mozilla.org/mozilla-central/rev/66d2a14f0fb0
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Assignee | ||
Comment 59•10 years ago
|
||
And the other two (yes, I typoed *two* of the bug numbers, it turns out) are:
https://hg.mozilla.org/mozilla-central/rev/879c78ef29ec
https://hg.mozilla.org/mozilla-central/rev/3c316731c170
Depends on: 1228077
You need to log in
before you can comment on or make changes to this bug.
Description
•