Bug 194845

Summary: Crash under RemoteLayerTreePropertyApplier::applyProperties when reattaching to old process
Product: WebKit Reporter: Tim Horton <thorton>
Component: New BugsAssignee: Tim Horton <thorton>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, ews, koivisto, rniwa, simon.fraser, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Archive of layout-test-results from ews101 for mac-highsierra
none
Archive of layout-test-results from ews106 for mac-highsierra-wk2
none
Archive of layout-test-results from ews117 for mac-highsierra
none
Archive of layout-test-results from ews126 for ios-simulator-wk2
none
Archive of layout-test-results from ews200 for win-future
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch none

Description Tim Horton 2019-02-19 17:42:31 PST
Crash under RemoteLayerTreePropertyApplier::applyProperties when reattaching to old process
Comment 1 Tim Horton 2019-02-19 17:42:51 PST
Created attachment 362461 [details]
Patch
Comment 2 Tim Horton 2019-02-19 17:42:52 PST
<rdar://problem/47944579>
Comment 3 Tim Horton 2019-02-19 17:45:15 PST
Created attachment 362462 [details]
Patch
Comment 4 Build Bot 2019-02-19 18:56:27 PST Comment hidden (obsolete)
Comment 5 Build Bot 2019-02-19 18:56:28 PST Comment hidden (obsolete)
Comment 6 Build Bot 2019-02-19 19:05:40 PST Comment hidden (obsolete)
Comment 7 Build Bot 2019-02-19 19:05:42 PST Comment hidden (obsolete)
Comment 8 Build Bot 2019-02-19 19:46:55 PST Comment hidden (obsolete)
Comment 9 Build Bot 2019-02-19 19:46:57 PST Comment hidden (obsolete)
Comment 10 Build Bot 2019-02-19 19:59:56 PST Comment hidden (obsolete)
Comment 11 Build Bot 2019-02-19 19:59:58 PST Comment hidden (obsolete)
Comment 12 Build Bot 2019-02-19 20:12:10 PST Comment hidden (obsolete)
Comment 13 Build Bot 2019-02-19 20:12:21 PST Comment hidden (obsolete)
Comment 14 Tim Horton 2019-02-19 21:07:55 PST
Oh... boy.
Comment 15 Tim Horton 2019-02-20 10:50:57 PST
Created attachment 362515 [details]
Patch
Comment 16 Tim Horton 2019-02-20 17:02:56 PST
Created attachment 362573 [details]
Patch
Comment 17 Tim Horton 2019-02-20 17:04:08 PST
Created attachment 362574 [details]
Patch
Comment 18 Simon Fraser (smfr) 2019-02-20 17:31:53 PST
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.
Comment 19 Tim Horton 2019-02-20 17:34:23 PST
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
Comment 20 Tim Horton 2019-02-20 17:48:02 PST
Created attachment 362578 [details]
Patch
Comment 21 Tim Horton 2019-02-20 19:00:54 PST
Created attachment 362583 [details]
Patch
Comment 22 Antti Koivisto 2019-02-21 10:17:35 PST
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.
Comment 23 WebKit Commit Bot 2019-02-21 13:34:32 PST
Comment on attachment 362583 [details]
Patch

Clearing flags on attachment: 362583

Committed r241899: <https://trac.webkit.org/changeset/241899>
Comment 24 WebKit Commit Bot 2019-02-21 13:34:34 PST
All reviewed patches have been landed.  Closing bug.
Comment 25 Truitt Savell 2019-02-22 11:32:33 PST
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
Comment 26 Tim Horton 2019-02-22 11:40:45 PST
(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!
Comment 27 Tim Horton 2019-02-22 13:13:43 PST
(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).
Comment 28 Tim Horton 2019-02-22 15:59:07 PST
Fixing the API test in https://bugs.webkit.org/show_bug.cgi?id=194963