1363601 - Crash in mozilla::ipc::MessageChannel::Clear from ~PContentParent()

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Ting-Yu Chou [:ting] (away)]

This bug was filed from the Socorro interface and is 
report bp-c3a28f8d-7da4-4940-9288-f42a90170510.
=============================================================
Top #12 of Nightly 20170507030205 on Windows, 14 reports from 11 installations. The moz crash reason is:

  MOZ_CRASH(MessageChannel destroyed without being closed)

Comment 1

[Calixte Denizet (:calixte)]

There are 88 crashes in nightly 55 and they shew up in nightly 20170504030320 (more info in [1]).
:billm, could you help to find someone to fix that ?

[1] https://crash-stats.mozilla.com/search/?signature=%3Dmozilla%3A%3Aipc%3A%3AMessageChannel%3A%3AClear&version=55.0a1&proto_signature=~PContentParent%3A%3A~PContentParent&product=Firefox&date=%3E%3D2017-02-10T13%3A05%3A10.000Z&date=%3C2017-05-10T13%3A05%3A10.000Z&_sort=-date&_facets=signature&_facets=build_id&_facets=install_time&_facets=proto_signature&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-build_id

Comment 2

[Andrew McCreight [:mccr8]]

I think this is the set of changesets added in the 5-4 Nightly, though nothing obviously related jumps out at me:
  https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=82c2d17e74ef9cdf38a5d5ac4eb3ae846ec30ba4&tochange=33b92d9c40562dab3d7b602368c75619f1d793f7

92 of these crashes look more or less like the crash in comment 0, from ~ContentParent, getting run in the FreeSnowWhite call during CC shutdown.

There are another 40 from the GPU process. I'll file another bug for that.

Comment 3

[Nicholas Nethercote [inactive]]

In the last 7 days there have been 89 crashes with the signature in Firefox 55.0a1, across 70 installations, making it #15 top crasher.

Comment 4

[[:philipp]]

[Tracking Requested - why for this release]: this seems to have risen to the #2 browser crash in 55.0b9

Comment 5

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Hi Andrew and Bill,
Could either of you help take a look at this? The rising crash volume is concerning for 55. Thanks!

Comment 6

[Julien Cristau [:jcristau]]

tracking as topcrash in beta55

Comment 7

[Andrew McCreight [:mccr8]]

I'm not entirely sure what this assertion is, but 100% of the beta crashes have ShutdownProgress equal to xpcom-shutdown

Comment 8

[Andrew McCreight [:mccr8]]

(The first signature is no longer present, due to skip list changes.)

Comment 9

[Andrew McCreight [:mccr8]]

This assertion seems like it would be racy for PContentParent. I think when we're shutting down, at some point we call PContentParent::ShutDownProcess, which sends a message to the child and starts a force kill timer. Is there anything that is going to prevent shutdown from going along further until either we get a reply from the child or the force kill timer runs? Though somehow the refcount of ContentParent is hitting zero.

This comment in ContentParent::ActorDestroy seems like it might be an issue:
    // If we shut down normally but haven't called Close, assume somebody
    // else called Close on us. In that case, we still need to call
    // ShutDownProcess below to perform other necessary clean up.

Anyways, I don't think I know enough about this to do more, so hopefully Bill will have a chance to look at it.

Comment 10

[[:philipp]]

the increase on beta seems to coincide with fixing the rollout of e10s-multi in bug 1380725.
this is the crashing graph on beta for the e10s cohort "multibucket4": http://bit.ly/2u9EuPO

Comment 11

[Gabor Krizsanits (INACTIVE)]

Since I remember hitting this in a very edge case scenario, with the preallocated process manager (which was fixed before landing it) I'm considering turning it off completely on beta if we don't find another solution to see if that helps here.

Comment 12

[Jim Mathies [:jimm]]

Some detail:

1) all of these occur in the main process during shutdown
2) no uptime correlation, so not a startup crash
3) triggers the crash reporter dialog, we get a few comments but not many

