Bug 192230

Summary: REGRESSION(r235165): [GTK][WPE] Garbled rendering on GitLab
Product: WebKit Reporter: Michael Catanzaro <mcatanzaro>
Component: WebKitGTKAssignee: Zan Dobersek <zan>
Status: RESOLVED FIXED    
Severity: Normal CC: bugs-noreply, cadubentzen, cgarcia, cmarcelo, ews, kondapallykalyan, luiz, magomez, mcatanzaro, noam, zan
Priority: P2    
Version: WebKit Nightly Build   
Hardware: PC   
OS: Linux   
Bug Depends on:    
Bug Blocks: 192523    
Attachments:
Description Flags
Screenshot of garbled rendering
none
Different symptom: mismatched tiles
none
WIP
none
Patch
none
Patch for landing none

Description Michael Catanzaro 2018-11-30 09:34:15 PST
Created attachment 356188 [details]
Screenshot of garbled rendering

2.23.1 introduces a regression where renedring is sometimes garbled on gitlab.gnome.org. See attached screenshot. Unfortunately this regression occurs only rarely, so it is not possible to reproduce or bisect. And 2.23.1 was relatively late, so there is a huge range of commits (any commit since 2.22!) that could be a suspect.
Comment 1 Michael Catanzaro 2018-12-01 14:50:08 PST
Created attachment 356323 [details]
Different symptom: mismatched tiles

My guess is that AC mode is broken in 2.23.1, not sure.
Comment 2 Michael Catanzaro 2018-12-04 13:41:15 PST
Indeed I haven't noticed any problems with AC mode disabled.
Comment 3 Michael Catanzaro 2018-12-07 16:09:20 PST
Second screenshot here looks like bug #178387.
Comment 4 Zan Dobersek 2018-12-08 10:08:20 PST
Bisected all the way down to r235165.
https://trac.webkit.org/changeset/235165/webkit
Comment 5 Michael Catanzaro 2018-12-08 10:30:46 PST
Thanks Zan!

Note that we would have caught this months ago had we done 2.23.1 on our usual schedule. In retrospect, delaying that release while Carlos was on leave was a mistake.
Comment 6 Carlos Eduardo Ramalho 2018-12-15 13:47:09 PST
(In reply to Zan Dobersek from comment #4)
> Bisected all the way down to r235165.
> https://trac.webkit.org/changeset/235165/webkit

Zan, how did you reproduce this reliably? I guess you found a way to trigger this bug 100% to be able to bisect?
Comment 7 Michael Catanzaro 2018-12-15 17:54:44 PST
And are the two screenshots the same bug? The second screenshot looks like bug #178387.
Comment 8 Zan Dobersek 2018-12-17 07:15:20 PST
(In reply to Carlos Eduardo Ramalho from comment #6)
> (In reply to Zan Dobersek from comment #4)
> > Bisected all the way down to r235165.
> > https://trac.webkit.org/changeset/235165/webkit
> 
> Zan, how did you reproduce this reliably? I guess you found a way to trigger
> this bug 100% to be able to bisect?

It's not reliably reproducible.
Comment 9 Zan Dobersek 2018-12-17 07:18:27 PST
(In reply to Michael Catanzaro from comment #7)
> And are the two screenshots the same bug? The second screenshot looks like
> bug #178387.

I don't know. First screenshot is what I was able to fix. #178387 does not look related to this problem, but I don't know for sure what bug the second screenshot is capturing.
Comment 10 Zan Dobersek 2018-12-17 07:19:27 PST
Created attachment 357438 [details]
WIP

Please test.
Comment 11 Michael Catanzaro 2018-12-17 09:19:09 PST
(In reply to Zan Dobersek from comment #10)
> Created attachment 357438 [details]
> WIP
> 
> Please test.

Since the bug is not reproducible, I don't think this will get significant testing until it's released with 2.23.2. Alternatively, I can add the patch to the GNOME runtime now, if you want.
Comment 12 Carlos Eduardo Ramalho 2018-12-17 10:38:49 PST
(In reply to Michael Catanzaro from comment #11)
> (In reply to Zan Dobersek from comment #10)
> > Created attachment 357438 [details]
> > WIP
> > 
> > Please test.
> 
> Since the bug is not reproducible, I don't think this will get significant
> testing until it's released with 2.23.2. Alternatively, I can add the patch
> to the GNOME runtime now, if you want.

I hit the bug somewhat frequently scrolling Gitlab issues page. Let me just try it out first and see if is not there at least. I shall be able to test it later tonight.

But I agree that if we can't have a reduction for it and add a new test, then just a development release to ensure it's been fixed.
Comment 13 Zan Dobersek 2018-12-17 12:07:05 PST
Created attachment 357464 [details]
Patch
Comment 14 Carlos Eduardo Ramalho 2018-12-17 20:07:16 PST
(In reply to Zan Dobersek from comment #13)
> Created attachment 357464 [details]
> Patch

Bug doesn't happen for me anymore on gitlab issues page after applying this patch.

It was pretty frequent to reproduce on 2.23.1.
Comment 15 Carlos Garcia Campos 2018-12-18 00:07:13 PST
Comment on attachment 357464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357464&action=review

> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedBackingStore.cpp:60
> +    m_updates.clear();

Could updateTile be called during the loop? In that case I would move the vector before iterating it.
Comment 16 Zan Dobersek 2018-12-18 23:24:23 PST
Comment on attachment 357464 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=357464&action=review

>> Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedBackingStore.cpp:60
>> +    m_updates.clear();
> 
> Could updateTile be called during the loop? In that case I would move the vector before iterating it.

It shouldn't be possible, but it make sense to do the move.
Comment 17 Zan Dobersek 2018-12-18 23:37:24 PST
Created attachment 357664 [details]
Patch for landing
Comment 18 Zan Dobersek 2018-12-18 23:41:21 PST
Comment on attachment 357664 [details]
Patch for landing

Clearing flags on attachment: 357664

Committed r239371: <https://trac.webkit.org/changeset/239371>
Comment 19 Zan Dobersek 2018-12-18 23:41:25 PST
All reviewed patches have been landed.  Closing bug.