1354200 - crash in mozilla::ipc::MessageChannel::OnChannelErrorFromLink() on asan builds

Status: RESOLVED FIXED

(Core :: IPC, defect, P2)

Description

[Hanno Boeck]

Attachment: asan-mozilla::ipc::MessageChannel::OnChannelErrorFromLink-MessageChannel.cpp:2280.txt

I'm getting occasional crashes (with crash dumps) with the asan builds of mozilla. (asan build "target.tar.bz2" downloaded on 2017-04-04, sha256 69926deabcd04a4a2e6030001dae365bca5985ab83979b42bf1675b969e2377f)

The crashes usually happen if I want to close the browser during heavy activity (I was experimenting with HTML files including lots of iframes). However I could also reproduce them most reliably by just starting firefox and killing it shortly after, e.g.:
./firefox & sleep 3; killall firefox

(This doesn't depend on "killall", it also sometimes works with clicking the close symbol, but needs the right timing)

I got different stack traces, but the by far most common one indicates a crash in  mozilla::ipc::MessageChannel::OnChannelErrorFromLink().

I got a different crash in mozilla::ipc::BackgroundChildImpl::ProcessingError(), however I was not able to reproduce that and only got it once.

A third crash is probably irrelevant: It happens in RunWatchdog(), looking at the code it is just some kind of timeout where it kills the application if it supposedly hangs. I just mention it for completeness.

I'll attach all three stack traces.

Comment 1

[Hanno Boeck]

Attachment: asan-mozilla::ipc::MessageChannel::OnChannelErrorFromLink-MessageChannel.cpp:2280.txt

Comment 2

[Hanno Boeck]

Attachment: asan-mozilla::ipc::BackgroundChildImpl::ProcessingError-BackgroundChildImpl.cpp:142.txt

Comment 3

[Hanno Boeck]

Attachment: asan-mozilla::_::RunWatchdog-nsTerminator.cpp:159.txt

Comment 4

[Andrew McCreight [:mccr8]]

These are all various intentional crashes when we detect an error. The OnChannelError ones are here:
  MOZ_CRASH("Aborting on channel error.");
The nsTerminator one is here:
  MOZ_CRASH("Shutdown too long, probably frozen, causing a crash.");

Comment 5

[Bob Owen (:bobowen)]

jld - tsmith mentioned that they hit this in ASan builds at the all hands, this is one of a number of similar bugs that I picked.

It looks like this [1] was originally put in to try and clean up orphaned plugin processes (and I guess subsequently all children).
I wonder if MOZ_CRASH is really what we want to do here or whether something like ProcessChild::QuickExit would be more appropriate.

[1] https://searchfox.org/mozilla-central/rev/fd62b95c187a40b328d9e7fd9d848833a6942b57/ipc/glue/MessageChannel.cpp#2527

Comment 6

[Jed Davis [:jld] ⟨⏰|UTC-8⟩ ⟦he/him⟧]

It does look like that was more or less the intent in bug 778866.  Assuming nobody's trying to get information out of the crashes, I agree this could just be a QuickExit, but I think we should log something in case the exit happens when it shouldn't (cf. bug 1368036).

Comment 7

[James Willcox (:snorp) ([e-mail]) (he/him)]

Attachment: Bug 1354200 - Exit instead of MOZ_CRASH on channel error in child process r=jld

Android mercilessly kills the parent in low memory situations, and we
don't want that to trigger a crash when the child is abruptly
disconnected.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/36802e2a3490
Exit instead of MOZ_CRASH on channel error in child process r=jld

Comment 10

[James Willcox (:snorp) ([e-mail]) (he/him)]

Comment on attachment 9035725 [details]
Bug 1354200 - Exit instead of MOZ_CRASH on channel error in child process r=jld

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: N/A

User impact if declined: Frequent crashes when Firefox Focus is in the background.

Is this code covered by automated tests?: No

Has the fix been verified in Nightly?: No

Needs manual test from QE?: No

If yes, steps to reproduce:

List of other uplifts needed: None

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Very small and simply avoids an intentional crash.

String changes made/needed: None

Comment 11

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/36802e2a3490

Comment 12

[Chris Peterson [:cpeterson]]

(In reply to James Willcox (:snorp) ([e-mail]) from comment #10)

Attachment #9035725 [details] - Flags: approval-mozilla-release?

James, are you requesting uplift to just the GECKOVIEW_64_RELBRANCH branch? Or also Fennec 64 and Firefox 64 desktop on the Release channel?

IIUC, this fix won't affect Fennec (because it doesn't use e10s), but it could affect/benefit Firefox desktop.

Comment 13

[Julien Cristau [:jcristau]]

Comment on attachment 9035725 [details]
Bug 1354200 - Exit instead of MOZ_CRASH on channel error in child process r=jld

Moving uplift request to gv64, I don't think we should be taking this on the default branch.

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

Thanks. Julien can we poke someone to approve and land on geckoview64? That's the only current vehicle we have to confirm the fix, and there is very low risk.

Comment 15

[Julien Cristau [:jcristau]]

Comment on attachment 9035725 [details]
Bug 1354200 - Exit instead of MOZ_CRASH on channel error in child process r=jld

You can poke me :)

Approved for beta65 and gv64, will land on the latter in a bit.

Comment 16

[Julien Cristau [:jcristau]]

https://hg.mozilla.org/releases/mozilla-release/rev/f9045f8090c7578c1d92d145b3eb64efe459a25c

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/58340264bdac