Closed Bug 1121713 Opened 9 years ago Closed 9 years ago

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


(Core :: Graphics: Layers, defect)

Not set



Tracking Status
e10s + ---


(Reporter: billm, Assigned: billm)



(1 file, 1 obsolete file)

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.
Attached 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
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]

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!
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla38
You need to log in before you can comment on or make changes to this bug.