1461122 - Autoscrolling does not work on about:support

Status: VERIFIED FIXED

(Core :: Graphics: WebRender, defect, P1)

Description

[Darkspirit]

Attachment: 2018-05-12_19-32-02.mp4

Debian Testing, KDE, Radeon RX480
Win10 1803, GeForce GTX 1060
2560x1440 (Dell U2515H)

Attached video: 
1. Middle click, moving my mouse down. Nothing happens. Left click.
2. Ctrl+T (Painting is stuck), Ctrl+T.

gfx.webrender.async-scene-build:0 does not help.

mozregression --good 2018-05-11 --bad 2018-05-12 --pref gfx.webrender.all:true general.autoScroll:true startup.homepage_welcome_url:'about:support'
> 6:08.77 INFO: Last good revision: 2083171b3ecbc178bc5eaebe629a28800635bd1b
> 6:08.77 INFO: First bad revision: 61f51684cc29e527504f1ec72824bf5b53fa1853
> 6:08.77 INFO: Pushlog:
> https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=2083171b3ecbc178bc5eaebe629a28800635bd1b&tochange=61f51684cc29e527504f1ec72824bf5b53fa1853

> 61f51684cc29	Kartikaya Gupta — Bug 1452390 - Implement paint-skipping support in WebRender. r=botond
> d8b7069bf6ca	Kartikaya Gupta — Bug 1452390 - Make WebRenderScrollData::HasMetadataFor work properly on the parent side. r=botond

Comment 1

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

So.. the call to DidComposite at [1], when called from the root WRBP, does nothing. Because that DidComposite function that's getting called is a no-op [2][3]. So when we send a series of empty transactions over with no WR commands but with scroll offset updates, the first one of that series will have sendDidComposite=true at [4], add an entry into mPendingTransactionIds, and not actually flush it to the content side. Because the content side believes the transaction is still pending it doesn't fire rAF, so the main-thread autoscroll loop at [5] doesn't loop.

It seems to me that calling DidComposite(LayersId, ...) on the root CompositorBridgeParent should work. Fixing that alone doesn't solve the problem fully for me though, still investigating.

[1] https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/gfx/layers/wr/WebRenderBridgeParent.cpp#761
[2] https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/gfx/layers/ipc/CompositorBridgeParent.h#581
[3] https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/gfx/layers/ipc/CompositorBridgeParent.h#141
[4] https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/gfx/layers/wr/WebRenderBridgeParent.cpp#754
[5] https://searchfox.org/mozilla-central/rev/a85db9e29eb3f022dbaf8b9a6390ecbacf51e7dd/toolkit/content/browser-content.js#312

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a08b5367ac0bd6a216febc011cc6ce5808c9c9c

Comment 3

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

Attachment: Bug 1461122 - Implement DidComposite(LayersId,...) for CompositorBridgeParent.

Review commit: https://reviewboard.mozilla.org/r/243830/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243830/

Comment 4

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

Attachment: Bug 1461122 - Clean up the composition flow for empty transactions.

This is mostly just refactoring to make the code more understandable. However,
there are a couple of functional changes:
- If we have scroll offset updates we now schedule a composite instead of
  sending the DidComposite right away. This is needed because we want to
  actually composite the scroll offset change.
- If there are WebRenderParentCommands provided, we process them and update the
  epoch in a single transaction, instead of splitting it across two transactions
  for no good reason.

Review commit: https://reviewboard.mozilla.org/r/243832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243832/

Comment 5

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

Attachment: Bug 1461122 - Ensure we recomposite when getting a main-thread scroll offset update.

With non-WebRender, the composite is triggered unconditionally when we receive
the transaction from the content side, so we never needed this. The previous
patch adds the composite for the equivalent WebRender codepath, but it's better
to do it explicitly in NotifyLayersUpdated as well. The reason for this is that
with WebRender, this code runs on the updater thread which is not the same as
the compositor thread - so the composition that is scheduled from
WebRenderBridgeParent upon receiving the transaction might happen before the
scroll offset update is actually applied. Triggering a composition from
NotifyLayersUpdated should be a no-op in the cases where a composition is already
scheduled, but in cases where it has not been scheduled, it will schedule one
to ensure the scroll offset update gets composited properly.

Review commit: https://reviewboard.mozilla.org/r/243834/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/243834/

Comment 6

[Botond Ballo [:botond]]

Comment on attachment 8975592 [details]
Bug 1461122 - Ensure we recomposite when getting a main-thread scroll offset update.

https://reviewboard.mozilla.org/r/243834/#review249752

Comment 7

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8975591 [details]
Bug 1461122 - Clean up the composition flow for empty transactions.

https://reviewboard.mozilla.org/r/243832/#review249890

Comment 8

[Sotaro Ikeda [:sotaro]]

Comment on attachment 8975590 [details]
Bug 1461122 - Implement DidComposite(LayersId,...) for CompositorBridgeParent.

https://reviewboard.mozilla.org/r/243830/#review249888

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0efbd05857c2
Implement DidComposite(LayersId,...) for CompositorBridgeParent. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/9f87280ba566
Clean up the composition flow for empty transactions. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/90445f7b62cd
Ensure we recomposite when getting a main-thread scroll offset update. r=botond

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

Backed out 3 changesets (bug 1461122) for build bustages on workspace/build/src/obj-firefox/dist/include/mozilla/layers/CompositorBridgeParent.h 

Backout link: https://hg.mozilla.org/integration/autoland/rev/433e044b2702e625381b29c4b97de6d0322ade17 

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=90445f7b62cdb1deda8e3a860032c9f113de3262

Recent log: https://treeherder.mozilla.org/logviewer.html#?job_id=178533466&repo=autoland&lineNumber=13088

Comment 11

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

Comment on attachment 8975590 [details]
Bug 1461122 - Implement DidComposite(LayersId,...) for CompositorBridgeParent.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243830/diff/1-2/

Comment 12

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

Comment on attachment 8975591 [details]
Bug 1461122 - Clean up the composition flow for empty transactions.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243832/diff/1-2/

Comment 13

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

Comment on attachment 8975592 [details]
Bug 1461122 - Ensure we recomposite when getting a main-thread scroll offset update.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/243834/diff/1-2/

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0b04c87960ad
Implement DidComposite(LayersId,...) for CompositorBridgeParent. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/1232f9a7403c
Clean up the composition flow for empty transactions. r=sotaro
https://hg.mozilla.org/integration/autoland/rev/232e90a05b02
Ensure we recomposite when getting a main-thread scroll offset update. r=botond

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/0b04c87960ad
https://hg.mozilla.org/mozilla-central/rev/1232f9a7403c
https://hg.mozilla.org/mozilla-central/rev/232e90a05b02

Comment 16

[Darkspirit]

Verified fixed in Nightly 62 x64 20180516100125 de_DE @ Debian Testing (Radeon RX480). Thanks!