User Agent: Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:48.0) Gecko/20100101 Firefox/48.0 Build ID: 20160602004012 Steps to reproduce: 1. open attached html 2. click button Actual results: you see green rectangle *without* text. this bug is combination of: e10s, css:position:absolute, css:transition. click scrollbar by mouse: now text is shown. Expected results: you should see white text on green background.

Regression window: https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=51dbf899ae40b9fdc9f8e5ba7712c3694656be60&tochange=c5780f2a10e8de3171e12f080d99f62a0c6ca1a5 Regressed by: Bug 1223639

I can repro on OS X as well.

For me, the testcase behaves even more badly: - Wheel scrolling and scrollbar dragging the subframe do not work - Clicking the scrollbar does work, but the text still doesn't show up Switching away to a different tab and back resolve these problems.

Huh! Interestingly, in a local build, I *can* drag the scrollbar and that makes the text show up (and from that point on I can wheel-scroll as well). Still can't wheel-scroll initially, though.

Confirmed that backing out the interesting part of bug 1223639 locally makes the testcase work.

We are storing NaNs in DisplayPortMarginsPropertyData::mMargins for some reason, and that's really messing up the display port calculation. The code prior to bug 1223639 was just probably side-stepping the issue.

There's a "transform: scaleY(0);" in the testcase, which causes nsLayoutUtils::GetTransformToAncestorScale() to return 0. That causes FrameMetrics::mCumulativeResolution to be 0, which then introduces NaNs when we divide by it. Not immediately clear how to handle this.

Here's a frame dump for the test case, for reference.

