Closed Bug 885580 Opened 6 years ago Closed 6 years ago

Compositor::sBackend is unitialized in child processes

Categories

(Core :: Graphics: Layers, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla25

People

(Reporter: mattwoodrow, Unassigned)

References

Details

(Whiteboard: [e10s])

Attachments

(1 file, 1 obsolete file)

This causes a crash if you try look at about:support with e10s enabled.
Attachment #765670 - Flags: review?(nical.bugzilla)
Comment on attachment 765670 [details] [diff] [review]
Initialise sBackend when we get IdentifyTextureHost

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

sBackend was previously intended for use on the parent side only. With your patch it is set on the child side, and we still use it on the Compositor side. On a cross process setup it may not work (well, right now we only have one supported cross-process platform which always use OpenGL so i wouldn't be surprised that it actually still works but it is not correct). on the cross-thread scenario it looks prone to race conditions. How about we have two globals (one for the parent and one for the child) which are set at the appropriate time on the appropriate thread, and are used only in their respective thread/process?
Attachment #765670 - Flags: review?(nical.bugzilla)
(In reply to Nicolas Silva [:nical] from comment #2)
> Comment on attachment 765670 [details] [diff] [review]
> Initialise sBackend when we get IdentifyTextureHost
> 
> Review of attachment 765670 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> sBackend was previously intended for use on the parent side only. With your
> patch it is set on the child side, and we still use it on the Compositor
> side. On a cross process setup it may not work (well, right now we only have
> one supported cross-process platform which always use OpenGL so i wouldn't
> be surprised that it actually still works but it is not correct). on the
> cross-thread scenario it looks prone to race conditions. How about we have
> two globals (one for the parent and one for the child) which are set at the
> appropriate time on the appropriate thread, and are used only in their
> respective thread/process?

Ah! Unfortunately, I didn't know this, and have been adding main thread callers to it.

Lets just stop doing that, and add a comment/assertion so that it doesn't regress again.
Attachment #765670 - Attachment is obsolete: true
Attachment #767106 - Flags: review?(nical.bugzilla)
https://hg.mozilla.org/integration/mozilla-inbound/rev/37c7fcfdb186

Crap, for some reason I thought you'd reviewed this.

I'll back it out tomorrow if you don't like it.
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/607b8e1da1ca - there were only two csets in that push, linux32 Cipc started timing out, I backed out the other, and it kept timing out. The backouts will continue until morale improves.
Comment on attachment 767106 [details] [diff] [review]
Only use Compositor::GetBackend from the compositor thread

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

Cool thanks, this assumption should have been documented before (my bad).
Attachment #767106 - Flags: review?(nical.bugzilla) → review+
https://hg.mozilla.org/mozilla-central/rev/dabdd7ceb43e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25
You need to log in before you can comment on or make changes to this bug.