1274706 - Reduce message size for PExternalHelperAppChild::SendOnDataAvailable

Status: RESOLVED WORKSFORME

(Firefox :: PDF Viewer, defect, P3)

Description

[Andrew McCreight [:mccr8]]

This is a common source of large message fatal assertions seen with the signature in bug 1271102. It seems to be used to deal with downloaded files that are routed to some MIME handler (yes, the data goes from the parent to the child then to the parent via this protocol).

I was able to hit this code path by opening a PDF as usual in PDF.js, then click on the "Download" button, and then saving the file. For a 800KB-ish file, this resulted in a 1.1MB IPC message. It is easy to imagine how this could reach 128MB, if we're just sending the file up in a single message.

The easiest fix is hopefully to use bkelly's new IPCStream to transfer the nsIInputStream to the parent. A more complex fix might be to avoid routing all that data through the child, but I'm not going to look at that for now.

Example crash report:
  https://crash-stats.mozilla.com/report/index/73c2979a-95ec-40ff-9609-99a982160519

Comment 1

[Ben Kelly [:bkelly, not reviewing]]

Note, I wasn't really planning on uplifting IPCStream.  I suppose we could try though.

Comment 2

[Andrew McCreight [:mccr8]]

Yeah, I'm not sure how high priority this is. It shows up a lot with bug 1268616, but for some reason it isn't all that common on beta (with a different OOM signature).

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Fix the argument style for two ExternalHelperApp methods.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Use IPCStream for PExternalHelperApp. WIP

Comment 5

[Andrew McCreight [:mccr8]]

This compiles, but it doesn't seem to actually transmit any data. ExternalHelperAppParent::RecvOnDataAvailable calls into nsExternalAppHandler::OnDataAvailable, which calls into BackgroundFileSaverStreamListener::OnDataAvailable, which then does mPipeOutputStream->WriteFrom(aInputStream, aCount, &writeCount), the call succeeds, but it doesn't get any data out of it.

I'm not familiar with pipes or streams, so I'm not sure what is going wrong, except that I'm sure that this code just expects the data to be all there at once. It even has the comment "Since the pipe has an infinite buffer, we don't expect any write error to occur here". I guess I'll try to figure out how streams are supposed to work.

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

BackgroundFileSaverStreamListener should probably use NS_AsyncCopy() with the StreamtransportService as it's target.

Comment 7

[Ben Kelly [:bkelly, not reviewing]]

There might be an example of this in NS_CloneInputStream().

Comment 8

[Ben Kelly [:bkelly, not reviewing]]

Actually, I think this is a problem with the nsIStreamListener interface contract:

https://dxr.mozilla.org/mozilla-central/source/netwerk/base/nsIStreamListener.idl#18

It requires a set number of bytes to be read from the nsIInputStream *synchronously*.  This isn't really possible with a pipe oriented stream.

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Oh, I guess you can do the async copy in the actor method and only call the OnDocumentAvailable() method when it's done.

Comment 10

[Andrew McCreight [:mccr8]]

Bill's patch to make IPC messages not use contiguous memory will help fix the contiguous memory problem for this message, but it'll still be crummy to bloat up the parent process by the size of the entire file.

Comment 11

[Ben Kelly [:bkelly, not reviewing]]

Does our sandboxing allow write() calls to the file system?  I would have thought no.  I know this is definitely not supported for stuff managed by QuotaManager.

Comment 12

[Andrew McCreight [:mccr8]]

