1331685 - Crash in mozalloc_abort | NS_DebugBreak | mozilla::ipc::FatalError | mozilla::dom::PContentChild::FatalError | mozilla::dom::PContentChild::SendPBlobConstructor

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[(no longer active)]

This bug was filed from the Socorro interface and is 
report bp-229e62d8-48fc-4c2d-9583-b0cc92170116.
=============================================================

Here we have a ContentChild::Send*Constructor() crashing because it attempts to do IPC after shutdown is initiated.  I have a patch to fix this for all of these methods on ContentChild.

Comment 1

[(no longer active)]

Attachment: Ensure that the ContentChild constructor send methods return null when shutting down

Otherwise, the IPC attempt will crash the content process.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Do we get profile-before-change in the child process? I think it might be better to use mShuttingDown here.

Comment 3

[Benjamin Smedberg]

I don't think so/hope not: the child process doesn't "have a profile".

Comment 4

[Andrew McCreight [:mccr8]]

(In reply to Bill McCloskey (:billm) from comment #2)
> Do we get profile-before-change in the child process?

We definitely do not.

Comment 5

[(no longer active)]

Hmm, crash reports such as bp-42ea8b38-e99a-4462-a4a3-78f212170116 include |ShutdownProgress: profile-before-change|.  Doesn't this mean this notification is dispatched in the child?

Comment 6

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(In reply to :Ehsan Akhgari from comment #5)
> Hmm, crash reports such as bp-42ea8b38-e99a-4462-a4a3-78f212170116 include
> |ShutdownProgress: profile-before-change|.  Doesn't this mean this
> notification is dispatched in the child?

That annotation comes from the parent, I believe.

Comment 7

[(no longer active)]

Attachment: Ensure that the ContentChild constructor send methods return null when shutting down

Otherwise, the IPC attempt will crash the content process.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8827710 [details] [diff] [review]
Ensure that the ContentChild constructor send methods return null when shutting down

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

I filed bug 1332054 so we don't have to do this. But this patch makes sense for backporting.

::: dom/ipc/ContentChild.cpp
@@ +1588,5 @@
>                                        const uint32_t& aChromeFlags,
>                                        const ContentParentId& aCpID,
>                                        const bool& aIsForBrowser)
>  {
> +  if (IsShuttingDown()) {

We should just assert !IsShuttingDown here.

@@ +3164,5 @@
>  #if defined(XP_WIN) && defined(ACCESSIBILITY)
>  bool
>  ContentChild::SendGetA11yContentId()
>  {
> +  if (IsShuttingDown()) {

What's this for?

Comment 9

[(no longer active)]

(In reply to Bill McCloskey (:billm) from comment #8)
> I filed bug 1332054 so we don't have to do this. But this patch makes sense
> for backporting.

Should I land it on trunk?  Or just nominate for backporting?

> ::: dom/ipc/ContentChild.cpp
> @@ +1588,5 @@
> >                                        const uint32_t& aChromeFlags,
> >                                        const ContentParentId& aCpID,
> >                                        const bool& aIsForBrowser)
> >  {
> > +  if (IsShuttingDown()) {
> 
> We should just assert !IsShuttingDown here.
> 
> @@ +3164,5 @@
> >  #if defined(XP_WIN) && defined(ACCESSIBILITY)
> >  bool
> >  ContentChild::SendGetA11yContentId()
> >  {
> > +  if (IsShuttingDown()) {
> 
> What's this for?

That's a mistake, I'll remove it.

Comment 10

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Let's land it on trunk. Bug 1332054 is going to be more complicated than I thought.

Comment 11

[(no longer active)]

Comment on attachment 8827710 [details] [diff] [review]
Ensure that the ContentChild constructor send methods return null when shutting down

Approval Request Comment
[Feature/Bug causing the regression]: Unclear, we have probably had this bug in e10s mode for quite a while.
[User impact if declined]: Shutdown crashes.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: No.  This is a low volume crash and is hard to verify on Nightly because of that, but the nature of the crash is well understood.
[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?]: This just makes us fail early in some cases during shutdown where we currently crash.
[String changes made/needed]: None.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbda0614fc92
Ensure that the ContentChild constructor send methods return null when shutting down; r=billm

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

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

Comment 14

[Julien Cristau [:jcristau]]

Comment on attachment 8827710 [details] [diff] [review]
Ensure that the ContentChild constructor send methods return null when shutting down

avoid shutdown crashes, aurora53+, beta52+

Comment 15

[Ryan VanderMeulen [:RyanVM]]

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

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/764865df33d2

Comment 17

[Andrei Vaida [:avaida]]

Setting qe-verify- per Comment 11.