Bug 221252

Summary: Assertion failure in Debug build blocks webpage rendering
Product: WebKit Reporter: Eleni Maria Stea <estea>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: NEW ---    
Severity: Minor CC: bfulgham, estea, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: WebKit Local Build   
Hardware: PC   
OS: Linux   
See Also: https://bugs.webkit.org/show_bug.cgi?id=191962
https://bugs.webkit.org/show_bug.cgi?id=166008
Attachments:
Description Flags
proposed patch simon.fraser: review-

Description Eleni Maria Stea 2021-02-02 04:46:55 PST 
Overview:

Assertion failure from: ASSERT(willBeComposited == needsToBeComposited(layer, queryData)); in rendering/RenderLayerCompositor.cpp stops the rendering of some webpages in MiniBrowser in Debug builds. The reason seems to be that the layers are asynchronously updated since the time willBeComposited is set and needsToBeComposited is called, and so events like scrolling overflow can cause assertion failure. The assertion should be moved right after willBeComposited is set.

Steps to reproduce:
Build WPE/Webkit in Debug mode and run MiniBrowser with https://www.igalia.com. 

Results:
An empty page will be displayed.

Expected Result:
The webpage to be displayed.

Proposed Fix:
If the assertion is moved right after:

    if (layer.needsPostLayoutCompositingUpdate() || compositingState.fullPaintOrderTraversalRequired || compositingState.descendantsRequireCompositingUpdate) {
        layer.setIndirectCompositingReason(IndirectCompositingReason::None);
        willBeComposited = needsToBeComposited(layer, queryData);
    }
the page will appear fine in MiniBrowser.

Additional Information:
Bug was spotted on Linux/X11 using nested Weston and running MiniBrowser like this:
./run-wpe-x11 <path_to_minibrowser_installdir>MiniBrowser https://www.igalia.com
Comment 1 Eleni Maria Stea 2021-02-02 05:03:54 PST 
Created attachment 418983 [details]
proposed patch

Proposed patch: move the assertion before all other checks that might change the needsToBeComposited(layer, queryData) value.
Comment 2 Simon Fraser (smfr) 2021-02-02 09:58:50 PST 
Comment on attachment 418983 [details]
proposed patch

The whole point of this assertion is to check that willBeComposited has the correct value.
Comment 3 Eleni Maria Stea 2021-02-02 14:23:07 PST 
(In reply to Simon Fraser (smfr) from comment #2)
> Comment on attachment 418983 [details]
> proposed patch
> 
> The whole point of this assertion is to check that willBeComposited has the
> correct value.

Thank you for your reply, 

What I tried to prevent by moving the assertion earlier was the case that needsToBeComposited() is different from willBeComposited value without that being a bug. I noticed that willBeComposited and needsToBeComposited have the same value at the beginning of the function but further down there are cases that change the value of the variable and so, the same check fails.

I thought that the willBeComposited changes were correctly made and so I assumed that the problem is that the assertion failure takes place too late after the changes.

My description was not so clear, sorry.

What are your thoughts on this?
Thanks in advance!
Comment 4 Simon Fraser (smfr) 2021-02-02 14:37:38 PST 
If this assertion fires, it means that some state changed that affected the answer to needsToBeComposited() inside this function, which implies there's some side-effect. If willBeComposited does not match needsToBeComposited() then the behavior is wrong.

You should go looking for why needsToBeComposited() changes its answer unexpectedly.
Comment 5 Eleni Maria Stea 2021-02-02 15:05:34 PST 
(In reply to Simon Fraser (smfr) from comment #4)
> If this assertion fires, it means that some state changed that affected the
> answer to needsToBeComposited() inside this function, which implies there's
> some side-effect. If willBeComposited does not match needsToBeComposited()
> then the behavior is wrong.
> 
> You should go looking for why needsToBeComposited() changes its answer
> unexpectedly.

Alright, thank you very much! 

needsToBeComposited() was true because compositing was required for overflow scrolling (although I saw the assertion failure in some other cases too). I hadn't realized this is unexpected, I will investigate it further.
Comment 6 Radar WebKit Bug Importer 2021-02-09 04:47:54 PST 
<rdar://problem/74138081>