1121713 - [e10s] CompositorParent doesn't seem to clear itself out of LayerTreeState when destroyed

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

I've been trying to debug bug 1120898 and I think I found a problem in how the cross process compositors are managed. The test in that bug opens and closes windows very quickly. Here's what I see happening:

1. Window is closed. The widget is torn down along with the CompositorChild and CompositorParent for that window.
2. The child process sends up a ForceComposite message.
3. The parent receives the ForceComposite message and it's routed to CrossProcessCompositorParent::ForceComposite. That code finds the CompositorParent that was deleted in step 1 and then tries to use it.

I don't see anything that is designed to prevent this from happening. I think the only reason my patch in bug 567058 triggered this is that it made it possible to create windows a little faster in e10s.

Marking s-s in case this also affects b2g.

Comment 1

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: compositor-fix

This fixes the immediate problem. However, I'm a little worried about the other places where we do MOZ_ASSERT(state->mParent). I left them alone, but we might want to do null checks there instead.

Comment 2

[Milan Sreckovic [:milan] (needinfo for best results)]

Wonder if this is the same root cause as bug 1120331 and bug 1120485, though I appreciate the stacks are not necessarily the same...

Comment 3

[Milan Sreckovic [:milan] (needinfo for best results)]

Comment on attachment 8549210 [details] [diff] [review]
compositor-fix

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

Did you attach the correct patch?

Comment 4

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: compositor-fix v2

No I did not.

Comment 5

[Nicolas Silva [:nical]]

Comment on attachment 8549214 [details] [diff] [review]
compositor-fix v2

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

Not sure about the other assertions you mentionned. Ideally we'd not have the possibility to shut a compositor down and still try to send updates to it (rather than null checking when it happens).

Comment 6

[Nicolas Silva [:nical]]

(In reply to Milan Sreckovic [:milan] from comment #2)
> Wonder if this is the same root cause as bug 1120331 and bug 1120485, though
> I appreciate the stacks are not necessarily the same...

I think that they are different things.

Comment 7

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

On further thought, b2g doesn't have multiple OS windows, so I don't think this is security sensitive.

Comment 8

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dff2539946b2

Comment 9

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Ooh, let's backport this as far as possible!

Comment 10

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/dff2539946b2