Linux: move X operations from the IO to UI2 thread.

Linux: move X operations from the IO to UI2 thread.

Currently we perform several X operations on the IO thread including
geometry and clipboard work. This is causing races inside Xlib and
crashing the browser.

These are the result of synchronous calls from the renderer, so we
cannot route these requests to the UI thread without risking deadlock.
Thus we introduce the UI2 thread. This thread has a second connection
to the X server and can perform X operations safely the without UI
thread.

Work remains to be done:

Since we still have the hack where we pass GtkWidget pointers into the
renderer and back, we still have to access these structures from the
IO and UI2 threads. This still needs to be fixed, but this is not the
patch for it.

Also, not all the X calls from the IO thread have been moved over in
this patch; just a few small ones.

[agl, 2009-04-14 22:46:30]

There's also a WebKit side to this patch. If this CL looks to be on the right
path, I'll file a bug for this patch too:


Index: WebKit/chromium/src/gtk/WebScreenInfoFactory.cpp
===================================================================
--- WebKit/chromium/src/gtk/WebScreenInfoFactory.cpp	(revision 13628)
+++ WebKit/chromium/src/gtk/WebScreenInfoFactory.cpp	(working copy)
@@ -31,10 +31,13 @@
 #include "config.h"
 #include "WebScreenInfoFactory.h"
 
-#include <gtk/gtk.h>
+#include <X11/Xlib.h>
+#include <gdk/gdkx.h>
 
 #include "WebScreenInfo.h"
 
