Closed Bug 1216972 Opened 4 years ago Closed 4 years ago

AsyncShutdown for content processes

Categories

(Toolkit :: Async Tooling, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox44 + fixed
firefox45 --- fixed
b2g-v2.5 --- fixed

People

(Reporter: Yoric, Assigned: Yoric)

References

(Blocks 1 open bug)

Details

Attachments

(3 files)

No description provided.
Comment on attachment 8676810 [details]
MozReview Request: Bug 1216972 - AsyncShutdown for content processes;r=froydnj

https://reviewboard.mozilla.org/r/22813/#review20275
Attachment #8676810 - Flags: review+
Attachment #8676810 - Flags: review?(nfroyd)
Comment on attachment 8676810 [details]
MozReview Request: Bug 1216972 - AsyncShutdown for content processes;r=froydnj

Bug 1216972 - AsyncShutdown for content processes;r=froydnj
Assignee: nobody → dteller
Comment on attachment 8676810 [details]
MozReview Request: Bug 1216972 - AsyncShutdown for content processes;r=froydnj

Bug 1216972 - AsyncShutdown for content processes;r=froydnj
Attachment #8676810 - Flags: review?(nfroyd)
Comment on attachment 8676810 [details]
MozReview Request: Bug 1216972 - AsyncShutdown for content processes;r=froydnj

Bug 1216972 - AsyncShutdown for content processes;r?froydnj
Attachment #8676810 - Attachment description: MozReview Request: Bug 1216972 - AsyncShutdown for content processes;r=froydnj → MozReview Request: Bug 1216972 - AsyncShutdown for content processes;r?froydnj
Attachment #8676810 - Flags: review?(nfroyd)
Removing checkin-needed as Nathan can't wait to re-review that code :)
Keywords: checkin-needed
Comment on attachment 8676810 [details]
MozReview Request: Bug 1216972 - AsyncShutdown for content processes;r=froydnj

https://reviewboard.mozilla.org/r/22813/#review20473

r=me with the IDL fixed.  I guess we need a followup bug for the AsyncShutdown partitioning of blockers between parent and child processes?  (Unless you've tested with the partitioning and find that everything's OK?)

::: toolkit/components/asyncshutdown/AsyncShutdown.jsm:985
(Diff revision 4)
> +// Parent process
>  this.AsyncShutdown.profileChangeTeardown = getPhase("profile-change-teardown");
>  this.AsyncShutdown.profileBeforeChange = getPhase("profile-before-change");
>  this.AsyncShutdown.sendTelemetry = getPhase("profile-before-change2");

Shouldn't we be limiting these to !isContent?

(I see we had the same sort of conversation in https://bugzilla.mozilla.org/show_bug.cgi?id=1043863#c13 .  It's good to know that I'm at least a little consistent in my reviews. :)

::: toolkit/components/asyncshutdown/nsIAsyncShutdown.idl:208
(Diff revision 4)
> +  readonly attribute nsIAsyncShutdownClient contentBeforeShutdown;

Shouldn't this be contentChildShutdown to correspond with the variables declared in the jsm?
Attachment #8676810 - Flags: review?(nfroyd) → review+
https://reviewboard.mozilla.org/r/22813/#review20473

> Shouldn't we be limiting these to !isContent?
> 
> (I see we had the same sort of conversation in https://bugzilla.mozilla.org/show_bug.cgi?id=1043863#c13 .  It's good to know that I'm at least a little consistent in my reviews. :)

> Shouldn't we be limiting these to !isContent?

Yes, we should, but for the moment, this breaks stuff. I'm working on it in bug 1158156.

> Shouldn't this be contentChildShutdown to correspond with the variables declared in the jsm?

You are entirely right, it should.
I haven't been able to land this because for some reason it breaks a add-on manager test.
This is not a measurement issue, but rather a correctness fix
No longer blocks: e10s-measurement
Attachment #8676810 - Attachment description: MozReview Request: Bug 1216972 - AsyncShutdown for content processes;r?froydnj → MozReview Request: Bug 1216972 - AsyncShutdown for content processes;r=froydnj
Comment on attachment 8676810 [details]
MozReview Request: Bug 1216972 - AsyncShutdown for content processes;r=froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22813/diff/4-5/
Bug 1216972 - OS.File AsyncShutdown for content processes;r?froydnj
Attachment #8689208 - Flags: review?(nfroyd)
Comment on attachment 8676810 [details]
MozReview Request: Bug 1216972 - AsyncShutdown for content processes;r=froydnj

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/22813/diff/5-6/
Bug 1216972 - OS.File AsyncShutdown for content processes;r?froydnj
Attachment #8689399 - Flags: review?(nfroyd)
Attachment #8689208 - Attachment description: MozReview Request: Bug 1216972 - OS.File AsyncShutdown for content processes;r?froydnj → MozReview Request: Bug 1216972 - MediaManager AsyncShutdown for content processes;r?jesup
Attachment #8689208 - Flags: review?(nfroyd) → review?(rjesup)
Comment on attachment 8689208 [details]
MozReview Request: Bug 1216972 - MediaManager AsyncShutdown for content processes;r?jesup

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/25575/diff/1-2/
Attachment #8689208 - Flags: review?(rjesup) → review+
Comment on attachment 8689208 [details]
MozReview Request: Bug 1216972 - MediaManager AsyncShutdown for content processes;r?jesup

https://reviewboard.mozilla.org/r/25575/#review23059

::: dom/media/MediaManager.cpp:113
(Diff revision 2)
> +  MOZ_RELEASE_ASSERT(NS_SUCCEEDED(rv));

I realize you're being cautious here, but release asserts to catch GetContentChildShutdown succeeding with a null shutdownPhase (in release) seems like overkill.  However, it's really just a code-size issue, not perf, and it's shutdown only, so I'm good with whatever you want.
https://reviewboard.mozilla.org/r/25575/#review23059

> I realize you're being cautious here, but release asserts to catch GetContentChildShutdown succeeding with a null shutdownPhase (in release) seems like overkill.  However, it's really just a code-size issue, not perf, and it's shutdown only, so I'm good with whatever you want.

That was already there in the previous versiou, so I'm fine with the regular `MOZ_ASSERT`.
Attachment #8689399 - Flags: review?(nfroyd) → review+
Comment on attachment 8689399 [details]
MozReview Request: Bug 1216972 - OS.File AsyncShutdown for content processes;r?froydnj

https://reviewboard.mozilla.org/r/25623/#review23127
are the try failures in mozreview related to this changes ?
Flags: needinfo?(dteller)
Comment on attachment 8689208 [details]
MozReview Request: Bug 1216972 - MediaManager AsyncShutdown for content processes;r?jesup

Approval Request Comment: See bug 1228064 comment 7.

[Risks and why]: We believe the 5 patches (Bug 1227407, Bug 1216972 and Bug 1218799, as outlined in bug 1228064 comment 5) collectively fix the 30 second hang, debug assert-crash and leak that plagued the new shutdown behavior in trunk for a while, and that aurora is better off using this approach than what it currently has, which exhibits at least the debug crash, maybe more. Worst case it still crashes on shutdown, but it is much less likely with these patches, and we have not seen any sign of trouble with them.

3 of the patches are in this bug.
Attachment #8689208 - Flags: approval-mozilla-aurora?
Attachment #8676810 - Flags: approval-mozilla-aurora?
Attachment #8689399 - Flags: approval-mozilla-aurora?
Comment on attachment 8676810 [details]
MozReview Request: Bug 1216972 - AsyncShutdown for content processes;r=froydnj

Jib and I discussed these patches and he is pretty confident we have the right set of patches to fix the regressions in 44 that include memleaks, crash, hang. Aurora44+
Attachment #8676810 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8689208 [details]
MozReview Request: Bug 1216972 - MediaManager AsyncShutdown for content processes;r?jesup

Aurora44+
Attachment #8689208 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment on attachment 8689399 [details]
MozReview Request: Bug 1216972 - OS.File AsyncShutdown for content processes;r?froydnj

Aurora44+
Attachment #8689399 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Tracking to ensure all the patches once uplifted make all the problems mentioned in comment 27 go away.
You need to log in before you can comment on or make changes to this bug.