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

RESOLVED FIXED in mozilla38

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: billm, Assigned: billm)

Tracking

unspecified
mozilla38
x86_64
Linux
Points:
---

Firefox Tracking Flags

(e10s+)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Posted patch compositor-fix (obsolete) — Splinter Review
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.
Assignee: nobody → wmccloskey
Status: NEW → ASSIGNED
Attachment #8549210 - Flags: review?(nical.bugzilla)
Wonder if this is the same root cause as bug 1120331 and bug 1120485, though I appreciate the stacks are not necessarily the same...
Flags: needinfo?(nical.bugzilla)
Comment on attachment 8549210 [details] [diff] [review]
compositor-fix

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

Did you attach the correct patch?
No I did not.
Attachment #8549210 - Attachment is obsolete: true
Attachment #8549210 - Flags: review?(nical.bugzilla)
Attachment #8549214 - Flags: review?(nical.bugzilla)
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).
Attachment #8549214 - Flags: review?(nical.bugzilla) → review+
(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.
Flags: needinfo?(nical.bugzilla)
On further thought, b2g doesn't have multiple OS windows, so I don't think this is security sensitive.
Group: core-security
Ooh, let's backport this as far as possible!
https://hg.mozilla.org/mozilla-central/rev/dff2539946b2
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.