1437114 - Crash in mozilla::ipc::ProcessLink::SendMessageW | IPC_Message_Name=PExternalHelperApp::Msg_OnDataAvailable

Status: RESOLVED FIXED

(Core :: XPCOM, defect, P3)

Description

[Calixte Denizet (:calixte)]

This bug was filed from the Socorro interface and is
report bp-ce89f17d-1396-4dee-9c8c-39f3c0180209.
=============================================================

Top 10 frames of crashing thread:

0 xul.dll mozilla::ipc::ProcessLink::SendMessageW ipc/glue/MessageLink.cpp:164
1 xul.dll mozilla::ipc::MessageChannel::Send ipc/glue/MessageChannel.cpp:902
2 xul.dll mozilla::dom::PExternalHelperAppChild::SendOnDataAvailable ipc/ipdl/PExternalHelperAppChild.cpp:98
3 xul.dll mozilla::dom::ExternalHelperAppChild::OnDataAvailable uriloader/exthandler/ExternalHelperAppChild.cpp:51
4 xul.dll nsDocumentOpenInfo::OnDataAvailable uriloader/base/nsURILoader.cpp:340
5 xul.dll nsBaseChannel::OnDataAvailable netwerk/base/nsBaseChannel.cpp:904
6 xul.dll nsInputStreamPump::OnStateTransfer netwerk/base/nsInputStreamPump.cpp:599
7 xul.dll nsInputStreamPump::OnInputStreamReady netwerk/base/nsInputStreamPump.cpp:432
8 xul.dll mozilla::NonBlockingAsyncInputStream::RunAsyncWaitCallback xpcom/io/NonBlockingAsyncInputStream.cpp:379
9 xul.dll mozilla::NonBlockingAsyncInputStream::AsyncWaitRunnable::Run xpcom/io/NonBlockingAsyncInputStream.cpp:31

=============================================================

There are 25 crashes (from are 16 installations) in nightly 60 starting with buildid 20180209005628. In analyzing the backtrace, the regression may have been introduced by patch [1] to fix bug 1371699.

[1] https://hg.mozilla.org/mozilla-central/rev?node=68c85fdcd9f6

Comment 1

[Nathan Froyd [:froydnj]]

There's also ~250 crashes from release in the last week for basically the same signature (going through JS in some cases)...

Comment 2

[Andrea Marchesini [:baku]]

MOZ_CRASH(IPC message size is too large)

We should probably split the body message instead sending it directly as it is in PExternalHelperAppChild::SendOnDataAvailable.

Comment 3

[Andrea Marchesini [:baku]]

Attachment: ipc.patch

What about this approach?

Comment 4

[Andrew McCreight [:mccr8]]

This signature also shows up when you try to save a PDF, FWIW. See bug 1274706.

Comment 5

[Nathan Froyd [:froydnj]]

Comment on attachment 8949822 [details] [diff] [review]
ipc.patch

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

This approach is essentially the same as the one taken by HttpChannelParent::OnDataAvailable, with a couple significant differences:

1. The HttpChannelParent implementation uses a smaller chunk size, which is desirable to avoid OOMs.
2. The HttpChannelParent implementation allocates the read buffer once, rather than every trip through the loop.

I think we should make both of those changes here.

Comment 6

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #5)
> 2. The HttpChannelParent implementation allocates the read buffer once,
> rather than every trip through the loop.

Actually, this is completely untrue, because NS_ReadInputStreamToString completely ignores the string capacity: it simply reads into a new buffer and then has the string Adopt() the buffer.  Which I guess makes a certain amount of sense...

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd [:froydnj] from comment #6)
> (In reply to Nathan Froyd [:froydnj] from comment #5)
> > 2. The HttpChannelParent implementation allocates the read buffer once,
> > rather than every trip through the loop.
> 
> Actually, this is completely untrue, because NS_ReadInputStreamToString
> completely ignores the string capacity: it simply reads into a new buffer
> and then has the string Adopt() the buffer.  Which I guess makes a certain
> amount of sense...

Actually, NS_ReadInputStringToString used to behave as I thought it did, and then got changed by bug 1415081.  Filed bug 1437152 for the regression.

Comment 8

[Andrea Marchesini [:baku]]

Attachment: ipc.patch

Comment 9

[Nicholas Nethercote [:njn] (On Long Service Leave)]

This is the #2 Windows top crash in Nightly 20180209005628.

Comment 10

[Nathan Froyd [:froydnj]]

Comment on attachment 8949962 [details] [diff] [review]
ipc.patch

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

WFM, thank you!  If we want to uplift this, it would be nice if we uplifted bug 1437152 as well.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75b746a5d92c
ExternalHelperAppChild should send data to the parent actor in chunks, r=froydnj

Comment 12

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/75b746a5d92c

Comment 13

[[:philipp]]

i don't see any more crashes on nightly builds during the past week after the patch has landed. should we get this uplifted to 59?

Comment 14

[Andrea Marchesini [:baku]]

Comment on attachment 8949962 [details] [diff] [review]
ipc.patch

Approval Request Comment
[Feature/Bug causing the regression]: bug 1371699.
[User impact if declined]: A crash can occur if a big chunk of data is sent to the external helper app handler.
[Is this code covered by automated tests?]: n/a
[Has the fix been verified in Nightly?]: no crash report received
[Needs manual test from QE? If yes, steps to reproduce]: n/a 
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: low
[Why is the change risky/not risky?]: Instead sending a huge chunk of data as a string to the ExternalHelperAppChild actor, this patch splits the data in multiple IPC packages. This is something similar we do for HttpChannel actors.
[String changes made/needed]: none

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8949962 [details] [diff] [review]
ipc.patch

Looks like a low-risk fix and already verified via crash-stats on Nightly. Let's take this for 59b12.

Comment 16

[Sebastian Hengst [:aryx] (needinfo on intermittent or backout)]

https://hg.mozilla.org/releases/mozilla-beta/rev/e0f32aa8b45d