Closed Bug 793501 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: Layout, defect, trivial)

18 Branch
x86
Linux
defect
Not set
trivial

Tracking

()

RESOLVED FIXED
mozilla19
Tracking Status
firefox17 --- unaffected
firefox18 --- wontfix

People

(Reporter: alice0775, Assigned: karlt)

References

Details

(Keywords: regression)

Attachments

(8 files, 1 obsolete file)

Attached image 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
Much more obvious without a compositing window manager.
viewmanager.refresh-driver-painting.enabled false restores the old behavior.
I don't know if this is useful information, but I don't see this problem when omtc is enabled.  (For whatever reason.)
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.
(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.
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.
Assignee: nobody → 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.
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.
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.
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().)
Attachment #673082 - Flags: review?(roc)
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
Attachment #673083 - Flags: review?(bzbarsky)
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.
Attachment #673084 - Flags: review?(roc)
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?)
Attachment #673083 - Flags: review?(bzbarsky) → review+
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.
Attachment #673088 - Flags: review?(roc)
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.
Attachment #673091 - Flags: review?(roc)
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.
Attachment #673098 - Flags: review?(roc)
One X window, without GDK black magic.
Attachment #673078 - Attachment is obsolete: true
Attachment #673117 - Flags: review?(roc)
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)
Attachment #673088 - Flags: review?(roc) → review+
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);

{}
Attachment #673117 - Flags: review?(roc) → review+
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.
Depends on: 805753
Depends on: 808873
Depends on: 835044
Duplicate of this bug: 835331
Depends on: 825944
You need to log in before you can comment on or make changes to this bug.