1610731 - Implement webrender backend for the interaction between position:sticky and dynamic toolbar

Status: RESOLVED FIXED

(Core :: Graphics: WebRender, enhancement, P3)

Description

[Hiroyuki Ikezoe (:hiro)]

I am trying to implement the backend for non-webrender in bug 1594451. This is a bug for webrender backend.

Comment 1

[Botond Ballo [:botond]]

Here's an outline of what I believe needs to be done here:

• Add nsDisplayStickyPosition::UpdateScrollData() (similar to nsDisplayFixedPosition::UpdateScrollData()) to populate the missing sticky-position related fields of WebRenderScrollDataWrapper.

• Update APZCTreeManager::SampleForWebRender() to provide transforms for sticky layers to offset them for dynamic toolbar movement, similar to how it does so for fixed layers. (The calculation of the transform for sticky layers is similar to the existing calculation done in ComputeTransformForNode().)

Comment 2

[Jessie [:jbonisteel] pls NI]

Just marking this as blocking 75 for now - since WebRender will not be enabled anywhere for Fenix GA this doesn't need to block but getting this on the radar so we can figure out when to prioritize.

Comment 3

[Kris Taeleman (:ktaeleman)]

Attachment: Bug 1610731 - Implement UpdateScrollData for sticky position.

Comment 4

[Kris Taeleman (:ktaeleman)]

Hey Botond, I started looking into this and had some initial questions. I've uploaded them in phabricator here:
https://phabricator.services.mozilla.com/D64494

Comment 5

[Botond Ballo [:botond]]

Thanks for looking at this! I answered the questions in the review.

Comment 6

[Kris Taeleman (:ktaeleman)]

For the second part that calculates the transforms, the calculation in ComputeTransformForNode() uses aNode->GetFixedPosSides() & SideBits::eBottom.
That seems to get set here for example, but that data would only be filled in in nsDisplayFixedPosition::UpdateScrollData(), which wouldn't run for sticky position.
I started including the fixed side calculation in stickyposition data in nsDisplayStickyPosition::UpdateScrollData()) and setting a StickyPositionSides, but I'm not sure if that's correct or if I can count on the GetFixedPosSides() being available in the HitTestingNode and don't have to set that for sticky.

Comment 7

[Botond Ballo [:botond]]

