Closed Bug 1205045 Opened 4 years ago Closed 4 years ago

GTK backend makes GTK calls off the main thread when OMTC is used

Categories

(Core :: Widget: Gtk, defect)

All
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla43
Tracking Status
firefox43 --- fixed

People

(Reporter: lsalzman, Assigned: lsalzman)

References

Details

Attachments

(1 file)

When BasicCompositor/OMTC is used in combination with the GTK backend, various thread-unsafe GTK calls are made on the compositor thread inside nsWindow::StartRemoteDrawing and nsWindow::EndRemoteDrawing.
This is a multi-pronged assault on thread-unsafety in BasicCompositor, nsShmImage, and nsWindow.

Some small changes inside BasicCompositor to ensure it is passing the same region/size info it internally uses throughout the entire composing process to the widget backend, so that they better agree on what size draw target they are working with.

Significant changes to nsShmImage to both get rid of gdk calls (replacing them with raw X calls) and working directly with DrawTargets instead of gfxASurfaces.

In nsWindow, getting rid of the obsolete GetThebesSurface() interface and replacing it with a more succinct GetDrawTarget(), as well making sure to get the underlying X drawable information when the window is created, so that we don't need to unsafely query this information from GTK when compositing is occurring.
Attachment #8661448 - Flags: review?(jmuizelaar)
OS: Unspecified → Linux
Hardware: Unspecified → All
Comment on attachment 8661448 [details] [diff] [review]
Remove GTK calls from compositor thread

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

::: widget/gtk/nsWindow.cpp
@@ +6338,5 @@
> +  if (!dt) {
> +    RefPtr<gfxXlibSurface> surf = new gfxXlibSurface(mXDisplay, mXWindow, mXVisual, size);
> +    if (!surf->CairoStatus()) {
> +      dt = gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(surf.get(), surf->GetSize());
> +    }

Can we just create the Cairo draw target around the mXDisplay, mXWindow, mXVisual, size instead of also creating a thebes surface?
Attachment #8661448 - Flags: review?(jmuizelaar) → review+
(In reply to Jeff Muizelaar [:jrmuizel] from comment #2)
> Comment on attachment 8661448 [details] [diff] [review]
> Remove GTK calls from compositor thread
> 
> Review of attachment 8661448 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: widget/gtk/nsWindow.cpp
> @@ +6338,5 @@
> > +  if (!dt) {
> > +    RefPtr<gfxXlibSurface> surf = new gfxXlibSurface(mXDisplay, mXWindow, mXVisual, size);
> > +    if (!surf->CairoStatus()) {
> > +      dt = gfxPlatform::GetPlatform()->CreateDrawTargetForSurface(surf.get(), surf->GetSize());
> > +    }
> 
> Can we just create the Cairo draw target around the mXDisplay, mXWindow,
> mXVisual, size instead of also creating a thebes surface?

The problem is that the only other way we can do that is by directly using cairo_xlib_surface_create. If used within the widget/gtk code that will cause us to use system cairo to do it. With gfxXlibSurface we will get a tree cairo xlib surface, which is much safer for now.
https://hg.mozilla.org/mozilla-central/rev/3d9840db7d35
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
See Also: → 1249813
You need to log in before you can comment on or make changes to this bug.