1151663 - Optimization to layerize primary scrollable frame on page load is wrongly firing for non-primary scrollable frame

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

[Botond Ballo [:botond]]

STR:
  1. Load http://people.mozilla.org/~bballo/iframe.html
  2. Observe the initial layer tree on page load

Actual results:
  - On B2G, the iframe starts layerized.
  - On desktop (I tested on Linux), the iframe does not start
    layerized. It only becomes layerized after you start panning it.

Expected results:
  Layerization behaviour is the same on both platforms.
  It's not immediately clear to me which behaviour is the intended one.

Comment 1

[Botond Ballo [:botond]]

This layerization difference does not exist for http://people.mozilla.org/~bballo/div.html: there, the subframe starts off not layerized on both platforms.

Comment 2

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #1)
> This layerization difference does not exist for
> http://people.mozilla.org/~bballo/div.html: there, the subframe starts off
> not layerized on both platforms.

I lied - the difference exists there too! (I was judging by the handoff behaviour, and failed to take the scrollgrabbing container into account on B2G.)

Comment 3

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

Is e10s enabled in the desktop build you're testing on?

Comment 4

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Is e10s enabled in the desktop build you're testing on?

Good question! ni myself to check this when I'm back at my office desktop (can't get ssh with X forwarding to work, and I don't know how to check a pref value from the command line).

Comment 5

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #3)
> Is e10s enabled in the desktop build you're testing on?

I have e10s enabled, yes.

Should I also be testing things with e10s disabled?

Comment 6

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

No, e10s enabled is the case we care about for now.

Comment 7

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #0)
>   It's not immediately clear to me which behaviour is the intended one.

The layerization of the subframe on page load is unexpected. It's done by the optimization added in bug 982141 to layerize the primary scrollable frame; it's firing wrongly, because the subframe isn't the primary scrollable frame here (the root scroll frame is).

Comment 8

[Botond Ballo [:botond]]

This was regressed by bug 1013780. The patch in that bug added a builder.IsPaintingToWindow() flag to the condition required for a scrollable frame to be considered the primary scrollable frame. However, nsLayoutUtils::PaintFrame() sets this flag on the builder further down than the call to GetOrMaybeCreateDisplayPort() which checks it for the root scroll frame.

Comment 9

[Botond Ballo [:botond]]

The reason the same issue doesn't affect desktop is that on desktop, we have containerless scrolling for root scroll frames, and so the call to GetOrMaybeCreateDisplayPort() for the root scroll frame happens in ScrollFrameHelper::BuildDisplayList() (by which time the painting-to-window flag has been set) rather than in nsLayoutUtils::PaintFrame().