(In reply to Ben Kelly [:bkelly] from comment #12)
> Does our sandboxing allow write() calls to the file system?  I would have
> thought no.  I know this is definitely not supported for stuff managed by
> QuotaManager.

I think so. You just can't open files (at least, ideally...). See for instance  NS_OpenAnonymousTemporaryFile(). I'll check with a sandboxing person, after I figure out what this code is doing. Conceptually, this method is already happy piping whatever arbitrary garbage you send it into a file, so it doesn't make things worse to do it directly in the child.

Comment 13

[Andrew McCreight [:mccr8]]

Haik, from a sandboxing perspective, is it okay for the parent process to open a file and then pass it to the child, to have the child write stuff into it? Thanks.

Comment 14

[Haik Aftandilian [:haik]]

(In reply to Andrew McCreight [:mccr8] from comment #14)
> Haik, from a sandboxing perspective, is it okay for the parent process to
> open a file and then pass it to the child, to have the child write stuff
> into it? Thanks.

Long term we don't want the content process to be writing to the filesystem directly, but we're in the early stages of implementing those restrictions. The meta bug is 1196384.

Sandboxing is only enabled on Nightly and limited right now. On OS X, the content process can write to most locations that the OS permits the user to write to, but ~/Library is read/write protected with a few exceptions for some specific subdirectories including a temp directory. We're starting to fill in a wiki page with information on filesystem sandboxing here <https://wiki.mozilla.org/Security/Sandbox/Deny_Filesystem_Access>.

So you could have the parent process write to a file, then pass the path to that file for the child to read, (and vice versa) but to me it sounds like it would be better to tackle this with an improved IPC transfer so that we have less things to revert later.

Comment 15

[Andrew McCreight [:mccr8]]

Ah, I guess I was wrong! I'll keep trying with the IPC transfer method then.

Comment 16

[Andrew McCreight [:mccr8]]

jst pointed out that it would make more sense to just write to the temp file in the child process, rather than pipe all of this data to the parent. Maybe we could pass a file descriptor to the child, or maybe there is some temporary area the child can just write to.

Comment 17

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Fix the argument style for two ExternalHelperApp methods.

Comment 18

[Andrew McCreight [:mccr8]]

Attachment: part 2 - Use IPCStream for PExternalHelperApp.

Ben, does this make any sense? I have no idea what I'm doing with pipes. The whole thing is weird because I'm trying to mash together what seems like two kinds of asynchronous pipes.

Comment 19

[Andrew McCreight [:mccr8]]

Comment on attachment 8757447 [details] [diff] [review]
part 2 - Use IPCStream for PExternalHelperApp.

Oops that was supposed to be f?

Comment 20

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8757447 [details] [diff] [review]
part 2 - Use IPCStream for PExternalHelperApp.

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

::: uriloader/exthandler/ExternalHelperAppParent.cpp
@@ +162,5 @@
>    MOZ_ASSERT(mPending, "must be pending!");
>  
> +  nsCOMPtr<nsIInputStream> stream = DeserializeIPCStream(aIPCStream);
> +  nsCOMPtr<nsIAsyncInputStream> asyncStream = do_QueryInterface(stream);
> +  asyncStream->AsyncWait(this, 0, 0, nullptr);

You can almost use nsInputStreamPump here, which takes an nsIInputStream and writes to an nsIStreamListener, but it will also sense start/stop request calls.  Not quite what you want.

@@ +174,5 @@
>    MOZ_ASSERT(!mDiverted, "child forwarding callbacks after request was diverted");
>  
> +  // XXX Is there anything that guarantees that this won't be called
> +  // until after we're done streaming data to the parent? I can't see
> +  // how.

Yea, I think this is a problem.  There is also a similar problem with another OnDataAvailable() call coming in before the previous one completes.

What we really need is a class that lets you append nsIInputStream objects to a list.  It reads them sequentially and calls OnDataAvailable() on the provided nsIStreamListener.  It also lets you set "call OnStopRequest() after last stream is complete" so you can signal the end of the stream.

This would probably be worth having as a separate class.  Kind of like nsInputStreamPump.  Or maybe we can enhance nsInputStreamPump to support this use case.

What do you think?

Comment 21

[Andrew McCreight [:mccr8]]

> There is also a similar problem with another OnDataAvailable() call coming in before the previous one completes.

Doesn't IPC guarantee that if message A is sent before message B that message A will be received first?

Comment 22

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Andrew McCreight [:mccr8] from comment #21)
> > There is also a similar problem with another OnDataAvailable() call coming in before the previous one completes.
> 
> Doesn't IPC guarantee that if message A is sent before message B that
> message A will be received first?

Yes, but if you enter AsyncWait(), then you have returned from the first OnDataAvailable call.

Comment 23

[Andrew McCreight [:mccr8]]

Oh, right, I didn't think about the child sending multiple OnDataAvailable() calls. In my testing that never happened. I should should see if I could break up the data on the child by doing multiple ODA calls instead.

Comment 24

[Andrew McCreight [:mccr8]]

Bill, in bug 1127927, you changed nsContextMenu.js a little bit, so you might know the answer to this: Is that file run in the chrome process? I'm wondering because I'm trying to figure out if anybody besides pdf.js is calling extHelperAppSvc.doContent from the child process.

Comment 25

[Andrew McCreight [:mccr8]]

Assuming nsContextMenu.js is run in the content process, then I think this large call must be from pdf.js, as we don't seem to call doContent anywhere else (aside from browser-element, which I assume is not used in desktop). What PdfStreamConverter.jsm seems to do is set up an input-stream-channel which streams the original URL of the PDF file down into the parent process, then redirects it through a few layers of JS into a listener returned by doContent from external-helper-app-service, which then streams the file back up to the parent process.

It seems like the best fix to this issue would be to figure out how to get pdf.js to save something to disk without routing all the data through the child process. I'm not sure what the best way to do that is. ExternalHelperAppChild::OnStartRequest has some code that checks if the source stream is divertible, which I think makes it so the data is not routed through the child, but presumably the intermediate JS wrappers defeat this. Maybe the request that is passed could be made to implement nsIDivertableChannel? I can try looking into that.

Comment 26

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

The call in nsContextMenu.js will always be in the main process--never content. We do use CPOWs from that file, but this isn't one of them.

Comment 27

[Andrew McCreight [:mccr8]]

I tried figuring out how the nsIDivertibleChannel stuff works, but I couldn't wrap my head around all the streams and things. Maybe somebody else would have better luck with it. Though it would be good to double check how common this is to figure out if it is worth doing. But in the current state, I think anybody on win32 is going to have a tough time saving a large PDF file to disk.

Comment 29

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 24 weeks.

Comment 30

[BugBot [:suhaib / :marco/ :calixte]]

Closing because no crashes reported for 12 weeks.