(In reply to Botond Ballo [:botond] from comment #8) > There's a "transform: scaleY(0);" in the testcase, which causes > nsLayoutUtils::GetTransformToAncestorScale() to return 0. That causes > FrameMetrics::mCumulativeResolution to be 0, which then introduces NaNs when > we divide by it. You mean in GetDisplayPortFromMarginsData? Yeah I see this too. In this case I think it makes sense to just return something sensible, in this case the base rect. I tried doing that, but it doesn't fix the problem (I checked that the base rect is what I would expect). So there must be more to it. The reason we get into this case is that nsDisplayTransform::ShouldPrerenderTransformedContent is true when called in nsIFrame::BuildDisplayListForStackingContext, so we descent into it even though it has 0 size. (I see 0 height even though your frame dump has 1 appunit height.)

(In reply to Timothy Nikkel (:tnikkel) from comment #10) > (In reply to Botond Ballo [:botond] from comment #8) > > There's a "transform: scaleY(0);" in the testcase, which causes > > nsLayoutUtils::GetTransformToAncestorScale() to return 0. That causes > > FrameMetrics::mCumulativeResolution to be 0, which then introduces NaNs when > > we divide by it. > > You mean in GetDisplayPortFromMarginsData? Yeah I see this too. In this case Oh, I see now, when we call nsLayoutUtils::CalculateAndSetDisplayPortMargins.

I think we should try just handling this degenerate case.

(In reply to Timothy Nikkel (:tnikkel) from comment #12) > I think we should try just handling this degenerate case. We can try. What should FrameMetrics::CalculateCompositedSizeInCSSPixels() return if the cumulative resolution is 0 - (0,0,0,0)?

(In reply to Botond Ballo [:botond] from comment #13) > (In reply to Timothy Nikkel (:tnikkel) from comment #12) > > I think we should try just handling this degenerate case. > > We can try. > > What should FrameMetrics::CalculateCompositedSizeInCSSPixels() return if the > cumulative resolution is 0 - (0,0,0,0)? That seems like the only logical thing for it to return in that situation. I confirmed that making that change to CalculateCompositedSizeInCSSPixels and changing GetDisplayPortFromMarginsData to just return the base rect if the resolution is 0 fix the testcase.

Comment on attachment 8760362 [details] Candidate fix >(In reply to Timothy Nikkel (:tnikkel) from comment #14) >> (In reply to Botond Ballo [:botond] from comment #13) >> > What should FrameMetrics::CalculateCompositedSizeInCSSPixels() return if the >> > cumulative resolution is 0 - (0,0,0,0)? >> >> That seems like the only logical thing for it to return in that situation. >> >> I confirmed that making that change to CalculateCompositedSizeInCSSPixels >> and changing GetDisplayPortFromMarginsData to just return the base rect if >> the resolution is 0 fix the testcase. > >I was able to fix the testcase without touching GetDisplayPortFromMarginsData, by just adding a zero check to CalculateCompositedSizeInCSSPixels() and in one other place (AsyncPanZoomController::CalculatePendingDisplayPort()). The latter change prevents us from setting bogus display port margins, and as a result, GetDisplayPortFromMarginsData returns a reasonable result without any changes. > >Timothy, does this approach seem reasonable to you? Isn't |res| 0 here http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=0f0586c0b68d#1000 and then we divide by it here http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp?rev=0f0586c0b68d#1110 I'm pretty sure this is what I observed when debugging this.

(In reply to Timothy Nikkel (:tnikkel) from comment #16) > Isn't |res| 0 here > > http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils. > cpp?rev=0f0586c0b68d#1000 > > and then we divide by it here > > http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils. > cpp?rev=0f0586c0b68d#1110 > > I'm pretty sure this is what I observed when debugging this. I looked into this some more. It turns out that you are right, but that the MoveInsideAndClamp() call corrects the bad displayport in this case. With the previous div-by-zero in CalculateCompositedSizeInCSSPixels(), the displayport value is so messed up (its dimensions are negative) that even MoveInsideAndClamp() doesn't fix it. That said, you're right in that we should avoid the div-by-zero in GetDisplayPortFromMarginsData() as well, rather than relying on code down the line to correct it.

I've been working on an APZ mochitest for this as well.

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

The division-by-zero was introducing NaNs into the displayport calculations, resulting in bad displayports. Review commit: https://reviewboard.mozilla.org/r/58016/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/58016/

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

Comment on attachment 8760447 [details] Bug 1277814 - Avoid division-by-zero when the cumulative resolution is zero. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58016/diff/1-2/

Comment on attachment 8760448 [details] Bug 1277814 - APZ mochitest for scroll frame with transform that results in a cumulative resolution of zero. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58018/diff/1-2/

Comment on attachment 8760447 [details] Bug 1277814 - Avoid division-by-zero when the cumulative resolution is zero. https://reviewboard.mozilla.org/r/58016/#review54920 The early return in GetDisplayPortFromMarginsData skips the ApplyRectMultiplier and MoveInsideAndClamp calls. Seems like we should still be doing those?

Comment on attachment 8760447 [details] Bug 1277814 - Avoid division-by-zero when the cumulative resolution is zero. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58016/diff/2-3/

Comment on attachment 8760448 [details] Bug 1277814 - APZ mochitest for scroll frame with transform that results in a cumulative resolution of zero. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58018/diff/2-3/

(In reply to Timothy Nikkel (:tnikkel) from comment #25) > The early return in GetDisplayPortFromMarginsData skips the > ApplyRectMultiplier and MoveInsideAndClamp calls. Seems like we should still > be doing those? Updated to clamp to the expanded scrollable rect in the early-return case. I don't think we should do the ApplyRectMultiplier, because that has the effect of making the margins larger in the low-res case. Since we're returning the base rect, we're returning a displayport that has no margins, so we might as well keep it that way.

(In reply to Botond Ballo [:botond] from comment #28) > (In reply to Timothy Nikkel (:tnikkel) from comment #25) > > The early return in GetDisplayPortFromMarginsData skips the > > ApplyRectMultiplier and MoveInsideAndClamp calls. Seems like we should still > > be doing those? > > Updated to clamp to the expanded scrollable rect in the early-return case. > > I don't think we should do the ApplyRectMultiplier, because that has the > effect of making the margins larger in the low-res case. Since we're > returning the base rect, we're returning a displayport that has no margins, > so we might as well keep it that way. ApplyRectMultiplier would affect the margins and the baserect. ie if the margins were zero we'd scale the baserect. So shouldn't we do the same if we are assuming the margins are zero?

(In reply to Timothy Nikkel (:tnikkel) from comment #29) > ApplyRectMultiplier would affect the margins and the baserect. ie if the > margins were zero we'd scale the baserect. I actually think that's a bug (one that we could perhaps take the opportunity to fix as a follow-up). I believe that when we set a zero-margin display port, such as here [1], the intention is that we only paint the viewport and not anything beyond it (for performance reasons). ni?kats to confirm what the intention here was. [1] https://dxr.mozilla.org/mozilla-central/rev/3e8ee3599a67edd971770af4982ad4b0fe77f073/layout/base/nsLayoutUtils.cpp#3277

Comment on attachment 8760448 [details] Bug 1277814 - APZ mochitest for scroll frame with transform that results in a cumulative resolution of zero. The test is failing intermittently on Try. That will need to be investigated before it lands.

I thought the multiplier was to compensate for the resolution we draw at.

(In reply to Timothy Nikkel (:tnikkel) from comment #32) > I thought the multiplier was to compensate for the resolution we draw at. It compensates for the resolution we draw at in the sense that we draw an area proportionally larger relative to the screen, on the basis that drawing an area X times larger at X times lower resolution should take around the same amount of time as drawing the smaller area at the higher resolution. But we're still drawing a larger area of the page, and so building display lists for more elements and so on. This is what makes me think that, if we're explicitly asking for zero margins for performance reasons, that that should apply even to the low-res case.

Comment on attachment 8760447 [details] Bug 1277814 - Avoid division-by-zero when the cumulative resolution is zero. https://reviewboard.mozilla.org/r/58016/#review54998 Okay, whatever we decide about the low res multiplier, it might be better to continue doing the same thing in this special case as we do in the general case in this bug. And then change the behavior of both in the followup. So that both bugs fix just one problem. But you have r+ so you can deal with it how you wish.

(In reply to Botond Ballo [:botond] from comment #30) > (In reply to Timothy Nikkel (:tnikkel) from comment #29) > > ApplyRectMultiplier would affect the margins and the baserect. ie if the > > margins were zero we'd scale the baserect. > > I actually think that's a bug (one that we could perhaps take the > opportunity to fix as a follow-up). I believe that when we set a zero-margin > display port, such as here [1], the intention is that we only paint the > viewport and not anything beyond it (for performance reasons). > > ni?kats to confirm what the intention here was. Yeah, the intention with a zero-margin displayport is to just paint the visible area, in high-res. Nothing additional should get painted in low-res. So it makes sense to return exactly the base rect for both high-res and low-res displayports when we have zero margins. If my memory of how the base rect is computed is correct, even the MoveInsideAndClamp should be a no-op for it.

Comment on attachment 8760446 [details] Bug 1277814 - Add printing support to BaseMargin. https://reviewboard.mozilla.org/r/58014/#review55072

Comment on attachment 8760448 [details] Bug 1277814 - APZ mochitest for scroll frame with transform that results in a cumulative resolution of zero. https://reviewboard.mozilla.org/r/58018/#review55074 ::: gfx/layers/apz/test/mochitest/test_bug1277814.html:8 (Diff revision 3) > +<!-- > +https://bugzilla.mozilla.org/show_bug.cgi?id=1277814 > +--> > +<head> > + <meta charset="utf-8"> > + <title>Test for Bug 1151663</title> Copypasta bug number ::: gfx/layers/apz/test/mochitest/test_bug1277814.html:33 (Diff revision 3) > + var foundIt = false; > + for (var seqNo in contentTestData.paints) { > + var paint = contentTestData.paints[seqNo]; > + for (var scrollId in paint) { > + var scrollFrame = paint[scrollId]; > + for (var key in scrollFrame) { > + if (key == 'contentDescription') { > + if (scrollFrame[key].includes('bug1277814')) { > + foundIt = true; > + } > + } > + } > + } > + } I think this can be rewritten as for (var paint of contentTestData.paints) { for (var scrollFrame of paint) { if ('contentDescription' in scrollFrame && scrollFrame['contentDescription'].includes('bug1277814')) { foundIt = true; } } } If you want to get really fancy you can also use the map/reduce functions on Array ::: gfx/layers/apz/test/mochitest/test_bug1277814.html:40 (Diff revision 3) > + var paint = contentTestData.paints[seqNo]; > + for (var scrollId in paint) { > + var scrollFrame = paint[scrollId]; > + for (var key in scrollFrame) { > + if (key == 'contentDescription') { > + if (scrollFrame[key].includes('bug1277814')) { I'd add the -div suffix on the search string here.

(In reply to Botond Ballo [:botond] from comment #31) > Comment on attachment 8760448 [details] > Bug 1277814 - APZ mochitest for scroll frame with transform that results in > a cumulative resolution of zero. > > The test is failing intermittently on Try. That will need to be investigated > before it lands. Here's a Try push with an extra waitForAllPaints() call added. That reduced the intermittent failure rate significantly, but there stil are a couple unfortunately: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f0eb76ecadb5

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #37) > ::: gfx/layers/apz/test/mochitest/test_bug1277814.html:33 > (Diff revision 3) > > + var foundIt = false; > > + for (var seqNo in contentTestData.paints) { > > + var paint = contentTestData.paints[seqNo]; > > + for (var scrollId in paint) { > > + var scrollFrame = paint[scrollId]; > > + for (var key in scrollFrame) { > > + if (key == 'contentDescription') { > > + if (scrollFrame[key].includes('bug1277814')) { > > + foundIt = true; > > + } > > + } > > + } > > + } > > + } > > I think this can be rewritten as > > for (var paint of contentTestData.paints) { > for (var scrollFrame of paint) { > if ('contentDescription' in scrollFrame && > scrollFrame['contentDescription'].includes('bug1277814')) { > foundIt = true; > } > } > } This is what I get when I try that: TypeError: contentTestData.paints is not iterable at test@http://mochi.test:8888/tests/gfx/layers/apz/test/mochitest/test_bug1277814.html:32:16

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

Comment on attachment 8760448 [details] Bug 1277814 - APZ mochitest for scroll frame with transform that results in a cumulative resolution of zero. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58018/diff/3-4/

Comment on attachment 8761439 [details] Bug 1277814 - For a zero-margin displayport, do not expand the displayport beyond the scroll port even during low-res painting. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58638/diff/1-2/

Comment on attachment 8760448 [details] Bug 1277814 - APZ mochitest for scroll frame with transform that results in a cumulative resolution of zero. Review request updated; see interdiff: https://reviewboard.mozilla.org/r/58018/diff/4-5/

(In reply to Timothy Nikkel (:tnikkel) from comment #34) > Okay, whatever we decide about the low res multiplier, it might be better to > continue doing the same thing in this special case as we do in the general > case in this bug. And then change the behavior of both in the followup. So > that both bugs fix just one problem. But you have r+ so you can deal with it > how you wish. I wrote a new patch to make the general case do the intended thing.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #37) > Copypasta bug number Fixed. > I think this can be rewritten as > > for (var paint of contentTestData.paints) { > for (var scrollFrame of paint) { > if ('contentDescription' in scrollFrame && > scrollFrame['contentDescription'].includes('bug1277814')) { > foundIt = true; > } > } > } I was able to use the "if ('contentDescription' in scrollFrame ...)" bit, but not the "var ... of ..." bits, as described in comment 39. > If you want to get really fancy you can also use the map/reduce functions on > Array Going to pass for now, but I'll keep it in mind :) > I'd add the -div suffix on the search string here. Thanks, I meant to do that! Fixed. The low-volume intermittent mentioned in comment 31 still needs to be investigated.

Comment on attachment 8761439 [details] Bug 1277814 - For a zero-margin displayport, do not expand the displayport beyond the scroll port even during low-res painting. https://reviewboard.mozilla.org/r/58638/#review55574

Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/08fc0d77146b Add printing support to BaseMargin. r=kats https://hg.mozilla.org/integration/mozilla-inbound/rev/d535bfed130d Avoid division-by-zero when the cumulative resolution is zero. r=tnikkel https://hg.mozilla.org/integration/mozilla-inbound/rev/213bc3441c38 For a zero-margin displayport, do not expand the displayport beyond the scroll port even during low-res painting. r=tnikkel

Landed the fixes. Going to wait with landing the test until the low-volume intermittent is figured out; marking leave-open.

https://hg.mozilla.org/mozilla-central/rev/08fc0d77146b https://hg.mozilla.org/mozilla-central/rev/d535bfed130d https://hg.mozilla.org/mozilla-central/rev/213bc3441c38

Testing a theory as to what might be the cause of the intermittent failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=686943da5c5f

(In reply to Botond Ballo [:botond] from comment #50) > Testing a theory as to what might be the cause of the intermittent failures: > > https://treeherder.mozilla.org/#/jobs?repo=try&revision=686943da5c5f Looks like I was right - the intermittent appears to be gone with this fix.

(The change I made was to scroll the wheel over the subframe to guarantee its layerization, instead of relying on the addition of the CSS class to do it.)

Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/488f4386606b APZ mochitest for scroll frame with transform that results in a cumulative resolution of zero. r=kats

^ Landed the test with the added fix.

Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/4a6d7d2c1e65 Disable test_bug1277814.html on Android because it uses wheel events which are not supported on Android. r=bustage

^ Follow-up to disable the test on Android because wheel events are not supported on Android.

https://hg.mozilla.org/mozilla-central/rev/488f4386606b https://hg.mozilla.org/mozilla-central/rev/4a6d7d2c1e65

Forgot to remove leave-open when landing the test.

Please request uplift if you think this is safe enough. If not we can mark this wontfix for status-firefox{48,49}.

Definitely ok for aurora. For beta, I'd like to understand how big of an issue this is. CoolCmd, have you run into this pattern (transform:scale(0)) on a production website? How common do you expect it to be?

Comment on attachment 8760447 [details] Bug 1277814 - Avoid division-by-zero when the cumulative resolution is zero. Approval Request Comment [Feature/regressing bug #]: APZ [User impact if declined]: In code that uses certain CSS patterns (transforms with a scale of 0), correct drawing of the page may be delayed until an event forces a repaint. [Describe test coverage new/current, TreeHerder]: The patch adds a mochitest which covers the affected scenario. [Risks and why]: Low but nonzero. [String/UUID change made/needed]: None. Requesting uplift to aurora. I may add a beta request pending comment 60. I don't think it's necessary to uplift the other three patches, this is the important one.

(In reply to Botond Ballo [:botond] (C++ standards meeting Jun 20-25) from comment #60) > CoolCmd, have you run into this pattern (transform:scale(0)) on a production website? yes > How common do you expect it to be? i don't know...

in my case, is transform:scaleY(.1) enough to temporary fix bug?

(In reply to CoolCmd from comment #63) > in my case, is transform:scaleY(.1) enough to temporary fix bug? Yes, 0 is the only problematic value.

Comment on attachment 8760447 [details] Bug 1277814 - Avoid division-by-zero when the cumulative resolution is zero. Based on comment 62, let's uplift to beta as well.

Comment on attachment 8760447 [details] Bug 1277814 - Avoid division-by-zero when the cumulative resolution is zero. Fix an e10s regression, taking it; Should be in 48 beta 4 Sheriff, this is ONLY for this patch.

I reproduced this bug using Fx 48.0a2 build ID:20160602004012, on Windows 10 x64. Confirmed the fix on Fx 48.0b4, build ID 20160627144420, on Windows 10 x64.

I reproduced this issue using Fx 49.0a1, build ID: 20160603030242, on Windows 10 x64. I re-tested on Windows 10x64, Ubuntu 14.04 LTS, Mac OS X 10.11, using : - Fx 48.0.1 (20160817112116) - Fx 49.0 (20160912134115) - Fx 50.0a2 (20160912004004) - Fx 51.0a1 (20160912030421) I can confirm the issue is fixed. Cheers!