WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
189743
[GTK][AC] Resizing the window doesn't always update the visible rect
https://bugs.webkit.org/show_bug.cgi?id=189743
Summary
[GTK][AC] Resizing the window doesn't always update the visible rect
Sergio Villar Senin
Reported
2018-09-19 03:16:31 PDT
I'm using Debian's 2.22.0 Not sure if it's the same case as the old
https://bugs.webkit.org/show_bug.cgi?id=115096
but sometimes I enlarge a window and the new space for the visible rect is just filled with a grey color but the contents of the web page are not painted there. I still haven't found a reliable way to reproduce it, but you just need to resize the window several times (perhaps not continuously but not sure) and the problem will eventually show up. I'm adding the [AC] tag because I suspect that's the culprit of the visual artifact I'm observing.
Attachments
Patch
(4.02 KB, patch)
2018-10-17 02:53 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch
(12.58 KB, patch)
2018-10-23 02:21 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Patch for landing
(12.54 KB, patch)
2018-10-25 04:06 PDT
,
Zan Dobersek
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Carlos Garcia Campos
Comment 1
2018-09-19 03:21:50 PDT
Yes, this is happening to me too, I had in my TODO to investigate the problem.
Michael Catanzaro
Comment 2
2018-09-19 06:27:35 PDT
This has been broken since I started back in 2014.
Carlos Garcia Campos
Comment 3
2018-09-19 06:31:19 PDT
This broke recently for me
Sergio Villar Senin
Comment 4
2018-09-20 04:30:16 PDT
(In reply to Michael Catanzaro from
comment #2
)
> This has been broken since I started back in 2014.
I've contributed to WK since 2010 and using it for even longer time and this is the first time it happens to me. I don't deny that it might have been a problem back then in some specific situations but now it's happening every day to me and several times each day.
Michael Catanzaro
Comment 5
2018-09-21 12:44:55 PDT
There has always been a conspicuously-broken visual issue, when resizing in AC mode, where the newly-visible portion of the window is gray/white, then painted later (too late), a problem which does not exist in non-AC mode. Seems that, as of recently, the newly-visible rect is sometimes never painted over at all, rather than just being painted late like before. I've noticed this a few times since you reported this bug and I agree that it's a new problem.
Sergio Villar Senin
Comment 6
2018-09-21 13:28:28 PDT
(In reply to Michael Catanzaro from
comment #5
)
> There has always been a conspicuously-broken visual issue, when resizing in > AC mode, where the newly-visible portion of the window is gray/white, then > painted later (too late), a problem which does not exist in non-AC mode.
Well I think that's the price you pay for having WebProcess composition. The UI is resized, so it asks the WebProcess for new contents, but they cannot be served immediately so you end up with those unpainted areas. That does not happen with UIProcess compositing because the UI process can already resolve the resizing/zooming/etc... interactions because it has a local copy of the compositing tree. A way to mitigate that is (I don't know if we're doing that) to render also content which is not currently in the visible rect to help with scrolling/resizing.
> Seems that, as of recently, the newly-visible rect is sometimes never > painted over at all, rather than just being painted late like before. I've > noticed this a few times since you reported this bug and I agree that it's a > new problem.
Great. Let's see if we manage to bisect it and find the culprit.
Michael Catanzaro
Comment 7
2018-09-23 07:26:50 PDT
(In reply to Sergio Villar Senin from
comment #6
)
> A way to mitigate that is (I don't know if we're doing that) to render also > content which is not currently in the visible rect to help with > scrolling/resizing.
Maybe we should do that then. This is one of the reasons we don't want to switch AC mode to always on: because resizing in AC mode looks so terrible....
Michael Catanzaro
Comment 8
2018-09-24 08:37:36 PDT
***
Bug 189849
has been marked as a duplicate of this bug. ***
Michael Catanzaro
Comment 9
2018-10-03 07:26:48 PDT
Hey Adrian, let's consider this one a blocker for the 2.22.3 release. (In reply to Sergio Villar Senin from
comment #6
)
> Great. Let's see if we manage to bisect it and find the culprit.
I agree we need to bisect this to make progress. I didn't have time this week. Would be really nice if someone with a build machine could handle it.
Adrian Perez
Comment 10
2018-10-03 08:16:48 PDT
(In reply to Michael Catanzaro from
comment #9
)
> Hey Adrian, let's consider this one a blocker for the 2.22.3 release.
Sure thing. There's a good bunch of MSE patches which are already merged in the release branch waiting for 2.22.3 to happen, but at least we don't have anyone completely blocked out of being able to watch videos so that's not in a rush anymore — we can wait for this.
Zan Dobersek
Comment 11
2018-10-17 02:53:44 PDT
Created
attachment 352559
[details]
Patch
Zan Dobersek
Comment 12
2018-10-17 02:59:29 PDT
Comment on
attachment 352559
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=352559&action=review
> Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:206 > - if (m_surface && m_surface->resize(size)) > + // if (m_surface && m_surface->resize(size))
When the page and its connected layer tree host is being resized, the associated AcceleratedSurface is resized right here, on the main thread, whereas the composition (which would be affected by the resize) is all done in a separate thread. So this is just an inappropriate cross-thread access into the wl_egl_window object that's managed in the derived AcceleratedSurfaceWayland class. This patch moves the resizing operation over to the composition thread, with the underlying resizing issue disappearing. Reason why this is done on the main thread is the X11-specific implementation of AcceleratedSurface. That one respawns a pixmap object on each resize and retrieves its address as the surface ID that's here assigned to the m_layerTreeContext.contextID variable, which I guess is necessary.
Zan Dobersek
Comment 13
2018-10-17 02:59:30 PDT
Comment on
attachment 352559
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=352559&action=review
> Source/WebKit/WebProcess/WebPage/CoordinatedGraphics/ThreadedCoordinatedLayerTreeHost.cpp:206 > - if (m_surface && m_surface->resize(size)) > + // if (m_surface && m_surface->resize(size))
When the page and its connected layer tree host is being resized, the associated AcceleratedSurface is resized right here, on the main thread, whereas the composition (which would be affected by the resize) is all done in a separate thread. So this is just an inappropriate cross-thread access into the wl_egl_window object that's managed in the derived AcceleratedSurfaceWayland class. This patch moves the resizing operation over to the composition thread, with the underlying resizing issue disappearing. Reason why this is done on the main thread is the X11-specific implementation of AcceleratedSurface. That one respawns a pixmap object on each resize and retrieves its address as the surface ID that's here assigned to the m_layerTreeContext.contextID variable, which I guess is necessary.
Michael Catanzaro
Comment 14
2018-10-17 08:31:06 PDT
I can confirm this patch fixes YouTube. Thanks a bunch.
Michael Catanzaro
Comment 15
2018-10-17 08:32:40 PDT
(In reply to Adrian Perez from
comment #10
)
> Sure thing. There's a good bunch of MSE patches which are already > merged in the release branch waiting for 2.22.3 to happen, but at > least we don't have anyone completely blocked out of being able to > watch videos so that's not in a rush anymore — we can wait for this.
IMO this bug is so important, we should stall further releases until a final patch is ready.
Adrian Perez
Comment 16
2018-10-17 15:06:07 PDT
(In reply to Michael Catanzaro from
comment #15
)
> (In reply to Adrian Perez from
comment #10
) > > Sure thing. There's a good bunch of MSE patches which are already > > merged in the release branch waiting for 2.22.3 to happen, but at > > least we don't have anyone completely blocked out of being able to > > watch videos so that's not in a rush anymore — we can wait for this. > > IMO this bug is so important, we should stall further releases until a final > patch is ready.
Yes, for the moment I am holding the release. Nice to see progress here! Note that if we have a patch that we think it's not fit to be landed upstream *just yet*, but we that works and we are comfortable having point release, it is also possible to merge the patch in its WIP form to the release branch and update later on to its final version. I would rather avoid doing that, but it's definitely an option (such a thing has been done a couple of times before). :-)
Zan Dobersek
Comment 17
2018-10-23 02:21:43 PDT
Created
attachment 352963
[details]
Patch
Michael Catanzaro
Comment 18
2018-10-23 07:25:03 PDT
Comment on
attachment 352963
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=352963&action=review
Thanks a bunch! If Miguel is available to review this before landing, that would be ideal.
> Source/WebKit/ChangeLog:3 > + REGRESSION(r??????): [GTK][AC] Resizing the window doesn't always update the visible rect
Probably best to remove the REGRESSION bit from the commit message, since we never figured out at which point this regressed.
Miguel Gomez
Comment 19
2018-10-24 08:16:11 PDT
(In reply to Michael Catanzaro from
comment #18
)
> Comment on
attachment 352963
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=352963&action=review
> > Thanks a bunch! > > If Miguel is available to review this before landing, that would be ideal.
LGTM
Michael Catanzaro
Comment 20
2018-10-24 16:47:19 PDT
(In reply to Michael Catanzaro from
comment #14
)
> I can confirm this patch fixes YouTube. Thanks a bunch.
The new version works too!
Zan Dobersek
Comment 21
2018-10-25 04:06:44 PDT
Created
attachment 353081
[details]
Patch for landing
Zan Dobersek
Comment 22
2018-10-25 04:09:42 PDT
Comment on
attachment 353081
[details]
Patch for landing Clearing flags on attachment: 353081 Committed
r237410
: <
https://trac.webkit.org/changeset/237410
>
Zan Dobersek
Comment 23
2018-10-25 04:09:46 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug