793501 - Graphical problem at start up browser window on Ubuntu12.04.

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Alice0775 White]

Attachment: Screenshot

Build Identifier:
http://hg.mozilla.org/mozilla-central/rev/48c4938eaf57
Mozilla/5.0 (X11; Linux i686; rv:18.0) Gecko/18 Firefox/18.0a1 ID:20120921030601

Ubuntu 12.04.1
At the time of a start of the browser window, a desktop is seen in the top left corner temporarily

This problem happens regardless with HWA on/off
I can reproduce only in Nightly18.0a1.

Regression window
Good:
http://hg.mozilla.org/mozilla-central/rev/f89feda9d997
Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17 Firefox/17.0a1 ID:20120813030532
Bad:
http://hg.mozilla.org/mozilla-central/rev/22288130fea2
Mozilla/5.0 (X11; Linux i686; rv:17.0) Gecko/17 Firefox/17.0a1 ID:20120814030521
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=f89feda9d997&tochange=22288130fea2

In local build,
Last Good: ed614ea130c0
First Bad: 13d19788ad1d

Graphics
  Adapter Description : VMware, Inc. -- Gallium 0.4 on SVGA3D; build: RELEASE;
  Device ID : Gallium 0.4 on SVGA3D; build: RELEASE;
  Driver Version : 2.1 Mesa 8.0.2
  GPU Accelerated Windows : 0/1 Basic no information
  Vendor ID : VMware, Inc.
  WebGL Renderer : VMware, Inc. -- Gallium 0.4 on SVGA3D; build: RELEASE;   -- 2.1 Mesa 8.0.2
  AzureCanvasBackend : cairo
  AzureContentBackend : none
  AzureFallbackCanvasBackend : none

Comment 1

[Karl Tomlinson (:karlt)]

Much more obvious without a compositing window manager.
viewmanager.refresh-driver-painting.enabled false restores the old behavior.

Comment 2

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

I don't know if this is useful information, but I don't see this problem when omtc is enabled.  (For whatever reason.)

Comment 3

[Karl Tomlinson (:karlt)]

This seems to be some interaction with GDK applying shapes to child windows.

Before GTK maps the toplevel window, it issues a ConfigureRequest to resize the window.  Probably because this request may be redirected to and modified by the window manager, though I'm not sure it would be for an unmapped window, GDK does not record the new size on the window until it receives the ConfigureNotify reply.

Immediately after the toplevel is mapped, GDK sets a shape on the child window.  This is based on parent window sizes - probably some client-side window code that doesn't really make much sense for the server-side toplevel window size.  GDK still considers the toplevel to have the size it had before the ConfigureRequest, which happens to be the default GtkWindow size of 200x200.

The child window has a back pixmap of None, meaning it shows nothing until it is drawn, and so the background remains.  The toplevel window has a back pixmap or background color based on the theme and so looks inconsistent.

Some options I'm thinking about:

1) Change the default size of the window.  This needs to be done before realizing the window, but the desired size is not provided at that point.  A small size could be selected to limit the effect to 1 pixel.

2) Try sizing the window as soon as possible instead of waiting until Show.
The event loop would need to run to process the ConfigureNotify, so this may not be helpful.

3) Look into setting the back pixmap of the toplevel window to None.

4) Don't set the back pixmap of child windows to None.

5) Find out why dlbi changes made this show up.

Comment 4

[Karl Tomlinson (:karlt)]

