Assertion failure !isInAcceleratedCompositingMode() in DrawingAreaProxyCoordinatedGraphics::incorporateUpdate when forceCompositingMode is turned on

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Tomoki Imai]

How to reproduce:

1. Build WebKitGTK with --debug to enable ASSERT
2. Run GTK MiniBrowser
3. Disable accelerated compositing from settings (Hardware Acceleration Policy -> never)
4. Go to https://projects.lukehaas.me/css-loaders/ (any website with animation is enough)
5. Enable forceCompositingMode from settings (Hardware Acceleration Policy -> always)

Actual result:
ASSERTION FAILED: ASSERTION FAILED: !isInAcceleratedCompositingMode()
../../Source/WebKit/UIProcess/CoordinatedGraphics/DrawingAreaProxyCoordinatedGraphics.cpp(245) : void WebKit::DrawingAreaProxyCoordinatedGraphics::incorporateUpdate(const WebKit::UpdateInfo&)

Expected result:
No ASSERT error occurred.

The root cause is that DrawingAreaProxyCoordinatedGraphics::isInAcceleratedCompositingMode checks both of alwaysUseCompositing() and !m_layerTreeContext.isEmpty().
alwaysUseCompositing() refers preferences, which is written by the application (UIProcess).
On the other hand, m_layerTreeContext is changed when it receives enterAcceleratedCompositingMode/exitAcceleratedCompositingMode from WebProcess.

It results when we set forceCompositingMode and acceleratedCompositingEnabled to true, WebProcess and UIProcess is out of sync until WebProcess sends enterAcceleratedCompositingMode message.
In such situation, WebProcess sends incorporateUpdate to UIProcess because WebProcess is in non-AC mode, but isInAcceleratedCompositingMode becomes true in UIProcess side.

Note that turning on forceCompositingMode on https://www.webkitgtk.org/ results blank page, and resize window trigger redraws.
I guess it is a different problem from this bug.

[Tomoki Imai]

Created attachment 365007 [details]
Patch

[Tomoki Imai]

Created attachment 365008 [details]
Patch

Replace tabs in ChangeLog

[Don Olmstead]

Igalia folks any particular issues with this patch? It LGTM but we wanted to make sure we aren't missing something here.

[Carlos Garcia Campos]

I'll review it, it was already in my TODO.

[Carlos Garcia Campos]

Comment on attachment 365008 [details]
Patch

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

> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:545
> -    if (webViewBase->priv->acceleratedBackingStore && drawingArea->isInAcceleratedCompositingMode())
> +    if (webViewBase->priv->acceleratedBackingStore && (drawingArea->isInAcceleratedCompositingMode() || drawingArea->alwaysUseCompositing()))

I don't think we want to do this. If we are still transitioning, incorporateUpdate updated the backing store, so if at this point we haven't entered AC mode, we want to paint the new backing store, since the accelerated backing store won't have anything to paint yet.

[Tomoki Imai]

Comment on attachment 365008 [details]
Patch

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

>> Source/WebKit/UIProcess/API/gtk/WebKitWebViewBase.cpp:545
>> +    if (webViewBase->priv->acceleratedBackingStore && (drawingArea->isInAcceleratedCompositingMode() || drawingArea->alwaysUseCompositing()))
> 
> I don't think we want to do this. If we are still transitioning, incorporateUpdate updated the backing store, so if at this point we haven't entered AC mode, we want to paint the new backing store, since the accelerated backing store won't have anything to paint yet.

Right, the reason I added `|| drawingArea->alwaysUseCompositing()` here is to stop painting new backing store right after `alwaysUseCompositing` is turned on, which is current behavior.
But if we don't have such usecase , we should check only `webViewBase->priv->acceleratedBackingStore && drawingArea->isInAcceleratedCompositingMode()` to continue using backing store until entering AC mode.

[Tomoki Imai]

Comment on attachment 365008 [details]
Patch

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

> Source/WebKit/UIProcess/CoordinatedGraphics/DrawingAreaProxyCoordinatedGraphics.cpp:79
> +    if (isInAcceleratedCompositingMode() || alwaysUseCompositing())

I think we shouldn't have " || alwaysUseCompositing()" here for the same reason as webkitWebViewBaseDraw. If it's transitioning to AC mode, we should update BackingStore.

[Tomoki Imai]

Created attachment 365854 [details]
Patch, fixes reviewed point

I removed alwaysUseCompositing() from webkitWebViewBaseDraw and DrawingAreaProxyCoordinatedGraphics::paint, because while transitioning to AC mode, we should render in non-AC mode.

[EWS Watchlist]

Comment on attachment 365854 [details]
Patch, fixes reviewed point

Attachment 365854 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11658597

New failing tests:
animations/resume-after-page-cache.html

[EWS Watchlist]

Created attachment 365856 [details]
Archive of layout-test-results from ews200 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Tomoki Imai]

Created attachment 365950 [details]
Patch, fixes reviewed point

WinCairo build seems to be broken. Upload patch again to re-run EWS.

[WebKit Commit Bot]

Comment on attachment 365950 [details]
Patch, fixes reviewed point

Clearing flags on attachment: 365950

Committed r243505: <https://trac.webkit.org/changeset/243505>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.