1282343 - Clean up CompositorSession shutdown

Status: RESOLVED FIXED

(Core :: Graphics, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

We don't ever explicitly release the last reference to CompositorBridgeParent - either nsBaseWidget will do it via its destructor, or CompositorBridgeParent::DeferredDestroy will do it if ~nsBaseWidget runs first.

I'd like to make this more explicit so it's totally clear that the compositor is no longer in use.

Comment 1

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, revoke all compositor pointers

This spells out the behavior of how CompositorBridgeParent is destroyed, from the point of view of the CompositorSession. It also makes sure to null out any CompositorSession-related pointers in nsBaseWidget::DestroyCompositor().

Comment 2

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 8765314 [details] [diff] [review]
part 1, revoke all compositor pointers

kats, to make sure nulling out mAPZC is okay here.

Comment 3

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 2, revoke compositor widget

Technically the compositor can still access the widget even though it may be gone. I don't think this actually happens, but we might as well null this out to be sure.

Comment 4

[David Anderson [:dvander] - inactive, e-mail if emergency]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0cc51d99571a5cec016f5bffb1709228a736395

Comment 5

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8765314 [details] [diff] [review]
part 1, revoke all compositor pointers

Review of attachment 8765314 [details] [diff] [review]:
-----------------------------------------------------------------

I'm not sure, to be honest. I read through my notes from the last time I had to go through the widget/compositor shutdown code [1] but I'm not sure when CompositorSession::Shutdown fits in that process. However, if it makes sense to do so, I would like to have mAPZC nulled out in the same place that we destroy the mRootContentController - so either do the nulling out in nsBaseWidget::OnDestroy, or move the existing mRootContentController code to nsBaseWidget::DestroyCompositor. I suspect the former is safer.

Also please check that opening and closing windows without shutting down the whole browser doesn't leak stuff (i.e. bug 1264161 doesn't regress) because we don't have any automated coverage for that scenario.

[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1264161#c25

Comment 6

[David Anderson [:dvander] - inactive, e-mail if emergency]

Attachment: part 1, revoke all compositor pointers v2

kats, this patch factors the content controller step into a separate method, and calls it in both places to be safe. I also cleaned up the fail case of CreateCompositor() and added something of a comment explaining the shutdown sequence.

I'll test on OS X tonight to make sure that memory leak hasn't regressed.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e7f191fbe56489cf40d8e57d31495a5ea8acf21f

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8765666 [details] [diff] [review]
part 1, revoke all compositor pointers v2

Review of attachment 8765666 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, the comment is very useful!

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0ac0b0b0968d
Don't leave temporary dangling nsWindow references to CompositorBridgeParent. (bug 1282343 part 1, r=mattwoodrow,kats)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8896a11ad540
Revoke widget pointer from CompositorBridgeParent on shutdown. (bug 1282343 part 2, r=mattwoodrow)

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0ac0b0b0968d
https://hg.mozilla.org/mozilla-central/rev/8896a11ad540