1195431 - Layerization differences cause intermittent reftest failures on Linux R-e10s with APZ enabled

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Copied from bug 1178359 comment 4:

I looked into one of the remaining failures briefly and I think there's another intermittent failure very similar to bug 1189010, except in this case it's the IsScrollingActive call at [1] that's the problem. This call depends on the value of mShouldBuildScrollableLayer in the scrollframe, but that doesn't get set (on the first layout) until after the call. So needsOwnLayer might get set to false on the first layout but true on the second layout. The layerization difference can result in the reftest failure.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp?rev=5a57076e5380#467

Comment 1

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

Attachment: WIP

This applies on top of the patches in bug 1204535. Although it does fix the bug, I'm not really happy with this patch. Posting it here in case anybody wants to steal it or build on it. The active ingredient in the patch is the change to IsScrollingActive so that it returns true to nsSubDocumentFrame even before the first call to BuildDisplayList. The rest of the patch is just trying to minimize code duplication and having multiple code paths that need to be kept in sync.

Comment 2

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

Attachment: Part 1 - Extract a couple of helper methods

I made it a tiny bit better, and split up the patch so the changes are clearer. Open to feedback for improving this still, but if we don't have anything better I'd like to get something in at least so we can keep moving towards turning on APZ on linux.

Comment 3

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

Attachment: Part 2 - Extract a new helper

Comment 4

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

Attachment: Part 3 - Make IsScrollingActive work before BuildDisplayList is called

Try push pending at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8d2f371a580

Comment 5

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

And a more comprehensive try push to make sure the assertions aren't firing on B2G/Fennec either:

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

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8661841 [details] [diff] [review]
Part 2 - Extract a new helper

>+bool
>+ScrollFrameHelper::WillUseDisplayPort(const nsDisplayListBuilder* aBuilder) const
>+{
>+  if (IsUsingDisplayPort(aBuilder)) {
>+    return true;
>+  }
>+
>+  if (aBuilder->GetIgnoreScrollFrame() == mOuter || IsIgnoringViewportClipping()) {
>+    return false;
>+  }
>+
>+  if (mIsRoot && gfxPrefs::LayoutUseContainersForRootFrames()) {
>+    return false;
>+  }
>+
>+  return aBuilder->IsPaintingToWindow()
>+    && nsLayoutUtils::WantDisplayPort(aBuilder, mOuter);
>+}

I don't quite understand what WillUseDisplayPort is trying to accomplish.

It seems like the semantic meaning is intended to be "if this scroll frame would use a display port if one were created by GetOrMaybeCreateDisplayPort".

In which case I don't think it's achieving that goal. For root scrollframes with containerful scrolling (mIsRoot && gfxPrefs::LayoutUseContainersForRootFrames()) we will call GetOrMaybeCreateDisplayPort in nsSubdocumentFrame::BuildDisplayList or nsLayoutUtils::PaintFrame. So returning false in that case would be wrong because we could create a displayport for this scroll frame, and we would use it.

Comment 7

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

The purpose of that function was basically to extract something that would correctly predict whether or not the usingDisplayPort variable gets set to true inside the BuildDisplayList function. I needed that because the mShouldBuildScrollableLayer value is dependent on usingDisplayPort, and so in order to implement WillBuildScrollableLayer as a standalone function I needed to extract a standalone function for computing usingDisplayPort.

For the purposes of this bug the code I cared about is the block of code highlighted at [1]. At the top of the block is the call to IsScrollingActive() and at the bottom of the block is the call to build the display list. The scrollframe's mShouldBuildScrollableLayer gets computed at the bottom of the block but we need the value at the top of the block, hence I added new functions to allow that computation.

The call to GetOrMaybeCreateDisplayPort you're referring to happens before this block, at line 426 in nsSubDocumentFrame.cpp, which means that once we get to the IsScrollingActive() call, the "maybe displayport setting" has already been decided. If that call happened in the *middle* of the highlighted block then yes, it would affect my patch because WillUseDisplayPort might return an incorrect value in the IsScrollingActive() call compared to the actual value of usingDisplayPort in BuildDisplayList, and that would cause WillBuildScrollableLayer to also return an incorrect value.

That being said you're right that there isn't a clear semantic meaning for these functions, since I really just extracted them from a subset of the existing code paths (i.e. the top of BuildDisplayList). I'm happy to augment them to include the extra conditions you mentioned.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsSubDocumentFrame.cpp?rev=96d67fd3c056&mark=467-499#467

Comment 8

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

Attachment: Part 2 - Extract a new helper (v2)

Comment 9

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

Attachment: Part 4 - Another trivial refactor

Saw this while poking around in the code.

Comment 10

[Timothy Nikkel (:tnikkel)]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #7)
> The purpose of that function was basically to extract something that would
> correctly predict whether or not the usingDisplayPort variable gets set to
> true inside the BuildDisplayList function. I needed that because the
> mShouldBuildScrollableLayer value is dependent on usingDisplayPort, and so
> in order to implement WillBuildScrollableLayer as a standalone function I
> needed to extract a standalone function for computing usingDisplayPort.

Understood. I was trying to make the code continue to make some sense (given that it doesn't make a lot already) so that it could be refactored into something easier to understand. It's just hard to justify in one's head why WillUseDisplayPort returns false when aBuilder->GetIgnoreScrollFrame() == mOuter, as they don't seem to have anything to do with each other. Even though the new code you are writing just mirrors code we already have.

Comment 11

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

It would be nice to somehow fold the GetOrMaybeCreateDisplayPort calls from nsSubDocumentFrame and PaintFrame into the same block that nsGfxScrollFrame currently has it in. I was looking at doing that but couldn't figure out a clean way to do it exactly because the call sites in nsSubDocumentFrame and PaintFrame use the displayport area and flag for other things. If we can make that work though I feel like it would simplify the code a lot.

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf841511b71a
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e5d1c6a4a0c
https://hg.mozilla.org/integration/mozilla-inbound/rev/8270ee9be504
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f44f9fa13e5

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/cf841511b71a
https://hg.mozilla.org/mozilla-central/rev/2e5d1c6a4a0c
https://hg.mozilla.org/mozilla-central/rev/8270ee9be504
https://hg.mozilla.org/mozilla-central/rev/3f44f9fa13e5

Comment 14

[Timothy Nikkel (:tnikkel)]

Backed out parts 1-3 of this bug in order to land bug 1210578. Bug 1210578 should solve this bug in a better way, and I think kats agrees since he used those patches plus these backouts in his try push in bug 1208780. So this bug should remain closed. Part 4 was untouched.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8842fd664fc6
https://hg.mozilla.org/integration/mozilla-inbound/rev/27ad8f2e14da
https://hg.mozilla.org/integration/mozilla-inbound/rev/f537f6ce1c80

Comment 15

[Timothy Nikkel (:tnikkel)]

And the backouts got merged to m-c
https://hg.mozilla.org/mozilla-central/rev/8842fd664fc6
https://hg.mozilla.org/mozilla-central/rev/27ad8f2e14da
https://hg.mozilla.org/mozilla-central/rev/f537f6ce1c80