(In reply to Karl Tomlinson (:karlt) from comment #3)
> Immediately after the toplevel is mapped, GDK sets a shape on the child
> window.

Immediately before, actually.

Comment 5

[Karl Tomlinson (:karlt)]

The reason this didn't happen before refresh-driver-painting was that GDK
didn't create a native X Window for the child window until we requested it,
and that didn't happen until the expose event, at which point I assume GDK knew
the size of the toplevel window.

What is different with refresh driver painting is that it calls
GetThebesSurface() off the refresh driver to select a buffer format for the
BasicThebesLayer buffer.  This can result in the X Window for the child window
being created before GDK receives the ConfigureNotify event.

Perhaps we could come up with a different reference surface (the toplevel
window surface would be fine in our controlled situation) but the problem
would be harder to solve with GL layers where the GLContext is created
for the native X window into which we'll draw.

I don't think we can Show/Map the window, and then paint immediately because
the Map request is redirected to the window manager and may not have been
effected before we paint.  And the child window will still have the incorrect
shape, masking out drawing until the ConfigureNotify is received.

Comment 6

[Karl Tomlinson (:karlt)]

It looks like _NET_WM_SYNC_REQUEST with background None would be a way to postpone client and window manager drawing until the ConfigureNotify has been received.
http://standards.freedesktop.org/wm-spec/1.5/ar01s06.html#id2761266
However, the support doesn't seem to be there in GTK or in window managers.
It seems it is used mostly for window-manager-initiated resize drags, possibly because most apps don't have background None.

gtk_window_show() does not increment configure_request_count when it resizes the
window, and so gtk_window_configure_event() bails out early, replying to the
_NET_WM_SYNC_REQUEST before the drawing has taken place.

kwin 4.8.5 does not wait (any significant time at least) for the
_NET_WM_SYNC_REQUEST reply before drawing the frame.  It only sends the
message for the resize before show when compositing is enabled.

metacity 2.30.3 and 2.34.8 do not send a _NET_WM_SYNC_REQUEST for the resize
before show, with or without compositing enabled.  They do send these when
dragging the frame border to resize.

I haven't tested compiz or mutter which I would expect to use
_NET_WM_SYNC_REQUEST at least for resize drags.  Bug 378293 seems to suggest compiz
is using _NET_WM_SYNC_REQUEST as an indicator of when it expects to have all Damage events for a window show.

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: use ParentRelative background pixmap to avoid effects from shapes GDK applies to child windows during Show

Given the window manager is drawing the frame before we respond to the
ConfigureNotify event, we might as well give in and let the background be
drawn on Show.

This would be quite the wrong thing for translucent windows, as the shape
would be wrong until we paint.  However, we only make override-redirect
windows transparent and GDK handles override-redirect windows differently,
resizing synchronously and temporarily setting the background to None during
show to suppress display of the background.

This patch is perhaps a simple quick fix, but we can do a better fix.

Using ParentRelative makes child Windows look just like their parent Window's
background until they are drawn.  This seems to look a little better than
setting background None on the toplevel, which leads to the window manager
frame painting while there still seems to be a big gap in the middle.

ParentRelative has the additional requirement that the parent window needs to
have matching depth.  Usually our child windows inherit the colormap/visual
from the parent and so that is satisfied.  That inheritance doesn't guarantee
the same depth when windows are reparented.  The only toplevel windows that we
give different depths are popup windows, so that would only be a problem if we
had windowed plugins in a popup.  However, I'm not sure that styles/themes can't set different visuals on different widgets, which would be a problem to solve when reparenting to the GtkInvisible widget.

Comment 8

[Karl Tomlinson (:karlt)]

I started looking into drawing directly to the toplevel X Window and never
doing anything to trigger creation of X Windows for child windows, but that
seems to be relying too much on the implementation of GdkWindows.

The best strategy would be to redesign MozContainer to be a GTK_NO_WINDOW widget and stop creating child nsWindows for each toplevel window.  I'll upload patches for that.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: set gFocusWindow on fallback instead of early

This was interfering with focus change detection in SetFocus() when the nsWindow of the container receives focus, and so mIMModule->OnFocusWindow() was not getting called.  (Currently the child window is the one receiving focus in SetFocus().)

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: don't resize toplevel window in response to resized notification

When the widget calls WindowResized() on either of the listeners,
nsDocShell::SetPositionAndSize() and DocumentViewerImpl::SetBounds() get
called.  This is a notification that the window has changed.  When the
viewer is not mAttachedToParent, it makes sense to resize the child window in
response to toplevel change.  Resizing the toplevel again doesn't make sense
in this situation, and would lead to infinite recursion (unless broken in some
way).  When !mAttachedToParent the Resize() here would not resize
the toplevel, so some other mechanism should be used if the intention is to resize the toplevel.

https://tbpl.mozilla.org/?tree=Try&rev=5ab9d45888b3
https://tbpl.mozilla.org/?tree=Try&rev=6691a8e5486b

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: remove unused event handler parameters

The following patch will change widget signal handlers somewhat.
Removing the unused GtkWidget parameters makes it clear where the particular widget provided by the signal is not important.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 673083 [details] [diff] [review]
don't resize toplevel window in response to resized notification

r=me if this is all tested in the cases when mAttachedToParent is true (Windows?)

Comment 13

[Karl Tomlinson (:karlt)]

Attachment: don't use a separate GdkWindow for MozContainer (unless necessary)

Gtk has the concept of GTK_NO_WINDOW widgets.  For these
gtk_widget_get_window() returns the parent (or ancestor) widget's window.
These widgets should not modify the parent widget's window, expect to draw.
gdk_window_get_user_data(gdk_widget_get_window(widget), &data) will not return widget for these windows.

Comment 14

[Karl Tomlinson (:karlt)]

Attachment: don't log child window XIDs as this has the side effect of creating X Windows early

When logging was enabled, the gdk_x11_window_get_xid here for the child window
was slightly concealing what has changed with dlbi because it caused GDK to generate a native X Window for the GdkWindow earlier.

The child window XIDs are available from xwininfo anyway, so I've just included the parent window for child window logs to help tie things together.

This also now output the single mGdkWindow as it is shared by both widgets.

Comment 15

[Karl Tomlinson (:karlt)]

Attachment: invalidate new regions of even toplevel resized windows

This addresses a noticable difference in painting the new area of a window
being resized by the window manager (through dragging the frame).

With child windows, the resize notification here of parent window change
caused a client request to resize the child window (through WindowResized).
GDK invalidated the new part of the child window while doing this.

Immediately after dispatching this size-allocate signal, GTK does a
synchronous paint of invalid regions.

Without a child window, the client-side invalidate doesn't happen and the X
Expose event hasn't been received yet and so the repaint lags behind, leaving
the background quite visible.

The ForcedRepaint() code in the view system doesn't work as well as we might
like.  It invalidates the whole window, but with basic layers only the region
in the native expose event is repainted.  The whole invalidated rect is
repainted later, but by that time it seems the window manager has already
started compositing the new portion that was painted and includes the
background of the new part of the window as well.  I guess we could change
this process to repaint freshly invalidated regions as well as the region of
the expose signal, but I'll save that for a later date.

This patch is essentially keeping the behavior that we have on child windows.
I guess we could invalidate the whole window here, but we don't seem to need
to.  The key is to prevent the background flickering by painting the new part
as fast as possible.  It doesn't seem to matter that the painting of resized
existing content lags a little.

While modifying this code, I'm skipping the WindowResized if there is no
change.  If mBounds was changed in the Resize() methods, then WindowResized
would have been called there.

Comment 16

[Karl Tomlinson (:karlt)]

Attachment: attach DocumentViewer to top GTK widget

One X window, without GDK black magic.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 673088 [details] [diff] [review]
don't use a separate GdkWindow for MozContainer (unless necessary)

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

::: widget/gtk2/mozcontainer.c
@@ +196,5 @@
>          tmp_list = tmp_list->next;
>      }
>  
> +    if (gtk_widget_get_has_window (widget))
> +        gdk_window_show (gtk_widget_get_window(widget));

