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

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.

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.

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

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 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.

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

Saw this while poking around in the code.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.

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.

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

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

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

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