Closed
Bug 1437114
Opened 4 years ago
Closed 4 years ago
Crash in mozilla::ipc::ProcessLink::SendMessageW | IPC_Message_Name=PExternalHelperApp::Msg_OnDataAvailable
Categories
(Core :: XPCOM, defect, P3)
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)
1.75 KB,
patch
|
froydnj
:
review+
RyanVM
:
approval-mozilla-beta+
|
Details | Diff | Splinter Review |
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)
![]() |
||
Comment 1•4 years ago
|
||
There's also ~250 crashes from release in the last week for basically the same signature (going through JS in some cases)...
Priority: -- → P3
Assignee | ||
Comment 2•4 years ago
|
||
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)
![]() |
||
Updated•4 years ago
|
Assignee | ||
Comment 3•4 years ago
|
||
What about this approach?
Assignee: nobody → amarchesini
Attachment #8949822 -
Flags: review?(nfroyd)
Comment 4•4 years ago
|
||
This signature also shows up when you try to save a PDF, FWIW. See bug 1274706.
See Also: → 1274706
![]() |
||
Comment 5•4 years ago
|
||
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-
Reporter | ||
Updated•4 years ago
|
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]
![]() |
||
Comment 6•4 years ago
|
||
(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•4 years ago
|
||
(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.
Assignee | ||
Comment 8•4 years ago
|
||
Attachment #8949822 -
Attachment is obsolete: true
Attachment #8949962 -
Flags: review?(nfroyd)
![]() |
||
Comment 9•4 years ago
|
||
This is the #2 Windows top crash in Nightly 20180209005628.
![]() |
||
Comment 10•4 years ago
|
||
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+
Comment 11•4 years ago
|
||
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
Comment 12•4 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/75b746a5d92c
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
Comment 13•4 years ago
|
||
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)
Assignee | ||
Comment 14•4 years ago
|
||
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 15•4 years ago
|
||
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+
Updated•4 years ago
|
![]() |
||
Comment 16•4 years ago
|
||
bugherderuplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/e0f32aa8b45d
You need to log in
before you can comment on or make changes to this bug.
Description
•