Closed Bug 1437114 Opened 2 years ago Closed 2 years ago

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

Categories

(Core :: XPCOM, defect, P3, critical)

x86
Windows 8
defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox58 --- wontfix
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: calixte, Assigned: baku)

References

(Blocks 1 open bug)

Details

(Keywords: crash, regression)

Crash Data

Attachments

(1 file, 1 obsolete file)

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
Flags: needinfo?(amarchesini)
There's also ~250 crashes from release in the last week for basically the same signature (going through JS in some cases)...
Priority: -- → P3
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.
Flags: needinfo?(amarchesini)
Attached patch ipc.patch (obsolete) — Splinter Review
What about this approach?
Assignee: nobody → amarchesini
Attachment #8949822 - Flags: review?(nfroyd)
This signature also shows up when you try to save a PDF, FWIW. See bug 1274706.
See Also: → 1274706
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.
Attachment #8949822 - Flags: review?(nfroyd) → review-
Crash Signature: [@ mozilla::ipc::ProcessLink::SendMessageW | IPC_Message_Name=PExternalHelperApp::Msg_OnDataAvailable] → [@ mozilla::ipc::ProcessLink::SendMessageW | IPC_Message_Name=PExternalHelperApp::Msg_OnDataAvailable] [@ mozilla::ipc::ProcessLink::SendMessage | IPC_Message_Name=PExternalHelperApp::Msg_OnDataAvailable]
(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...
(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.
Attached patch ipc.patchSplinter Review
Attachment #8949822 - Attachment is obsolete: true
Attachment #8949962 - Flags: review?(nfroyd)
This is the #2 Windows top crash in Nightly 20180209005628.
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.
Attachment #8949962 - Flags: review?(nfroyd) → review+
Pushed by amarchesini@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/75b746a5d92c
ExternalHelperAppChild should send data to the parent actor in chunks, r=froydnj
https://hg.mozilla.org/mozilla-central/rev/75b746a5d92c
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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?
Flags: needinfo?(amarchesini)
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
Flags: needinfo?(amarchesini)
Attachment #8949962 - Flags: approval-mozilla-beta?
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.
Attachment #8949962 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.