717925 - OMTC: Compositor's access to widget should be thread-safe

Status: RESOLVED INCOMPLETE

(Core :: Graphics, defect, P3)

Description

[Ali Juma [:ajuma]]

With OMTC and GL layers, LayerManagerOGL lives on the compositor thread but needs access to the widget. Currently, LayerManagerOGL makes the following calls to the widget:

  mWidget->GetClientBounds(rect);
  mWidget->DrawOver(this, rect);
  mWidget->GetBounds(rect);

This use of the widget is not thread-safe.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Another (related) potential source of thread un-safety is if the nsIWidget's native widget is resized, and we need to recreate or resize the GL context wrapping the native widget.  Might be worth dealing with in a separate bug.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Oops, that's bug 717938 for mac.  Ignore me.

Comment 3

[Joe Drew (not getting mail)]

Does this need to block bug 725095?

Comment 4

[Ali Juma [:ajuma]]

(In reply to Joe Drew (:JOEDREW!) from comment #3)
> Does this need to block bug 725095?

Yes.

Comment 5

[Benoit Girard (:BenWa)]

This doesn't block landing on central but we need to fix this ASAP.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Who's going to work on this?

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Is this relevant anymore? If we haven't had this in the code for the few weeks OMTC has been landed, why do we need it? Can we WONTFIX?

Comment 8

[Ali Juma [:ajuma]]

(In reply to Mark Finkle (:mfinkle) from comment #7)
> Is this relevant anymore? If we haven't had this in the code for the few
> weeks OMTC has been landed, why do we need it? Can we WONTFIX?

Though we haven't seen any problems, it's still something we should eventually fix, particularly for OMTC on desktop. However, it seems reasonable to no longer block Fennec on this bug.

Comment 9

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We really should fix this. this is the kind of defect that can lead to sg:crit kind of bugs.

Comment 10

[Ali Juma [:ajuma]]

Let's fix this on Android first. The calls in question are:

    mWidget->GetClientBounds(rect);
    mWidget->DrawWindowUnderlay(this, rect);
    mWidget->DrawWindowOverlay(this, rect);
    mWidget->GetBounds(rect);

On Android, DrawWindowUnderlay and DrawWindowOverlay were written with the knowledge that they'd be called off the main thread, and there doesn't appear to be anything more needed for these calls.

GetClientBounds just calls GetBounds.

So that leaves us with making GetBounds thread-safe. We can accomplish this by having nsWindow override nsBaseWidget's implementations of GetBounds and SetBounds with implementations that guard access to mBounds using a mutex, and then replacing all direct modifications to mBounds with calls to SetBounds.

Comment 11

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Does the compositor hold a strong reference to the nsIWidget, or is the lifetime otherwise bounded by the widget?

Comment 12

[Ali Juma [:ajuma]]

The compositor does not hold a strong reference to the widget, but the active lifetime of the compositor's layer manager (which make the calls to the widget) is strictly contained within the lifetime of the widget:

The widget creates the compositor (which in turn creates the layer manager). Destroy() is called on the compositor's layer manager during widget destruction; once Destroy() is called, the layer manager no longer makes any calls to the widget

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

To be clear, let's get this fixed. But since we're not seeing any crashes due to the lack of thread safety, the drivers feel we don't need to hold the beta for this.

Comment 14

[Ali Juma [:ajuma]]

(In reply to Ali Juma [:ajuma] from comment #10)
> So that leaves us with making GetBounds thread-safe.

Since the landing of Bug 733596, the compositor's layer manager no longer calls GetBounds on Android (instead, the compositor receives an event when the surface size changes, and then notifies the layer manager about the new size). That bug also added a call to GetBounds in CompositorParent::AllocPLayers, but Gecko is blocked on the compositor during CompositorParent::AllocPLayers. Since there is nothing left to do for this bug on Android, we should no longer block Fennec on this.

(But we still need to fix this for OMTC on other platforms.)

Comment 15

[Joe Drew (not getting mail)]

OK. No longer affects Android, but still a valid bug.

Comment 16

[Chris Peterson [:cpeterson]]

Mass tracking-e10s flag change. Filter bugmail on "2be0fcce-e36a-4e2c-aa80-0e3d33eb5406".

Comment 17

[Jesse Ruderman]

What platforms does this affect?

Comment 18

[Liz Henry (:lizzard) (privacy/fingerprinting team)]

Chris or Ali, do you know which platforms this affects?

Comment 19

[Ali Juma [:ajuma]]

This potentially affects all platforms with OMTC (which, I believe, is now all platforms).

I took a quick glance at CompositorOGL.cpp and it looks like the unsafe access to mWidget is gone there now (except for a call within "#ifdef MOZ_DUMP_PAINTING"), but someone who's worked with that code recently should verify.

BasicCompositor.cpp still has calls to mWidget->GetClientBounds, as does CompositorD3D11.cpp.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

I haven't been deep in this code for about a year and a half, so I'm not so sure.  At least on b2g and android, there were some pretty deep assumptions made historically around there being a single widget that basically never died.  I don't know if that's changed.  I hear omtc is enabled by default on OS X, so the situation must have improved, because widgets definitely churn there.

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

In the last few weeks I've had the displeasure of getting back into this code again, and I can re-confirm that the deep assumptions about there only being one immortal widget are still deeply baked in to the full complement of features used by gonk and android.  I suspect the full complement of features aren't used on other platforms.  There are also optional nsIWidget APIs that result in nsIWidget's being used on multiple threads when omtc is enabled.  I suspect the implementations of these APIs aren't aware of that and happen to mostly work incidentally.

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 24 weeks.

Comment 23

[Mayank Bansal]

OMTC is long gone.