885580 - Compositor::sBackend is unitialized in child processes

Status: RESOLVED FIXED

(Core :: Graphics: Layers, defect)

Description

[Matt Woodrow (:mattwoodrow)]

This causes a crash if you try look at about:support with e10s enabled.

Comment 1

[Matt Woodrow (:mattwoodrow)]

Attachment: Initialise sBackend when we get IdentifyTextureHost

Comment 2

[Nicolas Silva [:nical]]

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?

Comment 3

[Matt Woodrow (:mattwoodrow)]

(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.

Comment 4

[Matt Woodrow (:mattwoodrow)]

Attachment: Only use Compositor::GetBackend from the compositor thread

Comment 5

[Matt Woodrow (:mattwoodrow)]

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.

Comment 6

[Phil Ringnalda (:philor)]

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 7

[Nicolas Silva [:nical]]

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).

Comment 8

[Matt Woodrow (:mattwoodrow)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e969b1777a7

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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