(Actually, the call to GetOrMaybeCreateDisplayPort() in nsLayoutUtils::PaintFrame() is not conditioned on containerless scrolling for root frames being off, so if it's on, as on desktop, GetOrMaybeCreateDisplayPort() gets called twice for the root scroll frame (with only the second call, in ScrollFrameHelper::BuildDisplayList(), picking up the painting-to-window flag). We should probably omit the first call in this case.)

Comment 10

[Botond Ballo [:botond]]

Attachment: MozReview Request: bz://1151663/botond

/r/7085 - Bug 1151663 - Initialize display list builder flags earlier in nsLayoutUtils::PaintFrame(). r=tn

Pull down this commit:

hg pull -r c021ed7e178aa6b681df435f2fce7863cf7f344c https://reviewboard-hg.mozilla.org/gecko/

Comment 11

[Botond Ballo [:botond]]

I'm also working on a test for this.

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8593036 [details]
MozReview Request: bz://1151663/botond

/r/7085 - Bug 1151663 - Only call GetOrMaybeCreateDisplayPort() in nsLayoutUtils::PaintFrame() if we are using containers for root scroll frames. r=tn

Pull down this commit:

hg pull -r 302c07bbdbc81ac73bdcfb0d5564840d72612953 https://reviewboard-hg.mozilla.org/gecko/

Comment 13

[Botond Ballo [:botond]]

MozReview, you are failing me!

Comment 14

[Botond Ballo [:botond]]

Attachment: Part 1 - Fix for the optimization misfiring

Comment 15

[Botond Ballo [:botond]]

Attachment: Part 2 - Fix for the issue mentioned in comment 9

Comment 16

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8593047 [details] [diff] [review]
Part 2 - Fix for the issue mentioned in comment 9

Note that this will also change the dirty rect we are using. This should be fine though. We made the same change for subdocuments. It will get the displayport when we enter the scroll frame. And fixed pos items will also get their proper dirty rect.

Comment 17

[Botond Ballo [:botond]]

Attachment: Part 3 - Extract some helper functions for writing APZ mochitests into a separate file

Comment 18

[Botond Ballo [:botond]]

Attachment: Part 4 - Fix reconstruction of APZC tree structure in APZ mochitests

There was a bug in the code for communicating the APZ tree structure to APZ mochitests, introduced by parent-process-apz: the parent of the root APZC in the content process would incorrectly be picked up as the root APZC in the content process, causing the mochitest to misinterpret the APZC tree structure.

It so happened that the misinterpreted tree structure still satisfied the constraints of the bug 982141 mochitest, so it continued passing.

The problem came to light during my attempt to write a mochitest for this bug, though. This patch fixes it.

Comment 19

[Botond Ballo [:botond]]

Attachment: Part 5 - New mochitest for the optimization-misfiring issue

Comment 20

[Botond Ballo [:botond]]

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cd29aae9fae

Comment 21

[Botond Ballo [:botond]]

(In reply to Botond Ballo [:botond] from comment #20)
> Try push:
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cd29aae9fae

Looks green except for Linux build failures, which https://treeherder.mozilla.org/#/jobs?repo=try&revision=8f95fd0ea644 demonstrates are unrelated.

Comment 22

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

Comment on attachment 8595085 [details] [diff] [review]
Part 3 - Extract some helper functions for writing APZ mochitests into a separate file

Review of attachment 8595085 [details] [diff] [review]:
-----------------------------------------------------------------

r+ assuming this was a straight move of code, no changes.

Comment 23

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

Comment on attachment 8595087 [details] [diff] [review]
Part 4 - Fix reconstruction of APZC tree structure in APZ mochitests

Review of attachment 8595087 [details] [diff] [review]:
-----------------------------------------------------------------

Let me know if I misunderstood something...

::: gfx/layers/apz/test/apz_test_utils.js
@@ +94,1 @@
>        addLink(root, scrollId, paint[scrollId]["parentScrollId"]);

I don't think this works as intended. For one thing, you can have the same scrollId in multiple layers subtrees, so this addLink call may attach it to the wrong parent. And secondly calling addRoot for the "isRootForLayersId" means that the layers subtrees will get reparented to the root rather than corresponding to the position in the C++ tree. It might just be better to export the "layersId|scrollId" combo from the C++ code and use that as a unique identifier here.

Comment 24

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

Comment on attachment 8595089 [details] [diff] [review]
Part 5 - New mochitest for the optimization-misfiring issue

Review of attachment 8595089 [details] [diff] [review]:
-----------------------------------------------------------------

Nice test, thanks!

::: gfx/layers/apz/test/mochitest.ini
@@ +5,4 @@
>  support-files =
>    apz_test_utils.js
>    helper_bug982141.html
> +  helper_bug1151663.html

Better to do this:

[DEFAULT]
support-files =
  apz_test_utils.js
[test_bug982141.html]
skip-if...
support-files =
  helper_bug982141.html
[test_bug1151663.html]
skip-if...
support-files =
  helper_bug1151663.html

Comment 25

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #23)
> Comment on attachment 8595087 [details] [diff] [review]
> Part 4 - Fix reconstruction of APZC tree structure in APZ mochitests
> 
> Review of attachment 8595087 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Let me know if I misunderstood something...

The C++ code that logs the isRootForLayersId and parentScrollId messages, only does so for the layer subtree of the content process that originated the layer transaction [1]. Therefore, the APZC tree structure that the JS code reconstructs, is just the subtree corresponding to that layer subtree. (I'll update the comments in the JS code to make this more clear.)

Let me know if this addresses your concerns.

[1] http://mxr.mozilla.org/mozilla-central/source/gfx/layers/apz/src/APZCTreeManager.cpp?rev=6ee1d3e056b3#453

Comment 26

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

Comment on attachment 8595087 [details] [diff] [review]
Part 4 - Fix reconstruction of APZC tree structure in APZ mochitests

Review of attachment 8595087 [details] [diff] [review]:
-----------------------------------------------------------------

Doh! Yup that works then. Thanks.

Comment 27

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2fab0c1ed863
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6e788ba5506
https://hg.mozilla.org/integration/mozilla-inbound/rev/1f578c4228ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/fbbca69d29e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4cd578366a91

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/726a442167d9

Comment 29

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #24)
> ::: gfx/layers/apz/test/mochitest.ini
> @@ +5,4 @@
> >  support-files =
> >    apz_test_utils.js
> >    helper_bug982141.html
> > +  helper_bug1151663.html
> 
> Better to do this:
> 
> [DEFAULT]
> support-files =
>   apz_test_utils.js
> [test_bug982141.html]
> skip-if...
> support-files =
>   helper_bug982141.html
> [test_bug1151663.html]
> skip-if...
> support-files =
>   helper_bug1151663.html

This doesn't seem to work - the tests are busted with an error that suggests that apz_test_utils.js is not being found. That suggests the later support-files clauses are overwriting the [DEFAULT] one, not adding to it.

The push in comment 28 moves all support files into a single support-files clause under [DEFAULT].

Comment 30

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

Oh interesting. Well that's fine too.

Comment 31

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2fab0c1ed863
https://hg.mozilla.org/mozilla-central/rev/b6e788ba5506
https://hg.mozilla.org/mozilla-central/rev/1f578c4228ef
https://hg.mozilla.org/mozilla-central/rev/fbbca69d29e8
https://hg.mozilla.org/mozilla-central/rev/4cd578366a91
https://hg.mozilla.org/mozilla-central/rev/726a442167d9

Comment 32

[Botond Ballo [:botond]]

The issue fixed by the Part 1 patch is a regression dating back to B2G 2.0. The regression is that we layerize more than we intend to on initial page load (on a page where the page's root scroll frame is scrollable, we still layerize the first scrollable subframe we find). It shouldn't have correctness implications, but it could mean using more memory than we need to.

Kats, do you think it's worth pursuing uplift for this fix?

Comment 33

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

We could uplift to 2.2 but even for that I'm not really sure it's worth it. In cases where the root frame is scrollable I would think it unlikely that the first scrollable subframe is very large. Also fixing this may expose other issues that were previously masked which we would then have to uplift as well.

Comment 34

[Alaa Emad]

Attachment: Make test_Bug 1151663.html use runSubtestsSeriallyInFreshWindows

Comment 35

[Phabricator Automation]

Comment on attachment 9180884 [details]
Make test_Bug 1151663.html use runSubtestsSeriallyInFreshWindows

Revision D93160 was moved to bug 1444611. Setting attachment 9180884 [details] to obsolete.