1385252 - Prevent shutdown crashes from the preallocated process

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Gabor Krizsanits (INACTIVE)]

See: Bug 1363601.

Comment 1

[Gabor Krizsanits (INACTIVE)]

(In reply to Blake Kaplan (:mrbkap) from comment #26)
> Comment on attachment 8890877 [details] [diff] [review]
> ppm shutdownfix. v1
> 
> Review of attachment 8890877 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: 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".

Thanks :)

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

So this is a great question. I thought about this, we have a ClearOnShutdown(&sSingleton); on the preallocated process manager which should clear that reference soon after we hit this code, so it should not be a problem I think.

The alternative approach would be to keep the the "ipc:content-shutdown" listener and let that reset the reference, when it detects that the child process is gone. Then later we have to clean that listener too, but I think I could do that in the "xpcom-shutdown" and the rest during "profile-change-teardown". Would you prefer that approach? Or should I keep the original one?

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)
> So this is a great question. I thought about this, we have a
> ClearOnShutdown(&sSingleton); on the preallocated process manager which
> should clear that reference soon after we hit this code, so it should not be
> a problem I think.

I completely forgot about that ClearOnShutdown! I think that's fine (maybe just add a comment somewhere that we're relying on it near the call to Shutdown() in the xpcom-shutdown case. I can r+ the patch in the other bug if you'd like or re-attach it here and I'll stamp it here.

Comment 3

[Gabor Krizsanits (INACTIVE)]

Attachment: ppm shutdown fix. v2

(In reply to Blake Kaplan (:mrbkap) from comment #2)
> (In reply to Gabor Krizsanits [:krizsa :gabor] from comment #1)
> > So this is a great question. I thought about this, we have a
> > ClearOnShutdown(&sSingleton); on the preallocated process manager which
> > should clear that reference soon after we hit this code, so it should not be
> > a problem I think.
> 
> I completely forgot about that ClearOnShutdown! I think that's fine (maybe
> just add a comment somewhere that we're relying on it near the call to
> Shutdown() in the xpcom-shutdown case. I can r+ the patch in the other bug
> if you'd like or re-attach it here and I'll stamp it here.

Yeah I think it's really not obvious so it should be mentioned in the comment.
(I hope I have not added any extra typos this time...)

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8891936 [details] [diff] [review]
ppm shutdown fix. v2

Looks good.

Comment 5

[Gabor Krizsanits (INACTIVE)]

I accidentally landed it under Bug 1363601 because I forgot to update the patch comment after moving it over here :(