Bug 194845 – Crash under RemoteLayerTreePropertyApplier::applyProperties when reattaching to old process

• Component: New Bugs
• Status: RESOLVED FIXED

[Tim Horton]

Crash under RemoteLayerTreePropertyApplier::applyProperties when reattaching to old process

[Tim Horton]

Created attachment 362461 [details]
Patch

[Tim Horton]

<rdar://problem/47944579>

[Tim Horton]

Created attachment 362462 [details]
Patch

[EWS Watchlist]

Comment on attachment 362462 [details]
Patch

Attachment 362462 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11213039

New failing tests:
pageoverlay/overlay-small-frame-paints.html
pageoverlay/overlay-installation.html
pageoverlay/overlay-large-document-scrolled.html
pageoverlay/overlay-small-frame-mouse-events.html
pageoverlay/overlay-large-document.html

[EWS Watchlist]

Created attachment 362469 [details]
Archive of layout-test-results from ews101 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 362462 [details]
Patch

Attachment 362462 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/11212990

New failing tests:
pageoverlay/overlay-small-frame-paints.html
inspector/dom/highlightNodeList.html
pageoverlay/overlay-remove-reinsert-view.html
inspector/dom/highlightRect.html
inspector/dom/hideHighlight.html
pageoverlay/overlay-large-document-scrolled.html
pageoverlay/overlay-installation.html
pageoverlay/overlay-small-frame-mouse-events.html
inspector/dom/highlightFrame.html
inspector/dom/highlight-shape-outside-margin.html
inspector/dom/highlightNode.html
inspector/dom/highlightSelector.html
inspector/dom/highlightQuad.html
pageoverlay/overlay-large-document.html

[EWS Watchlist]

Created attachment 362470 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 362462 [details]
Patch

Attachment 362462 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11213275

New failing tests:
pageoverlay/overlay-small-frame-paints.html
pageoverlay/overlay-installation.html
pageoverlay/overlay-large-document-scrolled.html
pageoverlay/overlay-small-frame-mouse-events.html
pageoverlay/overlay-large-document.html

[EWS Watchlist]

Created attachment 362474 [details]
Archive of layout-test-results from ews117 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 362462 [details]
Patch

Attachment 362462 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11213290

New failing tests:
platform/ios/fast/scrolling/find-text-in-overflow-node.html
platform/ios/fast/scrolling/find-text-in-overflow-node-indicator-position.html
platform/ios/fast/scrolling/find-text-in-subframe.html

[EWS Watchlist]

Created attachment 362475 [details]
Archive of layout-test-results from ews126 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews126  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 362462 [details]
Patch

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

New failing tests:
pageoverlay/overlay-small-frame-paints.html
pageoverlay/overlay-remove-reinsert-view.html
pageoverlay/overlay-large-document-scrolled.html
pageoverlay/overlay-installation.html
pageoverlay/overlay-large-document.html
pageoverlay/overlay-small-frame-mouse-events.html

[EWS Watchlist]

Created attachment 362476 [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

[Tim Horton]

Oh... boy.

[Tim Horton]

Created attachment 362515 [details]
Patch

[Tim Horton]

Created attachment 362573 [details]
Patch

[Tim Horton]

Created attachment 362574 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 362574 [details]
Patch

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

> Source/WebCore/page/PageOverlayController.cpp:253
> +    for (auto& overlay : m_pageOverlays) {
> +        if (auto optionalLayer = m_overlayGraphicsLayers.take(overlay.get()))
> +            optionalLayer.value()->removeFromParent();
> +
> +        overlay->willReattachToPage(&m_page);

If an overlay triggers mutation of m_pageOverlays inside willReattachToPage() bad things will happen

> Source/WebCore/page/PageOverlayController.cpp:259
> +        installLayerForOverlay(*overlay);

Does this call into overlay code that could mutate m_pageOverlays?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PageOverlayPlugin.mm:61
> +
> +

Two blank lines.

[Tim Horton]

Comment on attachment 362574 [details]
Patch

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

>> Source/WebCore/page/PageOverlayController.cpp:253
>> +        overlay->willReattachToPage(&m_page);
> 
> If an overlay triggers mutation of m_pageOverlays inside willReattachToPage() bad things will happen

True, I suppose an oddly-written overlay implementation could do something weird like uninstall itself on attach (or install a new one). I don't think there are any that do now, but better safe than sorry

[Tim Horton]

Created attachment 362578 [details]
Patch

[Tim Horton]

Created attachment 362583 [details]
Patch

[Antti Koivisto]

Comment on attachment 362583 [details]
Patch

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

> Source/WebKit/WebProcess/WebPage/RemoteLayerTree/GraphicsLayerCARemote.h:56
> +    RemoteLayerTreeContext* m_context;

Some WeakPtrs would be nice.

[WebKit Commit Bot]

Comment on attachment 362583 [details]
Patch

Clearing flags on attachment: 362583

Committed r241899: <https://trac.webkit.org/changeset/241899>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

The new API test ProcessSwap.PageOverlayLayerPersistence

added in https://trac.webkit.org/changeset/241899/webkit

is failing on iOS: 
https://build.webkit.org/builders/Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/2763

Failed

    TestWebKitAPI.ProcessSwap.PageOverlayLayerPersistence
        Received data during response processing, queuing it.
        Received data during response processing, queuing it.
        
        /Volumes/Data/slave/ios-simulator-12-release/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/ProcessSwapOnNavigation.mm:5184
        Value of: hasOverlay([webView layer])
          Actual: false
        Expected: true

[Tim Horton]

(In reply to Truitt Savell from comment #25)
> The new API test ProcessSwap.PageOverlayLayerPersistence
> 
> added in https://trac.webkit.org/changeset/241899/webkit
> 
> is failing on iOS: 
> https://build.webkit.org/builders/
> Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/2763
> 
> Failed
> 
>     TestWebKitAPI.ProcessSwap.PageOverlayLayerPersistence
>         Received data during response processing, queuing it.
>         Received data during response processing, queuing it.
>         
>        
> /Volumes/Data/slave/ios-simulator-12-release/build/Tools/TestWebKitAPI/Tests/
> WebKitCocoa/ProcessSwapOnNavigation.mm:5184
>         Value of: hasOverlay([webView layer])
>           Actual: false
>         Expected: true

I'll take a peek; it was passing for me!

[Tim Horton]

(In reply to Tim Horton from comment #26)
> (In reply to Truitt Savell from comment #25)
> > The new API test ProcessSwap.PageOverlayLayerPersistence
> > 
> > added in https://trac.webkit.org/changeset/241899/webkit
> > 
> > is failing on iOS: 
> > https://build.webkit.org/builders/
> > Apple%20iOS%2012%20Simulator%20Release%20WK2%20%28Tests%29/builds/2763
> > 
> > Failed
> > 
> >     TestWebKitAPI.ProcessSwap.PageOverlayLayerPersistence
> >         Received data during response processing, queuing it.
> >         Received data during response processing, queuing it.
> >         
> >        
> > /Volumes/Data/slave/ios-simulator-12-release/build/Tools/TestWebKitAPI/Tests/
> > WebKitCocoa/ProcessSwapOnNavigation.mm:5184
> >         Value of: hasOverlay([webView layer])
> >           Actual: false
> >         Expected: true
> 
> I'll take a peek; it was passing for me!

Ah, I see what's up. It's a trunk-only problem (or actually a pair of them).

[Tim Horton]

Fixing the API test in https://bugs.webkit.org/show_bug.cgi?id=194963