The non-WR codepath reuses the FixedPositionSides field for sticky-pos as well (it's set here). We can probably do the same.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

Kris handed this off to me, so reassigning. Latest try push is at https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&revision=d09cc01a7d639475969c4b9ad17464746a0a6f65 which I expect to be all green. Patches incoming.

Comment 9

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1610731 - Replace inefficient cargo-culted code with more efficient version. r?botond

Instead of storing pointers to the nodes in the TreeBuildingState, and then
extracting information from them later, we can just extract the information
right away, put that in the TreeBuildingState, and move the final structure
into place.

This isn't possible with some of the other similar-looking structures that this
was presumably cargo-culted from (such as the one that holds the thumb
information) since those need to be able to look up references to other APZCs
which can only be done after the tree walk is complete.

Comment 10

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1610731 - Remove GetIsStickyPosition and clean up some TODOs. r?botond

The GetIsStickyPosition function isn't really needed since we can distinguish
whether or not a layer is sticky via NULL_SCROLL_ID as the container id. Also
ensure we have proper AtBottomLayer checks where needed for fixed and sticky
data.

Depends on D69553

Comment 11

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1610731 - Add plumbing for sticky data. r?botond

This patch is pretty uninteresting, just building the pipe to move data
from the main-thread to APZ.

Depends on D69554

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1610731 - Extract a HasDynamicToolbar helper. r?botond

Also use this to restrict when we send fixed-position data to APZ with WR
enabled, since we only need it when there's a dynamic toolbar involved.

Depends on D69555

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1610731 - Populate the sticky data from the layout end. r?botond

Depends on D69556

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1610731 - Incorporate the top/bottom margins set by reftests. r?botond

This makes the existing test for this codepath start passing on geckoview-qr.

Depends on D69557

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1610731 - Adjust the sticky positioned elements transform for the dynamic toolbar. r?botond

Depends on D69558

Comment 16

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1610731 - Add more tests for position:fixed and position:sticky. r?botond

Note that the sticky-top test still fails on non-WebRender, but that's outside
the scope of this bug.

Depends on D69559

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/035432f43d16
Replace inefficient cargo-culted code with more efficient version. r=botond
https://hg.mozilla.org/integration/autoland/rev/8d43fbe05f38
Remove GetIsStickyPosition and clean up some TODOs. r=botond
https://hg.mozilla.org/integration/autoland/rev/8f2bae2ca274
Add plumbing for sticky data. r=botond
https://hg.mozilla.org/integration/autoland/rev/a383a7100973
Extract a HasDynamicToolbar helper. r=botond
https://hg.mozilla.org/integration/autoland/rev/7bffd6eb9ac7
Populate the sticky data from the layout end. r=botond
https://hg.mozilla.org/integration/autoland/rev/840550c070e8
Incorporate the top/bottom margins set by reftests. r=botond
https://hg.mozilla.org/integration/autoland/rev/c33e6fe22774
Adjust the sticky positioned elements transform for the dynamic toolbar. r=botond
https://hg.mozilla.org/integration/autoland/rev/a26b63d0248e
Add more tests for position:fixed and position:sticky. r=botond

Comment 18

[Cristian Brindusan [:cbrindusan]]

Backed out 8 changesets (Bug 1610731) for reftest failures at dynamic-toolbar-fixed-bottom-1.html.
https://hg.mozilla.org/integration/autoland/rev/8b41511236b7c7489718fd4826427f04ff0c24a3

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&revision=a26b63d0248e778fc5e9ea5323081bcbc7bdd673&selectedJob=296204771

Failure log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296204771&repo=autoland&lineNumber=12027

Comment 19

[Kartikaya Gupta (email:[e-mail])]

Looks like I accidentally deleted the test file? Should be easy to fix, will do it in a bit.

Comment 20

[Kartikaya Gupta (email:[e-mail])]

The file deletion appears to be a moz-phab or phabricator bug.

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e7d6f26c1019
Replace inefficient cargo-culted code with more efficient version. r=botond
https://hg.mozilla.org/integration/autoland/rev/4162437c7931
Remove GetIsStickyPosition and clean up some TODOs. r=botond
https://hg.mozilla.org/integration/autoland/rev/c2b1bd759595
Add plumbing for sticky data. r=botond
https://hg.mozilla.org/integration/autoland/rev/d8fbbc7fc65d
Extract a HasDynamicToolbar helper. r=botond
https://hg.mozilla.org/integration/autoland/rev/0e5823826e91
Populate the sticky data from the layout end. r=botond
https://hg.mozilla.org/integration/autoland/rev/bcbf21dcd7b4
Incorporate the top/bottom margins set by reftests. r=botond
https://hg.mozilla.org/integration/autoland/rev/089ef5398b32
Adjust the sticky positioned elements transform for the dynamic toolbar. r=botond
https://hg.mozilla.org/integration/autoland/rev/8d11e5caff2a
Add more tests for position:fixed and position:sticky. r=botond

Comment 22

[Andrei Ciure[:aciure]]

Backed out 8 changesets (bug 1610731) for causing fullscreen related wpt failures
https://hg.mozilla.org/integration/autoland/rev/26bcd52f3e7664622e8a24ebf049cbcd91c35b74

push that caused the backout: https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cweb%2Cplatform%2Ctests%2Ctest-linux1804-64%2Fdebug-web-platform-tests-e10s-13%2Cw%28wpt13%29&fromchange=c526677e5ffb36954ca1a31acfb97b49fbb90f08&tochange=26bcd52f3e7664622e8a24ebf049cbcd91c35b74

log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=296250023&repo=autoland&lineNumber=25178

[task 2020-04-04T04:41:23.627Z] 04:41:23 INFO - PID 22399 | calling context
[task 2020-04-04T04:41:23.627Z] 04:41:23 INFO - PID 22399 | [stack trace unavailable]
[task 2020-04-04T04:41:23.628Z] 04:41:23 INFO - PID 22399 | ###!!! Deadlock may happen NOW!
[task 2020-04-04T04:41:23.629Z] 04:41:23 INFO - PID 22399 | [Parent 22399, Main Thread] ###!!! ASSERTION: Potential deadlock detected:
[task 2020-04-04T04:41:23.629Z] 04:41:23 INFO - PID 22399 | Cyclical dependency starts at
[task 2020-04-04T04:41:23.629Z] 04:41:23 INFO - PID 22399 | RecursiveMutex : APZCTreeLock (currently acquired)
[task 2020-04-04T04:41:23.629Z] 04:41:23 INFO - PID 22399 | Next dependency:
[task 2020-04-04T04:41:23.630Z] 04:41:23 INFO - PID 22399 | Mutex : APZCMapLock (currently acquired)
[task 2020-04-04T04:41:23.630Z] 04:41:23 INFO - PID 22399 | Cycle completed at
[task 2020-04-04T04:41:23.630Z] 04:41:23 INFO - PID 22399 | RecursiveMutex : APZCTreeLock (currently acquired)
[task 2020-04-04T04:41:23.631Z] 04:41:23 INFO - PID 22399 | ###!!! Deadlock may happen NOW!
[task 2020-04-04T04:41:23.631Z] 04:41:23 INFO - PID 22399 | : 'Error', file /builds/worker/checkouts/gecko/xpcom/threads/BlockingResourceBase.cpp, line 248
[task 2020-04-04T04:41:23.887Z] 04:41:23 INFO - PID 22399 | [Child 25427, Main Thread] WARNING: NS_ENSURE_TRUE(mPresShell) failed: file /builds/worker/checkouts/gecko/layout/base/nsPresContext.cpp, line 844
[task 2020-04-04T04:41:23.927Z] 04:41:23 INFO - PID 22399 | [Child 25397, Main Thread] WARNING: Extra shutdown CC: 'i < NORMAL_SHUTDOWN_COLLECTIONS', file /builds/worker/checkouts/gecko/xpcom/base/nsCycleCollector.cpp, line 3352
[task 2020-04-04T04:41:23.976Z] 04:41:23 INFO - PID 22399 | nsStringStats
[task 2020-04-04T04:41:23.977Z] 04:41:23 INFO - PID 22399 | => mAllocCount: 10042
[task 2020-04-04T04:41:23.977Z] 04:41:23 INFO - PID 22399 | => mReallocCount: 0
[task 2020-04-04T04:41:23.978Z] 04:41:23 INFO - PID 22399 | => mFreeCount: 10042
[task 2020-04-04T04:41:23.978Z] 04:41:23 INFO - PID 22399 | => mShareCount: 8383
[task 2020-04-04T04:41:23.978Z] 04:41:23 INFO - PID 22399 | => mAdoptCount: 557
[task 2020-04-04T04:41:23.979Z] 04:41:23 INFO - PID 22399 | => mAdoptFreeCount: 561
[task 2020-04-04T04:41:23.979Z] 04:41:23 INFO - PID 22399 | => Process ID: 25397, Thread ID: 140501024642944
[task 2020-04-04T04:41:23.994Z] 04:41:23 INFO - PID 22399 | [Parent 22399, Main Thread] WARNING: NS_ENSURE_TRUE(!aStringURI.IsEmpty()) failed: file /builds/worker/checkouts/gecko/docshell/base/nsDefaultURIFixup.cpp, line 95
[task 2020-04-04T04:41:24.259Z] 04:41:24 INFO - PID 22399 | ### XPCOM_MEM_BLOAT_LOG defined -- logging bloat/leaks to /tmp/tmpUiWV0m/runtests_leaks_977_tab_pid25457.log
[task 2020-04-04T04:41:24.324Z] 04:41:24 INFO - PID 22399 | [Parent 22399, Main Thread] WARNING: '!mName', file /builds/worker/checkouts/gecko/editor/libeditor/EditAggregateTransaction.cpp, line 92
[task 2020-04-04T04:41:24.324Z] 04:41:24 INFO - PID 22399 | [Parent 22399, Main Thread] WARNING: EditAggregationTransaction::GetName() failed: 'NS_SUCCEEDED(rv)', file /builds/worker/checkouts/gecko/editor/libeditor/PlaceholderTransaction.cpp, line 217
[task 2020-04-04T04:41:24.325Z] 04:41:24 INFO - PID 22399 | [Parent 22399, Main Thread] WARNING: nsIAbsorbingTransaction::GetTxnName() failed, but ignored: 'NS_SUCCEEDED(rvIgnored)', file /builds/worker/checkouts/gecko/editor/libeditor/PlaceholderTransaction.cpp, line 188
[task 2020-04-04T04:41:24.543Z] 04:41:24 INFO - PID 22399 | Couldn't convert chrome URL: chrome://branding/locale/brand.properties
[task 2020-04-04T04:41:24.745Z] 04:41:24 INFO - PID 22399 | [Child 25457, Main Thread] WARNING: could not set real-time limit at process startup: file /builds/worker/checkouts/gecko/dom/ipc/ContentChild.cpp, line 1692
[task 2020-04-04T04:41:25.004Z] 04:41:25 INFO - Got chrome assert count 1
[task 2020-04-04T04:41:25.020Z] 04:41:25 INFO - TEST-UNEXPECTED-FAIL | /feature-policy/reporting/fullscreen-reporting.html | assertion count 1 is more than expected 0 assertions
[task 2020-04-04T04:41:25.021Z] 04:41:25 INFO - .
[task 2020-04-04T04:41:25.021Z] 04:41:25 INFO - TEST-OK | /feature-policy/reporting/fullscreen-reporting.html | took 1499ms

Comment 23

[Kartikaya Gupta (email:[e-mail])]

Thanks for catching that. Looks like a simple fix, GetCompositorFixedLayerMargins should not need the tree lock. I have it fixed locally but I'll wait until after the soft freeze is over before re-landing.

Comment 24

[Kartikaya Gupta (email:[e-mail])]

https://treeherder.mozilla.org/#/jobs?repo=try&group_state=expanded&selectedJob=296290191&revision=84dbf05cf9b70b83c7da63dae6607e40b8dec8dc

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d3d50cfeaf25
Replace inefficient cargo-culted code with more efficient version. r=botond
https://hg.mozilla.org/integration/autoland/rev/a1efc63443c8
Remove GetIsStickyPosition and clean up some TODOs. r=botond
https://hg.mozilla.org/integration/autoland/rev/51e7e24af4f9
Add plumbing for sticky data. r=botond
https://hg.mozilla.org/integration/autoland/rev/9d7f4313698f
Extract a HasDynamicToolbar helper. r=botond
https://hg.mozilla.org/integration/autoland/rev/f2c15ffc5b0d
Populate the sticky data from the layout end. r=botond
https://hg.mozilla.org/integration/autoland/rev/6dfe10c68ad2
Incorporate the top/bottom margins set by reftests. r=botond
https://hg.mozilla.org/integration/autoland/rev/b49c0ab3d6e7
Adjust the sticky positioned elements transform for the dynamic toolbar. r=botond
https://hg.mozilla.org/integration/autoland/rev/a655b64d1ce1
Add more tests for position:fixed and position:sticky. r=botond

Comment 26

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/d3d50cfeaf25
https://hg.mozilla.org/mozilla-central/rev/a1efc63443c8
https://hg.mozilla.org/mozilla-central/rev/51e7e24af4f9
https://hg.mozilla.org/mozilla-central/rev/9d7f4313698f
https://hg.mozilla.org/mozilla-central/rev/f2c15ffc5b0d
https://hg.mozilla.org/mozilla-central/rev/6dfe10c68ad2
https://hg.mozilla.org/mozilla-central/rev/b49c0ab3d6e7
https://hg.mozilla.org/mozilla-central/rev/a655b64d1ce1

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Attachment: Bug 1610731 - Followup to fix IPC for RectAbsolute. r?ktaeleman

The parameters to the middle two arguments of SetBox were flipped, causing
RectAbsolute to get improperly swizzled over IPC.

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6b6662425ad8
Followup to fix IPC for RectAbsolute. r=ktaeleman,botond

Comment 29

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/6b6662425ad8