Bug 202256 - ScrollingTreeScrollingNode: use LayerRepresentation for scroll container, scrolled contents layers
Summary: ScrollingTreeScrollingNode: use LayerRepresentation for scroll container, scr...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: InRadar
Depends on: 202344
Blocks: 198475
  Show dependency tree / graph
 
Reported: 2019-09-25 22:21 PDT by Zan Dobersek
Modified: 2019-09-29 22:15 PDT (History)
10 users (show)

See Also:

Attachments
Patch (3.45 KB, patch)
2019-09-25 22:24 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
WIP (5.22 KB, patch)
2019-09-26 05:42 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Patch (5.52 KB, patch)
2019-09-26 06:19 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Patch (5.69 KB, patch)
2019-09-29 10:42 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
WIP (8.93 KB, patch)
2019-09-29 11:24 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Patch (8.29 KB, patch)
2019-09-29 12:22 PDT, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Show Obsolete (5) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Zan Dobersek 2019-09-25 22:21:12 PDT 
ScrollingTreeScrollingNode: use LayerRepresentation for scroll container, scrolled contents layers
Comment 1 Zan Dobersek 2019-09-25 22:24:16 PDT 
Created attachment 379620 [details]
Patch
Comment 2 Simon Fraser (smfr) 2019-09-26 04:05:51 PDT 
I'll review when the tests are passing.
Comment 3 Zan Dobersek 2019-09-26 05:42:23 PDT 
Created attachment 379635 [details]
WIP
Comment 4 Zan Dobersek 2019-09-26 06:19:57 PDT 
Created attachment 379636 [details]
Patch
Comment 5 Zan Dobersek 2019-09-26 10:04:41 PDT 
(In reply to Simon Fraser (smfr) from comment #2)
> I'll review when the tests are passing.

Builds fine now. The mac-debug-wk1 failure is not related.
Comment 6 Zan Dobersek 2019-09-26 23:19:54 PDT 
Comment on attachment 379636 [details]
Patch

Clearing flags on attachment: 379636

Committed r250415: <https://trac.webkit.org/changeset/250415>
Comment 7 Zan Dobersek 2019-09-26 23:19:59 PDT 
All reviewed patches have been landed.  Closing bug.
Comment 8 Radar WebKit Bug Importer 2019-09-26 23:20:18 PDT 
<rdar://problem/55772092>
Comment 9 Truitt Savell 2019-09-27 09:22:46 PDT 
I looks like the change in https://trac.webkit.org/changeset/250415/webkit

caused iOS debug testing to exit early with 50 crashes and 850 API failures.

Build:
https://build.webkit.org/builders/Apple%20iOS%2013%20Simulator%20Debug%20WK2%20%28Tests%29/builds/160

This will have to be rolled out.
Comment 10 Truitt Savell 2019-09-27 09:25:40 PDT 
Reverted r250415 for reason:

Broke iOS debug testing with 50 crashes and 850 API failure

Committed r250427: <https://trac.webkit.org/changeset/250427>
Comment 11 Zan Dobersek 2019-09-29 02:15:39 PDT 
With the switch to LayerRepresentation, an object of this type was used in a null check here:
https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/UIProcess/RemoteLayerTree/ios/ScrollingTreeFrameScrollingNodeRemoteIOS.mm#L69

Before the patch, this was testing nullness of a RetainPtr, which works fine. But with LayerRepresentation, the boolean evaluation ended up using the GraphicsLayer::PlatformLayerID operator to retrieve an integral value that could be tested in a boolean context. With the LayerRepresentation object containing a platform layer pointer, the asserts occurred.

Bug #202344 has a patch that makes the LayerRepresentation operators explicit for safer but more verbose use, as well as adds a new operator that would be used in boolean contexts, avoiding the problem that caused the regression.
Comment 12 Zan Dobersek 2019-09-29 10:42:06 PDT 
Created attachment 379813 [details]
Patch

Same patch up for re-review, with no problems expected now due to changes in bug #202344.
Comment 13 Zan Dobersek 2019-09-29 11:24:29 PDT 
Created attachment 379816 [details]
WIP

Never mind, some additional iOS changes are required.
Comment 14 Zan Dobersek 2019-09-29 12:22:06 PDT 
Created attachment 379817 [details]
Patch

Now ready.
Comment 15 Zan Dobersek 2019-09-29 22:15:42 PDT 
Comment on attachment 379817 [details]
Patch

Clearing flags on attachment: 379817

Committed r250498: <https://trac.webkit.org/changeset/250498>
Comment 16 Zan Dobersek 2019-09-29 22:15:46 PDT 
All reviewed patches have been landed.  Closing bug.