Reduce message size for PExternalHelperAppChild::SendOnDataAvailable

NEW
Unassigned

Status

()

Firefox
PDF Viewer
P1
normal
2 years ago
12 days ago

People

(Reporter: mccr8, Unassigned)

Tracking

(Blocks: 1 bug)

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox46 wontfix, firefox47 affected, firefox48 affected, firefox49 affected)

Details

(Whiteboard: [pdfjs-c-integration], crash signature)

Attachments

(4 obsolete attachments)

(Reporter)

Description

2 years ago
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
(Reporter)

Updated

2 years ago
status-firefox46: --- → wontfix
status-firefox47: --- → affected
status-firefox48: --- → affected

Comment 1

2 years ago
Note, I wasn't really planning on uplifting IPCStream.  I suppose we could try though.
(Reporter)

Comment 2

2 years ago
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).
(Reporter)

Comment 3

2 years ago
Created attachment 8755055 [details] [diff] [review]
part 1 - Fix the argument style for two ExternalHelperApp methods.
(Reporter)

Comment 4

2 years ago
Created attachment 8755056 [details] [diff] [review]
part 2 - Use IPCStream for PExternalHelperApp. WIP
(Reporter)

Comment 5

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

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

Comment 7

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

Comment 8

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

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

Updated

2 years ago
Blocks: 1259512
tracking-e10s: ? → +
Priority: -- → P1
(Reporter)

Comment 10

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

2 years ago
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.
(Reporter)

Comment 12

2 years ago
(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.
(Reporter)

Comment 13

2 years ago
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.
Flags: needinfo?(haftandilian)
(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.
Flags: needinfo?(haftandilian)
(Reporter)

Comment 15

2 years ago
Ah, I guess I was wrong! I'll keep trying with the IPC transfer method then.
(Reporter)

Comment 16

2 years ago
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.
(Reporter)

Comment 17

2 years ago
Created attachment 8757446 [details] [diff] [review]
part 1 - Fix the argument style for two ExternalHelperApp methods.
(Reporter)

Comment 18

2 years ago
Created attachment 8757447 [details] [diff] [review]
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.
Attachment #8757447 - Flags: review?(bkelly)
(Reporter)

Updated

2 years ago
Attachment #8755055 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8755056 - Attachment is obsolete: true
(Reporter)

Comment 19

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

Oops that was supposed to be f?
Attachment #8757447 - Flags: review?(bkelly) → feedback?(bkelly)

Comment 20

2 years ago
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?
Attachment #8757447 - Flags: feedback?(bkelly)
(Reporter)

Comment 21

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

2 years ago
(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.
(Reporter)

Comment 23

2 years ago
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.
(Reporter)

Comment 24

2 years ago
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.
Flags: needinfo?(wmccloskey)
(Reporter)

Comment 25

2 years ago
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.
(Reporter)

Updated

2 years ago
Attachment #8757446 - Attachment is obsolete: true
(Reporter)

Updated

2 years ago
Attachment #8757447 - Attachment is obsolete: true
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.
Flags: needinfo?(wmccloskey)
(Reporter)

Updated

2 years ago
Component: File Handling → PDF Viewer
Product: Core → Firefox
(Reporter)

Updated

2 years ago
No longer blocks: 1259512

Updated

2 years ago
Whiteboard: [pdfjs-c-integration]

Updated

2 years ago
Whiteboard: [pdfjs-c-integration] → [pdfjs-c-integration][e10st?]

Updated

2 years ago
Assignee: continuation → nobody
Whiteboard: [pdfjs-c-integration][e10st?] → [pdfjs-c-integration]
(Reporter)

Comment 27

2 years ago
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.
Crash Signature: [@ mozilla::ipc::ProcessLink::SendMessageW | IPC_Message_Name=PExternalHelperApp::Msg_OnDataAvailable ]
(Reporter)

Updated

12 days ago
Duplicate of this bug: 1391176
(Reporter)

Updated

12 days ago
See Also: → bug 1437114
You need to log in before you can comment on or make changes to this bug.