1457301 - Crash in mozilla::ipc::ProcessLink::SendMessage | IPC_Message_Name=PBrowser::Msg_AsyncMessage

Status: RESOLVED FIXED

(Core :: IPC, defect, P1)

Description

[Marcia Knous [:marcia]]

This bug was filed from the Socorro interface and is
report bp-b9253079-c032-408e-906d-06d400180325.
=============================================================

Seen while looking at Mac specific nightly crashes - also affects Linux: https://bit.ly/2JvwxJD. Not super huge volume.

Crashes go back to Build 20180322100349.

Possible regression range based on build ID: https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=3d21d31141dc5e2d2aff89203458125a3cce6c64&tochange=8bf380faae74e4921be6000496ca09d4a2c44e8d

Top 10 frames of crashing thread:

0 libxul.so mozilla::ipc::ProcessLink::SendMessage ipc/glue/MessageLink.cpp:164
1 libxul.so mozilla::ipc::MessageChannel::Send [clone .cold.466] 
2 libxul.so mozilla::dom::PBrowserChild::SendAsyncMessage ipc/ipdl/PBrowserChild.cpp:163
3 libxul.so mozilla::dom::TabChild::DoSendAsyncMessage dom/ipc/TabChild.cpp:3073
4 libxul.so nsFrameMessageManager::DispatchAsyncMessage 
5 libxul.so mozilla::dom::ContentFrameMessageManagerBinding::sendAsyncMessage 
6 libxul.so mozilla::dom::ContentFrameMessageManagerBinding::genericMethod 
7 libxul.so js::InternalCallOrConstruct 
8 libxul.so js::Call js/src/vm/Interpreter.cpp:535
9 libxul.so js::ForwardingProxyHandler::call const 

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

Comment 1

[Julien Wajsberg [:julienw]]

I got https://crash-stats.mozilla.com/report/index/d156ef25-9b40-4a28-a548-3f9e50180515 while trying to capture a big performance profile. I don't know if that should be expected as this is the first time I tried to capture such a big one.

Comment 3

[Andrew McCreight [:mccr8]]

This crash means that somebody tried to send an overly large message via the message manager. The real question is, who. I don't think you can easily tell from a C++ stack.

Comment 4

[Andrew McCreight [:mccr8]]

Bug 888600 is in the regression window in comment 0. Peter, can you imagine any way that converting the message manager to WebIDL bindings would cause the IPC messages it sends to be bigger? I can't imagine how off hand.

Comment 5

[Andrew McCreight [:mccr8]]

The signature I added looks like the same crash, but on Windows.

Comment 6

[Julien Wajsberg [:julienw]]

When I tried my STR from comment 1 on older browsers, what I got is that it didn't work either, but it silently failed, while in the current Nightly, it crashes. So my guess is that the issue was here with the old code but just silently ignored, while the new code is stricter. But if it can be triggered from some user code this is not good.

Comment 7

[Peter Van der Beken [:peterv]]

I think I lost some checks :-( (same thing happened in sendMessage but bz caught that in review).

Comment 8

[Andrew McCreight [:mccr8]]

Oh, right, I forgot that there were size checks in the message manager.

Comment 9

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 10

[Liz Henry (:lizzard) (relman/hg->git project)]

New crash in 61, sounds worth tracking even if it's low volume.

Comment 11

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/e92d5e5095310ea59e3f787825732722b974c6f7
Bug 1457301 - Crash in mozilla::ipc::ProcessLink::SendMessage | IPC_Message_Name=PBrowser::Msg_AsyncMessage. r=mccr8.

Comment 12

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/e92d5e509531

Comment 13

[Ryan VanderMeulen [:RyanVM]]

No crashes on Nightly since this landed. Please request Beta approval on this when you get a chance.

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 8976319 [details] [diff] [review]
v1

Approval Request Comment
[Feature/Bug causing the regression]: bug 888600
[User impact if declined]: crashes
[Is this code covered by automated tests?]: not the case where we crash, but we run it otherwise.
[Has the fix been verified in Nightly?]: Yes, the crash is gone.
[Needs manual test from QE? If yes, steps to reproduce]: No.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: All it does is add back a check we used to have.
[String changes made/needed]: None.

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Comment on attachment 8976319 [details] [diff] [review]
v1

Re-adds code to block too-large IPC messages from causing crashes. Approved for 61.0b8.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/175d58ad6bd1