+extern Display* kSecondaryXConnection;
+
 namespace WebKit {
 
 WebScreenInfo WebScreenInfoFactory::screenInfo(GtkWidget* widget)
@@ -44,14 +47,12 @@
     results.depthPerComponent = 8;
     results.isMonochrome = false;
 
-    if (!widget)
-        return results;
+    int defaultScreen = XDefaultScreen(kSecondaryXConnection);
+    int displayWidth = XDisplayWidth(kSecondaryXConnection, defaultScreen);
+    int displayHeight = XDisplayHeight(kSecondaryXConnection, defaultScreen);
 
-    GdkScreen* screen = gtk_widget_get_screen(widget);
+    results.rect = WebRect(0, 0, displayWidth, displayHeight);
 
-    results.rect = WebRect(
-        0, 0, gdk_screen_get_width(screen), gdk_screen_get_height(screen));
-
     // I don't know of a way to query the "maximize" size of the window (e.g.
     // screen size less sidebars etc) since this is something which only the
     // window manager knows.

[darin (slow to review), 2009-04-15 16:52:39]

> +extern Display* kSecondaryXConnection;

This is not going to work.  We want to be able to compile and link WebKit as a
separate DSO.  I think instead you should pass a Display pointer to the
screenInfo function.  (Each port can define the inputs to the screenInfo
function.)  Actually, why is defaultScreen correct?  The goal of this screenInfo
function is to return information for the screen associated with the given
widget.

[darin (slow to review), 2009-04-15 17:08:49]

http://codereview.chromium.org/67145/diff/1001/16
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/67145/diff/1001/16#newcode51
Line 51: #include
"third_party/WebKit/WebKit/chromium/public/gtk/WebScreenInfoFactory.h"
looks like this include can be removed.

http://codereview.chromium.org/67145/diff/1001/18
File chrome/browser/renderer_host/resource_message_filter_gtk.cc (right):

http://codereview.chromium.org/67145/diff/1001/18#newcode25
Line 25: Send(reply_msg);
it is not safe to call Send from the UI2 thread!  i think you need to PostTask
back to the IO thread in order to call Send.

http://codereview.chromium.org/67145/diff/1001/21
File chrome/common/x11_util.cc (right):

http://codereview.chromium.org/67145/diff/1001/21#newcode231
Line 231: static base::Thread* ui2_thread = NULL;
why not add this thread to chrome_thread.{h,cc}?

any reason not to have this thread managed by the BrowserProcessImpl just like
the rest of the threads?  that way you can ensure that it closes at a good time.

related to that:  perhaps it would be good to ensure that the UI2 thread goes
away before the IO thread.  that way the UI2 thread can always assume that
resources from the IO thread remain valid for the lifetime of the UI2 thread.

http://codereview.chromium.org/67145/diff/1001/22
File chrome/common/x11_util.h (right):

http://codereview.chromium.org/67145/diff/1001/22#newcode31
Line 31: namespace x11_util {
btw: per google style, the contents of a namespace are not supposed to be
indented.

[agl, 2009-04-15 19:15:31]

If this looks better I'll make the bugs.webkit.org entry.

http://codereview.chromium.org/67145/diff/1001/16
File chrome/browser/renderer_host/resource_message_filter.cc (right):

http://codereview.chromium.org/67145/diff/1001/16#newcode51
Line 51: #include
"third_party/WebKit/WebKit/chromium/public/gtk/WebScreenInfoFactory.h"
On 2009/04/15 17:08:50, darin wrote:
> looks like this include can be removed.

Done.

http://codereview.chromium.org/67145/diff/1001/18
File chrome/browser/renderer_host/resource_message_filter_gtk.cc (right):

http://codereview.chromium.org/67145/diff/1001/18#newcode25
Line 25: Send(reply_msg);
On 2009/04/15 17:08:50, darin wrote:
> it is not safe to call Send from the UI2 thread!  i think you need to PostTask
> back to the IO thread in order to call Send.

Opps, didn't know that. Thanks!

http://codereview.chromium.org/67145/diff/1001/21
File chrome/common/x11_util.cc (right):

http://codereview.chromium.org/67145/diff/1001/21#newcode231
Line 231: static base::Thread* ui2_thread = NULL;
On 2009/04/15 17:08:50, darin wrote:
> why not add this thread to chrome_thread.{h,cc}?
> 
> any reason not to have this thread managed by the BrowserProcessImpl just like
> the rest of the threads?  that way you can ensure that it closes at a good
time.
> 
> related to that:  perhaps it would be good to ensure that the UI2 thread goes
> away before the IO thread.  that way the UI2 thread can always assume that
> resources from the IO thread remain valid for the lifetime of the UI2 thread.

I did start to do that, but it ended up as a lot of #ifdefs. But you're right
that it would be good to shut them down in the correct order so I've done the
whole chrome_thread, browser_process thing now.

http://codereview.chromium.org/67145/diff/1001/22
File chrome/common/x11_util.h (right):

http://codereview.chromium.org/67145/diff/1001/22#newcode31
Line 31: namespace x11_util {
On 2009/04/15 17:08:50, darin wrote:
> btw: per google style, the contents of a namespace are not supposed to be
> indented.

Done.

[Dean McNamee, 2009-04-16 09:25:12]

http://codereview.chromium.org/67145/diff/27/28
File chrome/browser/browser_process.h (right):

http://codereview.chromium.org/67145/diff/27/28#newcode103
Line 103: // from the UI thread so, if you've ended up here, something has gone
wrong.
typo, did you mean IO or am I confused?

[Dean McNamee, 2009-04-16 09:32:55]

Ok, so.  I see what we're trying to achieve, and understand the issues.  What I
don't understand is why having a second UI thread is guaranteed to fix our
problems if GDK and GTK aren't really inherently thread safe, and now we'll be
using them across the UI and UI2 threads.

Secondly, if the UI2 thread is so tied to the IO thread, which it looks like it
is, I suggest a better name.

IO_UI_THREAD would be good with me.

http://codereview.chromium.org/67145/diff/27/29
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/67145/diff/27/29#newcode175
Line 175: // The UI2 thread must be outlived by the IO thread
I would have worded this the opposite way.

The IO thread must outlive the UI2 thread.

Also, you missed a full stop

http://codereview.chromium.org/67145/diff/27/30
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/67145/diff/27/30#newcode80
Line 80: // IO thread also creates the UI2 thread.
I am probably just dumb, but the way some of this worded I find a bit confusing
and it seems like it could be more straight forward.

http://codereview.chromium.org/67145/diff/27/35
File chrome/browser/renderer_host/resource_message_filter_gtk.cc (right):

http://codereview.chromium.org/67145/diff/27/35#newcode22
Line 22: // Called on the IO thread
full stops

http://codereview.chromium.org/67145/diff/27/35#newcode71
Line 71: gdk_window_get_toplevel(gfx::NativeViewFromId(view)->window);
Is it safe to call GDK on these threads?  Have we verified it doesn't have any
global / singleton state that it might be touching?

http://codereview.chromium.org/67145/diff/27/38
File chrome/common/x11_util.cc (right):

http://codereview.chromium.org/67145/diff/27/38#newcode234
Line 234: // Called on UI2 thread
blah blah full stops

http://codereview.chromium.org/67145/diff/27/39
File chrome/common/x11_util.h (right):

http://codereview.chromium.org/67145/diff/27/39#newcode29
Line 29: extern Display* kSecondaryXConnection;
woah?

[darin (slow to review), 2009-04-16 18:06:07]

http://codereview.chromium.org/67145/diff/27/32
File chrome/browser/chrome_thread.h (right):

http://codereview.chromium.org/67145/diff/27/32#newcode48
Line 48: UI2,
maybe a better name for this would be X2 or BACKGROUND_X11 (longer and more
descriptive is probably better since this thread is rather obscure)

[agl, 2009-04-16 19:56:04]

http://codereview.chromium.org/67145/diff/27/29
File chrome/browser/browser_process_impl.cc (right):

http://codereview.chromium.org/67145/diff/27/29#newcode175
Line 175: // The UI2 thread must be outlived by the IO thread
On 2009/04/16 09:32:55, Dean McNamee wrote:
> I would have worded this the opposite way.
> 
> The IO thread must outlive the UI2 thread.
> 
> Also, you missed a full stop

Done.

http://codereview.chromium.org/67145/diff/27/30
File chrome/browser/browser_process_impl.h (right):

http://codereview.chromium.org/67145/diff/27/30#newcode80
Line 80: // IO thread also creates the UI2 thread.
On 2009/04/16 09:32:55, Dean McNamee wrote:
> I am probably just dumb, but the way some of this worded I find a bit
confusing
> and it seems like it could be more straight forward.

Have reworded it. I don't know if it's better :(

http://codereview.chromium.org/67145/diff/27/32
File chrome/browser/chrome_thread.h (right):

http://codereview.chromium.org/67145/diff/27/32#newcode48
Line 48: UI2,
On 2009/04/16 18:06:08, darin wrote:
> maybe a better name for this would be X2 or BACKGROUND_X11 (longer and more
> descriptive is probably better since this thread is rather obscure)

Renamed to BACKGROUND_X11.

http://codereview.chromium.org/67145/diff/27/35
File chrome/browser/renderer_host/resource_message_filter_gtk.cc (right):

http://codereview.chromium.org/67145/diff/27/35#newcode22
Line 22: // Called on the IO thread
On 2009/04/16 09:32:55, Dean McNamee wrote:
> full stops

I don't generally care for full stops unless it's a long comment with several
sentences. But I don't really care either way so I've added them here.

http://codereview.chromium.org/67145/diff/27/35#newcode71
Line 71: gdk_window_get_toplevel(gfx::NativeViewFromId(view)->window);
On 2009/04/16 09:32:55, Dean McNamee wrote:
> Is it safe to call GDK on these threads?  Have we verified it doesn't have any
> global / singleton state that it might be touching?

No it's not. We have to solve the issues where we pass GtkWidget* into the
renderer and trust them. However, that's a different patch.

(I've thrown in a fixme for now.)

http://codereview.chromium.org/67145/diff/27/38
File chrome/common/x11_util.cc (right):

http://codereview.chromium.org/67145/diff/27/38#newcode234
Line 234: // Called on UI2 thread
On 2009/04/16 09:32:55, Dean McNamee wrote:
> blah blah full stops

Done.

http://codereview.chromium.org/67145/diff/27/39
File chrome/common/x11_util.h (right):

http://codereview.chromium.org/67145/diff/27/39#newcode29
Line 29: extern Display* kSecondaryXConnection;
On 2009/04/16 09:32:55, Dean McNamee wrote:
> woah?

I uploaded too soon; it's just a leftover. Removed.

[Dean McNamee, 2009-04-17 08:34:35]

Did you not upload the new snapshot?

On 2009/04/16 19:56:04, agl wrote:
> http://codereview.chromium.org/67145/diff/27/29
> File chrome/browser/browser_process_impl.cc (right):
> 
> http://codereview.chromium.org/67145/diff/27/29#newcode175
> Line 175: // The UI2 thread must be outlived by the IO thread
> On 2009/04/16 09:32:55, Dean McNamee wrote:
> > I would have worded this the opposite way.
> > 
> > The IO thread must outlive the UI2 thread.
> > 
> > Also, you missed a full stop
> 
> Done.
> 
> http://codereview.chromium.org/67145/diff/27/30
> File chrome/browser/browser_process_impl.h (right):
> 
> http://codereview.chromium.org/67145/diff/27/30#newcode80
> Line 80: // IO thread also creates the UI2 thread.
> On 2009/04/16 09:32:55, Dean McNamee wrote:
> > I am probably just dumb, but the way some of this worded I find a bit
> confusing
> > and it seems like it could be more straight forward.
> 
> Have reworded it. I don't know if it's better :(
> 
> http://codereview.chromium.org/67145/diff/27/32
> File chrome/browser/chrome_thread.h (right):
> 
> http://codereview.chromium.org/67145/diff/27/32#newcode48
> Line 48: UI2,
> On 2009/04/16 18:06:08, darin wrote:
> > maybe a better name for this would be X2 or BACKGROUND_X11 (longer and more
> > descriptive is probably better since this thread is rather obscure)
> 
> Renamed to BACKGROUND_X11.
> 
> http://codereview.chromium.org/67145/diff/27/35
> File chrome/browser/renderer_host/resource_message_filter_gtk.cc (right):
> 
> http://codereview.chromium.org/67145/diff/27/35#newcode22
> Line 22: // Called on the IO thread
> On 2009/04/16 09:32:55, Dean McNamee wrote:
> > full stops
> 
> I don't generally care for full stops unless it's a long comment with several
> sentences. But I don't really care either way so I've added them here.
> 
> http://codereview.chromium.org/67145/diff/27/35#newcode71
> Line 71: gdk_window_get_toplevel(gfx::NativeViewFromId(view)->window);
> On 2009/04/16 09:32:55, Dean McNamee wrote:
> > Is it safe to call GDK on these threads?  Have we verified it doesn't have
any
> > global / singleton state that it might be touching?
> 
> No it's not. We have to solve the issues where we pass GtkWidget* into the
> renderer and trust them. However, that's a different patch.
> 
> (I've thrown in a fixme for now.)
> 
> http://codereview.chromium.org/67145/diff/27/38
> File chrome/common/x11_util.cc (right):
> 
> http://codereview.chromium.org/67145/diff/27/38#newcode234
> Line 234: // Called on UI2 thread
> On 2009/04/16 09:32:55, Dean McNamee wrote:
> > blah blah full stops
> 
> Done.
> 
> http://codereview.chromium.org/67145/diff/27/39
> File chrome/common/x11_util.h (right):
> 
> http://codereview.chromium.org/67145/diff/27/39#newcode29
> Line 29: extern Display* kSecondaryXConnection;
> On 2009/04/16 09:32:55, Dean McNamee wrote:
> > woah?
> 
> I uploaded too soon; it's just a leftover. Removed.