https://crash-stats.mozilla.com/search/?signature=%3Dmozilla%3A%3Aipc%3A%3AMessageChannel%3A%3AClear%20%7C%20mozilla%3A%3Aipc%3A%3AMessageChannel%3A%3A~MessageChannel%20%7C%20mozilla%3A%3Adom%3A%3APContentParent%3A%3A~PContentParent&version=55.0b&product=Firefox&date=%3E%3D2017-07-12T13%3A56%3A00.000Z&date=%3C2017-07-19T13%3A56%3A00.000Z&_sort=-date&_facets=signature&_facets=shutdown_progress&_facets=process_type&_columns=date&_columns=signature&_columns=product&_columns=version&_columns=build_id&_columns=platform#facet-shutdown_progress

Comment 13

[Gabor Krizsanits (INACTIVE)]

Based on the difference between nightly and beta, I wonder if this was caused by an uplift.

Since all the crashes are from Windows I wonder if this is somehow related to the GPU process. Dave, you uplifted a modified version of the patch from bug 1374258 and I was wondering if that could be related.

Comment 14

[Blake Kaplan (:mrbkap) (inactive)]

FWIW, I don't think this can be the GPU process. This is pretty clearly a ContentParent (and PContentParent) that is dying and the GPU process doesn't use either. I've been staring at the code for a while now and I don't understand how this is possible. All ContentParents that have Init called on them listen for xpcom-shutdown and when they receive the notification, they synchronously wait for the process to shut down [1]. I spent a bunch of time trying to find a path through the ContentParent shutdown code that didn't end either by crashing the process or by calling Close and couldn't find anything (though I don't understand how the code to force a crash actually works).

I also looked at the differences between Nightly and Beta in the preallocated process manager and didn't see anything that jumps out. We added some checks that we don't try to reuse a shutdown content process, but the problem here is that we have a process that was never shut down. I also tried verifying that if we get into ContentParent::ActorDestroy (where we unregister from watching xpcom-shutdown) that it must mean that the MessageChannel was closed (either in error or not) and it appeared to be.

One Hail Mary idea I had was to try the sync wait in xpcom-shutdown in the process manager, but I really don't understand how it could be needed.

[1] http://searchfox.org/mozilla-central/rev/3a3af33f513071ea829debdfbc628caebcdf6996/dom/ipc/ContentParent.cpp#2760,2768

Comment 15

[Gabor Krizsanits (INACTIVE)]

(In reply to Blake Kaplan (:mrbkap) from comment #14)
> FWIW, I don't think this can be the GPU process. This is pretty clearly a
> ContentParent (and PContentParent) that is dying and the GPU process doesn't
> use either. I've been staring at the code for a while now and I don't

You're right, I set that flag before we validated the protocol name for the new crashes.
So, I'm unchecking the needinfo flag for David.

> understand how this is possible. All ContentParents that have Init called on
> them listen for xpcom-shutdown and when they receive the notification, they
> synchronously wait for the process to shut down [1]. I spent a bunch of time
> trying to find a path through the ContentParent shutdown code that didn't
> end either by crashing the process or by calling Close and couldn't find
> anything (though I don't understand how the code to force a crash actually
> works).
> 
> I also looked at the differences between Nightly and Beta in the
> preallocated process manager and didn't see anything that jumps out. We
> added some checks that we don't try to reuse a shutdown content process, but
> the problem here is that we have a process that was never shut down. I also
> tried verifying that if we get into ContentParent::ActorDestroy (where we
> unregister from watching xpcom-shutdown) that it must mean that the
> MessageChannel was closed (either in error or not) and it appeared to be.
> 
> One Hail Mary idea I had was to try the sync wait in xpcom-shutdown in the
> process manager, but I really don't understand how it could be needed.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 3a3af33f513071ea829debdfbc628caebcdf6996/dom/ipc/ContentParent.cpp#2760,2768

Actually there are some crashes on nightly as well just less frequent. I think
you were checking the right thing and what occurred to me is that the shutdown
listener for the preallocated process is triggered first because it listens to
"profile-change-teardown" [1] which comes before the "profile-before-change" a
regular content parent listens to. And for the preallocated process we don't
do the sync waiting for process shutdown, just this: [2]. I'll write a patch.

[1]: http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/dom/ipc/PreallocatedProcessManager.cpp#134
[2]: http://searchfox.org/mozilla-central/rev/a83a4b68974aecaaacdf25145420e0fe97b7aa22/dom/ipc/PreallocatedProcessManager.cpp#284

Comment 16

[Gabor Krizsanits (INACTIVE)]

Attachment: disable the preallocated process manager on beta and above. v1

Comment 17

[Gabor Krizsanits (INACTIVE)]

Attachment: beta uplift version

Approval Request Comment
[Feature/Bug causing the regression]: preallocated process manager
[User impact if declined]: frequent crashes
[Is this code covered by automated tests?]: yes
[Has the fix been verified in Nightly?]: it's already turned off on nightly for activity stream
[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?]: it's an undertested feature I'm just turning it off
[String changes made/needed]: none

Comment 18

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

Comment on attachment 8888388 [details] [diff] [review]
beta uplift version

Turning this off as it causes crashes with e10s-multi.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

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

Comment 20

[Andrei Vaida [:avaida]]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #17)
> [Is this code covered by automated tests?]: yes
> [Has the fix been verified in Nightly?]: it's already turned off on nightly
> for activity stream
> [Needs manual test from QE? If yes, steps to reproduce]: no

Setting qe-verify- based on Gabor's assessment on manual testing needs and the fact that this fix has automated coverage.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1044cf0518e9
Disabling the preallocated process manager on all channels. r=mrbkap

Comment 22

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

https://hg.mozilla.org/mozilla-central/rev/1044cf0518e9

Comment 23

[Ed Lee :Mardak]

On nightly, I had already turned off the preallocated process manager in bug 1381804. There were a couple talos performance regression/improvements associated to it.

tp5 memory improvement: bug 1381804 comment 21
damp regression: bug 1382608

Comment 24

[Gabor Krizsanits (INACTIVE)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7b355fdd39c52a2fc35f97f61b6b288dec08612f&selectedJob=118015987

Comment 25

[Gabor Krizsanits (INACTIVE)]

Attachment: ppm shutdownfix. v1

I think this will do the trick. I will have to fix bug 1376895 before I can re-enable the ppm on nightly though... but on try it looks OK with the ppm on, so I would like to land it.

(also removing the needinfo flag from Bill)

Comment 26

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8890877 [details] [diff] [review]
ppm shutdownfix. v1

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

Either we should re-open this bug or track this patch in a new bug. It's odd to me to work in a FIXED bug like this.

I have a question I'd like the answer to before stamping this.

::: dom/ipc/PreallocatedProcessManager.cpp
@@ +134,5 @@
>        os->RemoveObserver(this, "profile-change-teardown");
>      }
> +    // Let's prevent any new preallocated processes to start, but also,
> +    // we should let the ConentParent to handle the shutdown of the existing
> +    // process at this point.

"Let's prevent any new preallocated processes from starting." "ContentParent".

Do we have to worry about the strong ref to mPreallocatedProcess not getting cleared any more and holding the ContentParent alive through shutdown?

Comment 27

[Gabor Krizsanits (INACTIVE)]

(In reply to Blake Kaplan (:mrbkap) from comment #26)
> 
> Either we should re-open this bug or track this patch in a new bug. It's odd
> to me to work in a FIXED bug like this.

Oh, I thought I reopened it... anyway, even if this patch is the real fix probably for this crash I think we should do this in a new bug. I'll filed: bug 1385252.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c91659401a6d
Let the ContentParent destroy the preallocated process during shutdown. r=mrbkap

Comment 29

[Gabor Krizsanits (INACTIVE)]

My intention was to land this patch under bug 1385252 :( sorry for the confusion I caused here.

Comment 30

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

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