Prevent shutdown crashes from the preallocated process

RESOLVED FIXED

Status

()

Core
DOM: Content Processes
RESOLVED FIXED
4 months ago
4 months ago

People

(Reporter: krizsa, Assigned: krizsa)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

See: Bug 1363601.
Blocks: 1385249
(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?
Flags: needinfo?(mrbkap)
(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.
Flags: needinfo?(mrbkap) → needinfo?(gkrizsanits)
Created attachment 8891936 [details] [diff] [review]
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...)
Assignee: nobody → gkrizsanits
Flags: needinfo?(gkrizsanits)
Attachment #8891936 - Flags: review?(mrbkap)
Comment on attachment 8891936 [details] [diff] [review]
ppm shutdown fix. v2

Looks good.
Attachment #8891936 - Flags: review?(mrbkap) → review+
I accidentally landed it under Bug 1363601 because I forgot to update the patch comment after moving it over here :(
Status: NEW → RESOLVED
Last Resolved: 4 months ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.