Closed Bug 1331685 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: IPC, defect, critical)

x86
Windows 10
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox52 --- fixed
firefox53 --- fixed
firefox54 --- fixed

People

(Reporter: ehsan, Assigned: ehsan)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file, 1 obsolete file)

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.
Otherwise, the IPC attempt will crash the content process.
Attachment #8827558 - Flags: review?(amarchesini)
Do we get profile-before-change in the child process? I think it might be better to use mShuttingDown here.
I don't think so/hope not: the child process doesn't "have a profile".
(In reply to Bill McCloskey (:billm) from comment #2)
> Do we get profile-before-change in the child process?

We definitely do not.
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?
(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.
Otherwise, the IPC attempt will crash the content process.
Attachment #8827710 - Flags: review?(wmccloskey)
Attachment #8827558 - Attachment is obsolete: true
Attachment #8827558 - Flags: review?(amarchesini)
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?
Attachment #8827710 - Flags: review?(wmccloskey) → review+
(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.
Flags: needinfo?(wmccloskey)
Let's land it on trunk. Bug 1332054 is going to be more complicated than I thought.
Flags: needinfo?(wmccloskey)
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.
Attachment #8827710 - Flags: approval-mozilla-beta?
Attachment #8827710 - Flags: approval-mozilla-aurora?
Pushed by eakhgari@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbda0614fc92
Ensure that the ContentChild constructor send methods return null when shutting down; r=billm
https://hg.mozilla.org/mozilla-central/rev/cbda0614fc92
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Comment on attachment 8827710 [details] [diff] [review]
Ensure that the ContentChild constructor send methods return null when shutting down

avoid shutdown crashes, aurora53+, beta52+
Attachment #8827710 - Flags: approval-mozilla-beta?
Attachment #8827710 - Flags: approval-mozilla-beta+
Attachment #8827710 - Flags: approval-mozilla-aurora?
Attachment #8827710 - Flags: approval-mozilla-aurora+
Setting qe-verify- per Comment 11.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.