[GTK3] Electrolysis - nsWindow::StartRemoteDrawing() needs a cairo surface

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: kxra, Assigned: stransky)

Tracking

31 Branch
mozilla32
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Reporter

Description

5 years ago
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140428123133

Steps to reproduce:

In Firefox-GTK3 versions 28 or 29, go to about:config and set the following to 'true'

browser.tabs.remote

Restarting the browser will show a completely empty window (even tab bar isn't shown)


In Firefox-GTK3 version 30 and 31, I set the following to true as well for the same result: 

browser.tabs.remote.autostart

IF I do not set autostart to true, and instead only leave 

the original 'browser.tabs.remote' set to 'true', and then go to File -> New e10s window, then I get an error indicating that I must enable the following: 

layers.offmainthreadcomposition.enabled

After doing so, I still get an empty window, although the first time I was able to at least see the tab bar (and not one time since then). 



Actual results:

I got an empty window (obviously unusable)


Expected results:

I should've gotten a normally-functioning, although perhaps less stable window.
Reporter

Updated

5 years ago
Blocks: gtk3
Reporter

Comment 1

5 years ago
I should mention that this has been the case for the aferomentioned versions of Firefox-GTK3 on Fedora 20 and 21 (Rawhide), both 64bit
Does enabling |layers.offmainthreadcomposition.enabled| without either browser.tabs.remove pref still cause a blank window?
(In reply to John Schoenick [:johns] from comment #2)
> Does enabling |layers.offmainthreadcomposition.enabled| without either
> browser.tabs.remove pref still cause a blank window?

browser.tabs.remote*
Reporter

Comment 4

5 years ago
I am no longer running one of the older versions of Firefox-GTK3 in which browser.tabs.remote affects the main browser window, so I believe the short answer is no. 

In Firefox-GTK3 30 & 31, I either have to set browser.tabs.remote.autostart to 'true' and restart the browser OR specifically go to File -> New e10s window to get a blank window.
Assignee

Comment 5

5 years ago
I can reproduce it on latest trunk & gtk3 build. Looks like a gfx issue - a webpage is loaded but not rendered.
Assignee

Comment 6

5 years ago
It's because nsWindow::StartRemoteDrawing() does not provide a cairo surface for drawing - the cairo surface is provided by expose event in gtk3. So we may need to draw to some buffer and update actual window surface on the expose event. (It also affects Bug 991272).
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee

Updated

5 years ago
Component: Untriaged → Widget: Gtk
Product: Firefox → Core
Assignee

Updated

5 years ago
Summary: Electrolysis (separate processes per tab) breaks Firefox-GTK3 (empty window) → [GTK3] Electrolysis - nsWindow::StartRemoteDrawing() needs a cairo surface
Assignee

Comment 7

5 years ago
Posted patch patch (obsolete) — Splinter Review
What about this one? It creates a xlib surface when cr is not passed to nsWindow::GetThebesSurface().
Attachment #8427791 - Flags: feedback?(karlt)
Reporter

Updated

5 years ago
Blocks: e10s
Reporter

Updated

5 years ago
Assignee

Updated

5 years ago
Duplicate of this bug: 991272
Comment on attachment 8427791 [details] [diff] [review]
patch

> gfxASurface*
> #if (MOZ_WIDGET_GTK == 2)
> nsWindow::GetThebesSurface()
> #else
>+nsWindow::GetThebesSurface()
>+{
>+    return GetThebesSurface(nullptr);
>+}
>+gfxASurface*
> nsWindow::GetThebesSurface(cairo_t *cr)
> #endif
> {

The two nsWindow::GetThebesSurface() lines can be merged and so there would just be a "#if (MOZ_WIDGET_GTK > 2)" preprocessor conditional block.

>@@ -6092,18 +6084,31 @@ nsWindow::GetThebesSurface(cairo_t *cr)
>     mThebesSurface = new gfxXlibSurface
>         (GDK_WINDOW_XDISPLAY(mGdkWindow),
>          gdk_x11_window_get_xid(mGdkWindow),
>          visual,
>          size);
> #else
> #if MOZ_TREE_CAIRO
> #error "cairo-gtk3 target must be built with --enable-system-cairo"
>-#else
>-    mThebesSurface = gfxASurface::Wrap(surf);
>+#else    
>+    if (cr) {
>+        cairo_surface_t *surf = cairo_get_target(cr);
>+        if (cairo_surface_status(surf) != CAIRO_STATUS_SUCCESS) {
>+          NS_NOTREACHED("Missing cairo target?");
>+          return nullptr;
>+        }
>+        mThebesSurface = gfxASurface::Wrap(surf);
>+    } else {
>+        mThebesSurface = new gfxXlibSurface
>+            (GDK_WINDOW_XDISPLAY(mGdkWindow),
>+             gdk_x11_window_get_xid(mGdkWindow),
>+             visual,
>+             size);    
>+    }

Here also, the mThebesSurface = new gfxXlibSurface code is equivalent for both
GTK versions.  The (MOZ_WIDGET_GTK == 2) block can be removed by changing its
#else to "#if (MOZ_WIDGET_GTK > 2)" and ending that block after the "} else"
after the "if (cr)" block.

>-#if (MOZ_WIDGET_GTK == 2)
>     gfxASurface       *GetThebesSurface();
> 
>+#if (MOZ_WIDGET_GTK == 2)
>     static already_AddRefed<gfxASurface> GetSurfaceForGdkDrawable(GdkDrawable* aDrawable,
>                                                                   const nsIntSize& aSize);
> #else
>     gfxASurface       *GetThebesSurface(cairo_t *cr);

If you are able to make the first declaration

    virtual gfxASurface *GetThebesSurface() MOZ_OVERRIDE;

it would make it clearer that it is the public method.
Actually, GetThebesSurface(cairo_t *cr) should move to the private
declarations too.
Attachment #8427791 - Flags: review+
Attachment #8427791 - Flags: feedback?(karlt)
Attachment #8427791 - Flags: feedback?
Attachment #8427791 - Flags: feedback?
No longer blocks: e10s-m1
No longer blocks: old-e10s-m2
Assignee

Comment 10

5 years ago
Posted patch patch, v.2Splinter Review
An updated one, Thanks!
Assignee: nobody → stransky
Attachment #8432421 - Flags: review?(karlt)
Comment on attachment 8432421 [details] [diff] [review]
patch, v.2

Thank you!
Attachment #8432421 - Flags: review?(karlt) → review+
Assignee

Updated

5 years ago
Keywords: checkin-needed
Duplicate of this bug: 1015211
Attachment #8427791 - Attachment is obsolete: true
Hey Martin, do you have a link to a recent Try run for this? We're requiring that for all checkin-needed bugs to avoid bustage pileups. Feel free to ping me for assistance if you don't have access to Try. Otherwise, the link below has some suggestions for what to run.
https://wiki.mozilla.org/Sheriffing/How:To:Recommended_Try_Practices
Keywords: checkin-needed

Updated

5 years ago
Duplicate of this bug: 1022135
https://hg.mozilla.org/mozilla-central/rev/bb188e9fcdcc
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32

Updated

5 years ago
Blocks: 1034064

Updated

5 years ago
No longer blocks: 1034064
You need to log in before you can comment on or make changes to this bug.