| Summary: | [CoordinatedGraphics] Null dereference in CoordinatedGraphicsLayer::setCoordinatorIncludingSubLayersIfNeeded | ||||||||
|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Fujii Hironori <Hironori.Fujii> | ||||||
| Component: | Platform | Assignee: | Miguel Gomez <magomez> | ||||||
| Status: | RESOLVED FIXED | ||||||||
| Severity: | Normal | CC: | cgarcia, cmarcelo, commit-queue, ews-watchlist, kondapallykalyan, luiz, magomez, noam, webkit-bug-importer, zan | ||||||
| Priority: | P2 | Keywords: | InRadar | ||||||
| Version: | WebKit Nightly Build | ||||||||
| Hardware: | Unspecified | ||||||||
| OS: | Unspecified | ||||||||
| Attachments: |
|
||||||||
|
Description
Fujii Hironori
2019-03-12 03:58:46 PDT
PageOverlayController::m_viewOverlayRootLayer->m_coordinator is cleared in ~CompositingCoordinator. I think Miguel fixed this, or a very similar crash. Miguel, could you take a look. > Testing with: GTK port, trunk@242761, Debug build > > 1) Start MiniBrowser > 2) Go to https://youtube.com (to enter AC mode) > 3) Right click menu -> Inspect Element > 4) Close Web Inspector > 5) Go back > 6) Wait for five seconds (to destruct LayerTreeHost) > 7) Go forward (to YouTube again) > 8) Right click menu -> Inspect Element > 9) Web Process crashes I couldn't reproduce it exactly with those steps. For some reason opening the inspector and the closing it avoids leaving AC mode when going to the previous page. We may have a different issue there. Anyway, I was able to reproduce it by - start minibrowser - go to http://youtube.com and then back - wait 5 seconds - open the inspector The issue is the same. The overlay layers inside PageOverlayController are persistent and when AC is left, their coordinator is set to null. This is expected, and when they are added back to a tree with a valid coordinator, the code in CoordinatedGraphicsLayer::setCoordinatorIncludingSubLayersIfNeeded() will set the appropriate coordinator again. The thing is that, before a valid coordinator is set again, a new GraphicsLayer is added as child of the overlay layers (inside PageOverlayController::installPageOverlay), and that causes the crash cause the coordinator is null at that point. Created attachment 364391 [details]
Patch
Comment on attachment 364391 [details] Patch Attachment 364391 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/11480102 New failing tests: fast/scrolling/ios/overflow-scroll-overlap-2.html Created attachment 364449 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2
The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 364391 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=364391&action=review > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:981 > + if (!coordinator || m_coordinator == coordinator) You don't need the coordinator? Really? (In reply to Fujii Hironori from comment #7) > Comment on attachment 364391 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=364391&action=review > > > Source/WebCore/platform/graphics/texmap/coordinated/CoordinatedGraphicsLayer.cpp:981 > > + if (!coordinator || m_coordinator == coordinator) > > You don't need the coordinator? Really? You don't need it as long as the CoordinatedGraphicsLayer subtree is out of the main tree (the one with a valid coordinator). When this subtree with null coordinator is added to a tree with a valid one, the valid coordinator will be set for all the layers inside the subtree. Comment on attachment 364391 [details] Patch Clearing flags on attachment: 364391 Committed r242864: <https://trac.webkit.org/changeset/242864> All reviewed patches have been landed. Closing bug. |