David Teller [:Yoric] - still alive but not very active Assignee
	Description • 9 years ago

      No description provided.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 1 • 9 years ago

Bug 1216972 - AsyncShutdown for content processes;r=froydnj

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 2 • 9 years ago

https://reviewboard.mozilla.org/r/22811/#review20273

This patch was reviewed in bug 1089695 a long time ago.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 3 • 9 years ago

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

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

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 4 • 9 years ago

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

Bug 1216972 - AsyncShutdown for content processes;r=froydnj

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 5 • 9 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=36d03134218f

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 6 • 9 years ago

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

Bug 1216972 - AsyncShutdown for content processes;r=froydnj

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 7 • 9 years ago

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

Bug 1216972 - AsyncShutdown for content processes;r?froydnj

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 8 • 9 years ago

Removing checkin-needed as Nathan can't wait to re-review that code :)

	Nathan Froyd [:froydnj]
	Comment 9 • 9 years ago

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?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 10 • 9 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 11 • 9 years ago

I haven't been able to land this because for some reason it breaks a add-on manager test.

	Vladan Djeric (:vladan)
	Comment 12 • 9 years ago

This is not a measurement issue, but rather a correctness fix

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 13 • 9 years ago

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/

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 14 • 9 years ago

Bug 1216972 - OS.File AsyncShutdown for content processes;r?froydnj

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 15 • 9 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a64c1908cfa4

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 16 • 9 years ago

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ee4f83cde04

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 17 • 9 years ago

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/

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 18 • 9 years ago

Bug 1216972 - OS.File AsyncShutdown for content processes;r?froydnj

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 19 • 9 years ago

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/

	Randell Jesup [:jesup] (needinfo me)
	Comment 20 • 9 years ago

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.

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 21 • 9 years ago

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`.

	Nathan Froyd [:froydnj]
	Comment 22 • 9 years ago

Comment on attachment 8689399 [details]
MozReview Request: Bug 1216972 - OS.File AsyncShutdown for content processes;r?froydnj

https://reviewboard.mozilla.org/r/25623/#review23127

	Carsten Book [:Tomcat]
	Comment 23 • 9 years ago

are the try failures in mozreview related to this changes ?

	David Teller [:Yoric] - still alive but not very active Assignee
	Comment 24 • 9 years ago

I see only one orange on https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ee4f83cde04&selectedJob=13907668, and it seems to be bug 1097949.

	Pulsebot
	Comment 25 • 9 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

	Carsten Book [:Tomcat]
	Comment 26 • 9 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

	Jan-Ivar Bruaroey [:jib] (needinfo? me)
	Comment 27 • 9 years ago

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.

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 28 • 9 years ago

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+

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 29 • 9 years ago

Comment on attachment 8689208 [details]
MozReview Request: Bug 1216972 - MediaManager AsyncShutdown for content processes;r?jesup

Aurora44+

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 30 • 9 years ago

Comment on attachment 8689399 [details]
MozReview Request: Bug 1216972 - OS.File AsyncShutdown for content processes;r?froydnj

Aurora44+

	Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)
	Comment 31 • 9 years ago

Tracking to ensure all the patches once uplifted make all the problems mentioned in comment 27 go away.

	Carsten Book [:Tomcat]
	Comment 32 • 9 years ago
bugherder uplift

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

	Carsten Book [:Tomcat]
	Comment 33 • 9 years ago
bugherder uplift

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