{}

@@ +207,5 @@
>  
>      gtk_widget_set_mapped(widget, FALSE);
>  
> +    if (gtk_widget_get_has_window (widget))
> +        gdk_window_hide (gtk_widget_get_window(widget));

{}

::: widget/gtk2/nsWindow.cpp
@@ +3718,5 @@
>          g_signal_connect(mContainer, "drag_data_received",
>                           G_CALLBACK(drag_data_received_event_cb), NULL);
>  
> +        GtkWidget *widgets[] = { GTK_WIDGET(mContainer), mShell, nullptr };
> +        for (int i = 0; widgets[i]; ++i) {

Slightly better to just use for (int i = 0; i < ArrayLength(widgets); ++i)

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 673117 [details] [diff] [review]
attach DocumentViewer to top GTK widget

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

Cool!

On Windows this caused all kinds of breakage with third-party apps that try to monkey with Firefox's HWNDs. Hopefully we won't have that sort of trouble on X.

::: widget/gtk2/nsWindow.cpp
@@ +458,5 @@
> +    nsIWidgetListener *listeners[] =
> +        { mWidgetListener, mAttachedWidgetListener };
> +    for (size_t i = 0; i < ArrayLength(listeners); ++i) {
> +        if (listeners[i])
> +            listeners[i]->WindowResized(this, aWidth, aHeight);

{}

@@ +474,5 @@
>      aStatus = nsEventStatus_eIgnore;
> +    nsIWidgetListener* listener =
> +        mAttachedWidgetListener ? mAttachedWidgetListener : mWidgetListener;
> +    if (listener)
> +      aStatus = listener->HandleEvent(aEvent, mUseAttachedEvents);

{}

Comment 19

[Karl Tomlinson (:karlt)]

Thanks for the reviews.

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #17)
> > +        GtkWidget *widgets[] = { GTK_WIDGET(mContainer), mShell, nullptr };
> > +        for (int i = 0; widgets[i]; ++i) {
> 
> Slightly better to just use for (int i = 0; i < ArrayLength(widgets); ++i)

mShell may be null, so we'd also need if ( widgets[i] ) {
                                      }
but maybe that's not so bad because it would be a little clearer.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Indeed!

Comment 21

[Karl Tomlinson (:karlt)]

https://tbpl.mozilla.org/?tree=Try&rev=babd1f5a47c7

https://hg.mozilla.org/integration/mozilla-inbound/rev/45b8a00043d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/31df6929909f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ded8c7f9a0d2
https://hg.mozilla.org/integration/mozilla-inbound/rev/9541dbf6e020
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c2eaa7e3edd
https://hg.mozilla.org/integration/mozilla-inbound/rev/8da9c24417d0
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0158d370785

These changes are probably too significant to consider for Aurora, when weighed against the significance of the glitch.

Comment 22

[Ryan VanderMeulen [:RyanVM][PTO June 24-28]]

https://hg.mozilla.org/mozilla-central/rev/45b8a00043d5
https://hg.mozilla.org/mozilla-central/rev/31df6929909f
https://hg.mozilla.org/mozilla-central/rev/ded8c7f9a0d2
https://hg.mozilla.org/mozilla-central/rev/9541dbf6e020
https://hg.mozilla.org/mozilla-central/rev/1c2eaa7e3edd
https://hg.mozilla.org/mozilla-central/rev/8da9c24417d0
https://hg.mozilla.org/mozilla-central/rev/a0158d370785

Comment 24

[Karl Tomlinson (:karlt)]

(In reply to Alexander Larsson from comment #40)
> Not sure what you mean by synchronously, but the window manager is not
> involved with child window resizes, so we need not do any weird
> configurenotify roundtrip. When we get a configure-notify for the GtkWindow
> and the size has changed we queue a resize on the toplevel, which will
> trigger a layout cycle, calculating all the sizes of child widgets, which
> are then applied and we redraw the frame and push it. This cycle is tied to
> the frame clock, so i guess there could be some time before it is done. We
> generally map our windows initially bg-none though so nothing should be
> visible until we actually draw the contents.
> 
> I guess this is problematic for firefox, as it is not synced to the same
> frame clock, as it will create a child window without bg none, and start
> drawing to it earlier.

Thanks for your comments, Alexander.

I hadn't caught up with the _NET_WM_SYNC_REQUEST changes [1] but gather that
"synchronously" is no longer a requirement now.

If child widgets are resized and sent expose signals before the
_NET_WM_SYNC_REQUEST reply is sent then that should give Gecko a chance to
draw with window managers supporting the new _NET_WM_SYNC_REQUEST [2].

For older window managers, there may still be the issues of unsynchronized
mapping of windows.  I'm not clear how much the situation has improved there,
so feel safer sticking with the current strategy where practical until this is
clear.

[1] https://git.gnome.org/browse/gtk+/commit/?id=645b5f398d2d50c361600f4f8a089488a66eed99