AsyncShutdown for content processes

RESOLVED FIXED in Firefox 44, Firefox OS v2.5

Status

()

Toolkit
Async Tooling
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: Yoric, Assigned: Yoric)

Tracking

(Blocks: 1 bug)

unspecified
mozilla45
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox44+ fixed, firefox45 fixed, b2g-v2.5 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(3 attachments)

Comment hidden (empty)
Created attachment 8676810 [details]
MozReview Request: Bug 1216972 - AsyncShutdown for content processes;r=froydnj

Bug 1216972 - AsyncShutdown for content processes;r=froydnj
https://reviewboard.mozilla.org/r/22811/#review20273

This patch was reviewed in bug 1089695 a long time ago.
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
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36d03134218f
Keywords: checkin-needed
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.
Blocks: 1222849
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: 1222849
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/
Created attachment 8689208 [details]
MozReview Request: Bug 1216972 - MediaManager AsyncShutdown for content processes;r?jesup

Bug 1216972 - OS.File AsyncShutdown for content processes;r?froydnj
Attachment #8689208 - Flags: review?(nfroyd)
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a64c1908cfa4
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ee4f83cde04
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/
Created attachment 8689399 [details]
MozReview Request: Bug 1216972 - OS.File AsyncShutdown for content processes;r?froydnj

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/

Updated

2 years ago
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
Keywords: checkin-needed
are the try failures in mozreview related to this changes ?
Flags: needinfo?(dteller)
I see only one orange on https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ee4f83cde04&selectedJob=13907668, and it seems to be bug 1097949.
Flags: needinfo?(dteller)

Comment 25

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/504bd9c5e9a9
https://hg.mozilla.org/integration/fx-team/rev/657753f40cc7
https://hg.mozilla.org/integration/fx-team/rev/e799e37d4bd2
Keywords: checkin-needed

Comment 26

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/504bd9c5e9a9
https://hg.mozilla.org/mozilla-central/rev/657753f40cc7
https://hg.mozilla.org/mozilla-central/rev/e799e37d4bd2
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Blocks: 1229413
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?

Updated

2 years ago
status-firefox44: --- → affected
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.
tracking-firefox44: --- → +

Comment 32

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-aurora/rev/9a72cc443356
https://hg.mozilla.org/releases/mozilla-aurora/rev/af103a570400
https://hg.mozilla.org/releases/mozilla-aurora/rev/94fb54fd5330
status-firefox44: affected → fixed

Comment 33

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/9a72cc443356
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/af103a570400
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/94fb54fd5330
status-b2g-v2.5: --- → fixed
Depends on: 1270308
You need to log in before you can comment on or make changes to this bug.