STR: 1. Load http://www.bbc.co.uk/news/resources/idt-sh/mod_uk 2. Scroll down the page to the photo gallery with guy on a Vespa scooter. 3. Scroll down more to advance the photos in the photo gallery. RESULT: Scrolling the photos leaves large black gaps between the photos. There are lots of large black gaps on this page when scrolling, but I bisected the photo gaps to bug 1325550 in Firefox 53, which as a backout of clip-path bug 1318266. This bug looks a lot like bug 1303761, another BBC scrolling bug. I can reproduce the black gaps in Nightly 53-56, but *not* in the Beta or Release channels. There seems to be something enabled or disabled only in the Nightly channel that triggers this bug. I am running macOS 10.12.5.

I can reproduce this on the latest Beta. I see a lot of black lines as I scroll and it is easy to stop in a position where the photo is half of the Vespa and half the next photo. Running Windows 10 Creators update.

(In reply to David Walker from comment #1) > I can reproduce this on the latest Beta. I see a lot of black lines as I > scroll and it is easy to stop in a position where the photo is half of the > Vespa and half the next photo. The half and half photos are the expected transition between photos. The bug I see is that there are large black gaps where the photo halves meet. I can reproduce the black lines on Windows 10 with Nightly 56 but not Beta 55.

On Windows 10, I see problems on the release 54 as well, similar but not quite the same problems in Edge, and Chrome is the only one that works. I'm guessing they're doing something magical that is Chrome implementation specific.

CJ, can you take a look given comment 0?

Scrollbar scrolling fails with APZ on and succeeds with APZ off. Perhaps one of those "APZ and JS scrolling is hard", but I'll let the experts decide.

I am testing with a MacBook Pro and Windows 10 laptop. I can reproduce the black gaps when scrolling using the trackpad gesture or when clicking and dragging the scrollbar, but *not* when scrolling with the arrow keys. I haven't tested with a mouse.

(In reply to Chris Peterson [:cpeterson] from comment #6) > I am testing with a MacBook Pro and Windows 10 laptop. I can reproduce the > black gaps when scrolling using the trackpad gesture or when clicking and > dragging the scrollbar, but *not* when scrolling with the arrow keys. I > haven't tested with a mouse. That's because this bug is relative to APZ.

Please see bug 1350663 comment 18. Put a position:fixed element inside mask layer work not correcly with APZ. So you won't see this problem by turning off E10S. Best solution is to fix it right inside APZ module. Alernative, we may find workaround at positioned mask side by not using mask layer if we find position:fixed descenants inside the masked element. Drawback of this approach is performance will become worse. I think this one should be relative to bug 1300864.

BTW, on the photo gallery with guy on a Vespa scooter: 1. there is both clip-path:basic-shape + positioned mask applied onto that frame. 2. position:fixed is involved as well. So... you see that black glitch...

There is a test case in Bug 1300864 STR: 1. Go to http://codepen.io/Matori/full/BoaZeV/ 2. Scroll the pink slide away by scrolling down a little. 3. Slowly scroll down some more and watch as the blue curtain image moves with the slide instead of staying fixed in place. 4. Observe the same thing for the other slides. 5. Scroll back up and notice jiggly images and white parts where there shouldn't be any white. You will see white glitch. The same, this problem disappear by turning of e10s

(In reply to Milan Sreckovic [:milan] from comment #5) > Scrollbar scrolling fails with APZ on and succeeds with APZ off. Perhaps > one of those "APZ and JS scrolling is hard", but I'll let the experts decide. The bug affects APZ scrolling on this page by any input method. Based on the symptoms, I'd say this is a scroll-linked effect that doesn't play well with APZ (hence the black line showing up at all), combined with over-aggressive paint skipping (hence the black line sticking around until you click or otherwise force a repaint). We can probably fix the second, and bug 1375949 should help with the first if we can paint the page fast enough.

(In reply to Botond Ballo [:botond] from comment #11) > Based on the symptoms, I'd say this is a scroll-linked effect that doesn't > play well with APZ (hence the black line showing up at all), combined with > over-aggressive paint skipping (hence the black line sticking around until > you click or otherwise force a repaint). I investigated this a bit more, and discussed is with Markus. The issue is not due to a scroll-linked effect, it's due to mask layers not having correct scroll metadata. This should be fixable relatively easily for at least a subset of mask layers (hopefully including this one).

(In reply to Botond Ballo [:botond] from comment #12) > I investigated this a bit more, and discussed is with Markus. The issue is > not due to a scroll-linked effect, it's due to mask layers not having > correct scroll metadata. This should be fixable relatively easily for at > least a subset of mask layers (hopefully including this one). Just to provide an update here: I've been continuing to look at this, and discussing it with Markus and C.J., but I haven't been able to come up with a fix that works for this page yet. It may be more involved than I initially thought. I'll post back once I have a more concrete update.

This is not a fix yet, but it appears to be a prerequisite for further work. Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5df5a3e0d177834740f3e4d15189daa9fd0a0dd5

Comment on attachment 8895583 [details] Bug 1382534 - Layerize active masks even with HWA disabled. https://reviewboard.mozilla.org/r/166782/#review172022 ::: layout/painting/nsDisplayList.cpp:8811 (Diff revision 1) > const ContainerLayerParameters& aParameters) > { > - if (ShouldPaintOnMaskLayer(aManager)) { > - return RequiredLayerStateForChildren(aBuilder, aManager, aParameters, > - mList, GetAnimatedGeometryRoot()); > + if (CanPaintOnMaskLayer(aManager)) { > + LayerState result = RequiredLayerStateForChildren(aBuilder, aManager, > + aParameters, mList, GetAnimatedGeometryRoot()); > + return result == LAYER_INACTIVE ? LAYER_SVG_EFFECTS : result; Maybe add a comment here: "When we're not active, FrameLayerBuilder will call PaintAsLayer on us during painting. In that case we don't want a mask layer to be created, because PaintAsLayer takes care of applying the mask. So we return LAYER_SVG_EFFECTS instead of LAYER_INACTIVE so that FrameLayerBuilder doesn't set a mask layer on our layer."

For reference, here is my latest attempt at the actual fix: https://treeherder.mozilla.org/#/jobs?repo=try&revision=f50efbc8f6f07d50e18fa356578a11115d294b37 It's failing a lot of reftests, which I need to investigate.

This is marked as "unaffected" for 54 and 55, but I can reproduce the issue with both of those versions. Tested on Linux in the default configuration. Updating flags accordingly.

(In reply to Botond Ballo [:botond] from comment #21) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=df6da4b997eebef6ecb9fb23a11d3cfeea2ba76c Looks like we're down to just two failures: layout/reftests/svg/svg-integration/mask-html-zoomed-01.xhtml layout/reftests/svg/mask-basic-05.svg

Down to one failure: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d667628f25ff5048762cd056b748a1620e54317d (Ignore the unrelated infra bustage.)

I posted my WIP patch series for this bug. The current status is that it passes reftests [1], but does not achieve the desired async-scrolling behaviour on testcases that use clip-path, including the reduced testcase from bug 1214146 [2], and the original BBC testcase (it also triggers a "Need to be clipped wrt aASR" assertion on that testcase). [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2fc7190883a1065ae9190220b62a5565e79ec7b [2] https://bug1214146.bmoattachments.org/attachment.cgi?id=8673016

I'm going to summarize my current understanding, and where I'm currently stuck. To achieve the desired async-scrolling behaviour, we need nsDisplayMask display items to have scroll metadata for the scroll frame they're supposed to be scrolled by. To give them this scroll metadata, we need to give them the ASR for the scroll frame in question. To give a display item an ASR, it needs to have a clip with respect to that ASR. The main difficulty in this bug is computing the correct clip. My patch tries to follow the logic used in ComputeMaskGeometry() [1] to compute the clip in question. ComputeMaskGeometry() is called at painting time, and takes some inputs that are only available at painting time (like a gfxContext), whereas I need to compute my clip at display list building time. However, the inputs to ComputeMaskGeometry() that are actually used in the computation of the clip are all available at display list building time, so this is fine. The problem seems to be that nsDisplayMask is used to represent both masks and clip paths. ComputeMaskGeometry() is called for both, but it only computes a clip for masks (I believe that's because for clip paths, there will be no mask frames, and this early exit [2] will be taken). As a result, my patch also only computes a clip for masks. If there is no clip, it falls back to the previous behaviour of giving the nsDisplayMask the "container item ASR", which will not make it async-scroll. So it seems that what I need to do is also compute a clip for nsDisplayMasks that represent clip paths. Markus, do you know if there is some existing code (similar to ComputeMaskGeometry() for masks) that currently computes such a clip, that I can base my computation on? [1] http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/layout/painting/nsDisplayList.cpp#8690 [2] http://searchfox.org/mozilla-central/rev/cd82cacec2cf734768827ff85ba2dba90a534c5e/layout/painting/nsDisplayList.cpp#8704

(In reply to Botond Ballo [:botond] from comment #30) > So it seems that what I need to do is also compute a clip for nsDisplayMasks > that represent clip paths. Markus, do you know if there is some existing > code (similar to ComputeMaskGeometry() for masks) that currently computes > such a clip, that I can base my computation on? Maybe CJ has some ideas for how do this.

for how *to* do this.

There are two kinds of clip-path[1] 1. basic-shape and geometry-box: you may refer to [2] to get the clip bounds of a basic-shape clip-path. 2. url(): I think nsSVGUtils::GetBBox(clippedTargetFrame, nsSVGUtils::eBBoxIncludeClipped) should work, but I never try. [1] https://developer.mozilla.org/zh-TW/docs/Web/CSS/clip-path [2] https://hg.mozilla.org/mozilla-central/file/64d939893426/layout/svg/nsCSSClipPathInstance.cpp#l75

And, by using nsSVGUtils::DetermineMaskUsage, you can tell which kind of clip-path applied on the target frame.

I updated the patch to compute a clip rect for clip-path items. Unsurprisingly, that causes a bunch of reftest failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=edaab9c87f3832bbb6f2370fffbb28bc40ff20fd

Further updates: I made some progress by fixing the failures in Wr6: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e76f2fc5198a92346d7a16aa8228e6572037b1ae Then I tried to fix layout/reftests/w3c-css/submitted/masking/clip-path-contentBox-1b.html: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7f153a51fd4ec19ac5d2df801ab7dce7ef5b24c0 ... but that regressed a bunch of other things. Meanwhile, I tried to fix layout/reftests/box-shadow/boxshadow-inner-basic.html: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8e86e068f8f5d97b5f55f4020c1a3a4a9d0e6cf8 ... but that regressed even more things. All in all, I'm not sure that I've made much in the way of net progress, or that I'm even making changes in the right direction...

This revised attempt is looking better: https://treeherder.mozilla.org/#/jobs?repo=try&revision=3ecb8d129b90ed13c87189217c327d295833dfa6 R7 seems to be the only failing chunk now.

Now with fewer R7 failures: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b6f41eef89f9dc5de8c0784ca5888a5b6cde8366

(In reply to C.J. Ku[:cjku](UTC+8) from comment #33) > There are two kinds of clip-path[1] > 1. basic-shape and geometry-box: you may refer to [2] to get the clip bounds > of a basic-shape clip-path. > > [...] > > [2] > https://hg.mozilla.org/mozilla-central/file/64d939893426/layout/svg/ > nsCSSClipPathInstance.cpp#l75 So the linked code does two things: 1) Computes the reference box for the clip path 2) Draws the clip path onto a DrawTarget, relative to the reference box, and then derives a Path from it. My current code just uses the reference box as the clip rect. However, that's not correct, because the clip path doesn't have to be inside the reference box. For example, in this test [1], it's not. It looks like I need to do an extra step to get a bounding rect for the clip path itself. Do you have a suggestion for how to do that? I assume I should not be trying to create a DrawTarget at display list building time just so that I can get the bounds of the path. [1] https://hg.mozilla.org/mozilla-central/file/tip/layout/reftests/svg/svg-integration/clip-path/clip-path-circle-008.html

Besides the remaining reftest failures, another issue with these patches is that they trigger this ASR-related assertion [1] on the original BBC testcase. I reduced this to the attached minimal testcase. [1] http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/layout/painting/nsDisplayList.cpp#1918

This is a display list dump for the reduced testcase. The assertion occurs for the Mask item (0x7f8a816bbda0) and the null ASR.

(In reply to Botond Ballo [:botond] from comment #39) > It looks like I need to do an extra step to get a bounding rect for the clip > path itself. Do you have a suggestion for how to do that? I assume I should > not be trying to create a DrawTarget at display list building time just so > that I can get the bounds of the path. Markus suggested using ScreenReferenceDrawTarget(), similar to how SVGGeometryFrame::GetBBoxContribution() does it [1]. CJ, if you agree with this approach, feel free to just clear the needinfo. [1] http://searchfox.org/mozilla-central/rev/6326724982c66aaeaf70bb7c7ee170f7a38ca226/layout/svg/SVGGeometryFrame.cpp#552

Now with even fewer R7 failures, also I've regressed one test in Wr1: https://treeherder.mozilla.org/#/jobs?repo=try&revision=eb839037d89ea2e26db14aef16f36b3a38371252 (Ignore the Werror bustage.)

Down to two failures: layout/reftests/svg/clipPath-polygon-elementFromPoint-01.svg layout/reftests/svg/clipPath-winding-01.svg https://treeherder.mozilla.org/#/jobs?repo=try&revision=f826b6d6e113a1e34cc8251aade0187ff79351b2

I'm thinking this should wait for > 57

Agreed. This is the sort of change I'd like to bake on Nightly for a bit rather than uplifting.

Looks like we're passing all reftests now! https://treeherder.mozilla.org/#/jobs?repo=try&revision=1d2e94de889ca571ba5210461eb5435295c96334 The only thing remaining to be fixed before these patches are ready for review is the ASR-related assertion failure (for which there is a reduced testcase in comment 40).

Comment on attachment 8902348 [details] Bug 1382534 - Move ComputeOffsetToUserSpace() from nsDisplayList.cpp to nsLayoutUtils. https://reviewboard.mozilla.org/r/173900/#review193214 ::: layout/base/nsLayoutUtils.cpp:9777 (Diff revision 2) > + // After applying only "offsetToBoundingBox", aParams.ctx would have its > + // origin at the top left corner of frame's bounding box (over all > + // continuations). > + // However, SVG painting needs the origin to be located at the origin of the > + // SVG frame's "user space", i.e. the space in which, for example, the > + // frame's BBox lives. > + // SVG geometry frames and foreignObject frames apply their own offsets, so > + // their position is relative to their user space. So for these frame types, > + // if we want aCtx to be in user space, we first need to subtract the > + // frame's position so that SVG painting can later add it again and the > + // frame is painted in the right place. This comment needs to be rephrased slightly because it refers to arguments which no longer exist, aParams.ctx and aCtx. (aCtx was already gone before your changes, but at least you knew that aParams.ctx was probably what was meant.)

Comment on attachment 8902349 [details] Bug 1382534 - Add a UnionMaybeRects() method to gfx/2d/Rect.h. https://reviewboard.mozilla.org/r/173902/#review193252

Comment on attachment 8915335 [details] Bug 1382534 - Account for the local transform in nsSVGClipPathFrame::GetBBoxForClipPathFrame(). https://reviewboard.mozilla.org/r/186524/#review193262 Do you know if this bug had any other effects? It would be nice to add a test for it that doesn't depend on the other patches in this bug.

Comment on attachment 8915336 [details] Bug 1382534 - Make layout/reftests/svg/clipPath-polygon-elementFromPoint-01.svg a bit more permissive. https://reviewboard.mozilla.org/r/186526/#review193268

Comment on attachment 8915339 [details] Bug 1382534 - Relax the requirement of having a clip with respect to an ASR for nsDisplayMask items. https://reviewboard.mozilla.org/r/186532/#review193272 ::: layout/painting/nsDisplayList.cpp:1953 (Diff revision 1) > nsRect r = i->GetClippedBounds(aBuilder); > if (aASR != i->GetActiveScrolledRoot() && !r.IsEmpty()) { > - const DisplayItemClip* clip = DisplayItemClipChain::ClipForASR(i->GetClipChain(), aASR); > -#ifdef DEBUG > + if (Maybe<nsRect> clip = i->GetClippedBoundsWithRespectToASR(aBuilder, aASR)) { > + r = clip.ref(); The method name "GetClippedBoundsWithRespectToASR" makes it sound like it will return the clipped bounds, and not just a clip. So you could either adjust the method name, or just make it do what it says, by moving the lines nsRect r = i->GetClippedBounds(aBuilder); if (aASR != i->GetActiveScrolledRoot() && !r.IsEmpty()) into that method, too. Then it can return an nsRect and doesn't need to return a Maybe. ::: layout/painting/nsDisplayList.cpp:9186 (Diff revision 1) > + // The item does not have a clip with respect to |aASR|. However, it > + // might still have finite bounds with respect to |aASR|. Check its > + // children. I'd rephrase to: "This item" and "Check our children"

Comment on attachment 8915340 [details] Bug 1382534 - Add nsCSSClipPathInstance::GetBoundingRectForBasicShapeClip(). https://reviewboard.mozilla.org/r/186534/#review193274

Comment on attachment 8902350 [details] Bug 1382534 - Try to give nsDisplayMask items proper scroll metadata. https://reviewboard.mozilla.org/r/173904/#review193278 ::: layout/generic/nsFrame.cpp:2405 (Diff revision 2) > + nsSVGUtils::eBBoxIncludeClipped > + | nsSVGUtils::eBBoxIncludeFill > + | nsSVGUtils::eBBoxIncludeMarkers); Operator goes at the end of the previous line. ::: layout/generic/nsFrame.cpp:2468 (Diff revision 2) > + // Convert to user space. > + *combinedClip += devPixelOffsetToUserSpace; > + > + // Round the clip out. In FrameLayerBuilder we round clips to nearest > + // pixels, and if we have a really thin clip here, that can cause the > + // clip to become empty if we didn't round out here. Add a comment that says that this rounding happens on coordinates that are relative to the reference frame, which matches what FrameLayerBuilder does. ::: layout/generic/nsFrame.cpp:2476 (Diff revision 2) > + // Convert to app units. > + nsRect result = nsLayoutUtils::RoundGfxRectToAppRect(*combinedClip, devPixelRatio); > + > + // The resulting clip is relative to the reference frame, but the caller > + // expects it to be relative to the masked frame, so adjust it. > + result -= aBuilder->ToReferenceFrame(aMaskedFrame); You already have the offset to the reference frame, in a variable called toReferenceFrame. ::: layout/generic/nsFrame.cpp:2836 (Diff revision 2) > + const ActiveScrolledRoot* maskASR = clipForMask.isSome() > + ? aBuilder->CurrentActiveScrolledRoot() > + : containerItemASR; This also needs a comment. For example: The mask should move with aBuilder->CurrentActiveScrolledRoot(), so that's the ASR we want to use for the mask item. However, we can only do this if the mask is clipped with respect to that ASR, because an item always needs to have finite bounds with respect to its ASR. So we can only use aBuilder->CurrentActiveScrolledRoot() as the mask's ASR if we were able to compute a clip for the mask. In the case that we weren't, use lowest common ancestor clip of the mask's contents (the containerItemASR) instead. That's not entirely correct but it satisfies the base requirement of the ASR system ("always have finite bounds wrt your ASR").

Comment on attachment 8915341 [details] Bug 1382534 - Add a crashtest for the ASR-related assertion failure. https://reviewboard.mozilla.org/r/186536/#review193286

Comment on attachment 8915342 [details] Bug 1382534 - Add an async-scrolling reftest. https://reviewboard.mozilla.org/r/186538/#review193288

Comment on attachment 8915335 [details] Bug 1382534 - Account for the local transform in nsSVGClipPathFrame::GetBBoxForClipPathFrame(). https://reviewboard.mozilla.org/r/186524/#review193298 ::: layout/svg/nsSVGClipPathFrame.cpp:483 (Diff revision 1) > { > nsIContent* node = GetContent()->GetFirstChild(); > SVGBBox unionBBox, tmpBBox; > for (; node; node = node->GetNextSibling()) { > - nsIFrame *frame = > - static_cast<nsSVGElement*>(node)->GetPrimaryFrame(); > + nsSVGElement* svgNode = static_cast<nsSVGElement*>(node); > + nsIFrame *frame = svgNode->GetPrimaryFrame(); given that you're changing this, make the * hug nsIFrame

Comment on attachment 8915337 [details] Bug 1382534 - Use clip-rule instead of fill-rule when calculating the bounding box contribution of an SVG geometry frame inside a clip-path. https://reviewboard.mozilla.org/r/186528/#review193300 ::: layout/svg/SVGGeometryFrame.cpp:545 (Diff revision 1) > CreateDrawTargetForSurface(refSurf, IntSize(1, 1)); > #else > tmpDT = gfxPlatform::GetPlatform()->ScreenReferenceDrawTarget(); > #endif > > - FillRule fillRule = nsSVGUtils::ToFillRule(StyleSVG()->mFillRule); > + FillRule fillRule = nsSVGUtils::ToFillRule( There's a frame state bit for this: GetStateBits() & NS_STATE_SVG_CLIPPATH_CHILD)

Comment on attachment 8915338 [details] Bug 1382534 - Only reuse the cached path in SVGGeometryElement::GetOrBuildPath() if it has the fill rule we want. https://reviewboard.mozilla.org/r/186530/#review193302 ::: dom/svg/SVGGeometryElement.cpp:105 (Diff revision 1) > > // Checking for and returning mCachedPath before checking the pref means > // that the pref is only live on page reload (or app restart for SVG in > // chrome). The benefit is that we avoid causing a CPU memory cache miss by > // looking at the global variable that the pref's stored in. > - if (cacheable && mCachedPath) { > + if (cacheable && mCachedPath && mCachedPath->GetFillRule() == aFillRule) { should really just combine these if tests into one.

(In reply to Markus Stange [:mstange] from comment #67) > The method name "GetClippedBoundsWithRespectToASR" makes it sound like it > will return the clipped bounds, and not just a clip. So you could either > adjust the method name, or just make it do what it says, by moving the lines > nsRect r = i->GetClippedBounds(aBuilder); > if (aASR != i->GetActiveScrolledRoot() && !r.IsEmpty()) > into that method, too. Then it can return an nsRect and doesn't need to > return a Maybe. I'm going to go with the rename, because the proposed alternative doesn't fit the call site in ContainerState::ProcessDisplayItems().

Robert, you may want to consider putting "[:longsonr]" in your Bugzilla nick. Without that, MozReview doesn't recognize "r=longsonr" and does silly things like what it just did now (re-set the review flags to :mstange).

Comment on attachment 8915335 [details] Bug 1382534 - Account for the local transform in nsSVGClipPathFrame::GetBBoxForClipPathFrame(). Carrying r=longsonr.

Comment on attachment 8915338 [details] Bug 1382534 - Only reuse the cached path in SVGGeometryElement::GetOrBuildPath() if it has the fill rule we want. Carrying r=longsonr.

All-platform Try push to make sure we don't need to adjust fuzzing and such: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d962b99fca6ea5b53338d49377ff2519936f6045

(In reply to Botond Ballo [:botond] from comment #91) > https://treeherder.mozilla.org/#/ > jobs?repo=try&revision=d962b99fca6ea5b53338d49377ff2519936f6045 There are a couple of failures that were not caught by the previous Linux-only Try pushes: 1) The crashtest that was added is failing with WebRender enabled 2) layout/style/test/test_clip-path_polygon.html is failing on Android The stack trace for (1) is: ASSERTION: bad aListVisibleBounds: 'r.GetBounds().IsEqualInterior(aListVisibleBounds)', file /builds/worker/workspace/build/src/layout/painting/nsDisplayList.cpp, line 2032 #01: nsDisplayMask::ComputeVisibility [layout/painting/nsDisplayList.cpp:9059] #02: nsDisplayItem::RecomputeVisibility [layout/painting/nsDisplayList.cpp:2733] #03: mozilla::layers::WebRenderCommandBuilder::GenerateFallbackData [mfbt/RefPtr.h:287] #04: mozilla::layers::WebRenderCommandBuilder::BuildWrMaskImage [mfbt/AlreadyAddRefed.h:121] #05: nsDisplayMask::CreateWebRenderCommands [layout/painting/nsDisplayList.cpp:9150] #06: mozilla::layers::WebRenderCommandBuilder::CreateWebRenderCommandsFromDisplayList [gfx/layers/wr/WebRenderCommandBuilder.cpp:207] #07: mozilla::layers::WebRenderCommandBuilder::BuildWebRenderCommands [gfx/layers/wr/WebRenderCommandBuilder.cpp:50] #08: mozilla::layers::WebRenderLayerManager::EndTransactionWithoutLayer [gfx/layers/wr/WebRenderLayerManager.cpp:259] #09: nsDisplayList::PaintRoot [layout/painting/nsDisplayList.cpp:2168]

Comment on attachment 8915337 [details] Bug 1382534 - Use clip-rule instead of fill-rule when calculating the bounding box contribution of an SVG geometry frame inside a clip-path. https://reviewboard.mozilla.org/r/186528/#review193460

(In reply to Botond Ballo [:botond] from comment #92) > The stack trace for (1) is: > > ASSERTION: bad aListVisibleBounds: > 'r.GetBounds().IsEqualInterior(aListVisibleBounds)', file > /builds/worker/workspace/build/src/layout/painting/nsDisplayList.cpp, line > 2032 Huh, interesting. I was seeing this assertion on a bunch of webrender tests and recently landed a patch in bug 1403920 to fix it. However it looks like you have that patch already so I'm not sure why you're seeing this. Matt might be able to help.

(In reply to Botond Ballo [:botond] from comment #92) > The stack trace for (1) is: > > ASSERTION: bad aListVisibleBounds: > 'r.GetBounds().IsEqualInterior(aListVisibleBounds)', file > /builds/worker/workspace/build/src/layout/painting/nsDisplayList.cpp, line > 2032 This appears to be related to nsDisplayList::GetClippedBoundsWithRespectToASR(). When processing the child list inside an nsDisplayMask, for one of the items we enter this branch [1] where previously we didn't (because the nsDisplayMask had a different ASR that I'm guessing was the same as the item's ASR). Notice how in that branch, we _overwrite_ the GetClippedBounds() with the value of the clip rect, rather than intersecting it. In this case, we end up making it _larger_, whereas the previous value (derived from GetClippedBounds() only) would have made the assertion in ComputeVisibilityForSublist() happy. Markus, do you know why we're not intersecting there? Is there an implicit requirement that the clip-with-respect-to-ASR be no larger than the GetClippedBounds(), that the clip I'm computing is not obeying? [1] http://searchfox.org/mozilla-central/rev/1c1a5cef772214e9cff487040ac3068d56e96cc6/layout/painting/nsDisplayList.cpp#1962

I should also mention that I don't think this issue is specific to WebRender. It looks like the only reason it shows up in a WebRender test run and not in a non-WebRender test run, is that we only get into nsDisplayList::ComputeVisibilityForSublist() to begin with in the WebRender test run.

I have the test passing on WebRender now [1] (and not regressing other things [2]). The Android test failure remains to be fixed. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=85677b713a465a80f8de9619e443f54aab878641 [2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=af98a934ebddaca32edaddb23d5ad7807bc3c3bf

(In reply to Botond Ballo [:botond] from comment #97) > The Android test failure remains to be fixed. The issue seems to be related to the css-to-device-pixel scale. The testcase renders incorrectly on desktop too with a full zoom applied.

The problem was that ComputeClipForMaskItem() was unconditionally multiplying the computed clip rect by the css-to-device-pixel ratio, even though for some of the code paths that computed it, it was already in device pixels. (The SVG code doesn't do a great job of using typed units - it seems to use gfxRect sometimes for CSS units and sometimes for device pixels.) With that addressed, I've got a green try push! https://treeherder.mozilla.org/#/jobs?repo=try&revision=49d21ba7f74176ed4e7d576cc051015b0c248d08

Comment on attachment 8915339 [details] Bug 1382534 - Relax the requirement of having a clip with respect to an ASR for nsDisplayMask items. https://reviewboard.mozilla.org/r/186532/#review200718 I was stuck on this patch for a long time, trying to come up with a name for the method that doesn't make the nsDisplayMask implementation a lie, but I couldn't find a better name. So let's just go with this.

Comment on attachment 8919813 [details] Bug 1382534 - Use GetBounds() in nsDisplayMask::ComputeVisibility(). https://reviewboard.mozilla.org/r/190740/#review200720

Rebased patch series.

One more all-platform Try push to be on the safe side: https://treeherder.mozilla.org/#/jobs?repo=try&revision=85c3345af12db0a1f8d1990db4fff01dc3ec9a2f

Comment on attachment 8915335 [details] Bug 1382534 - Account for the local transform in nsSVGClipPathFrame::GetBBoxForClipPathFrame(). Carrying r=longsonr.

Comment on attachment 8915338 [details] Bug 1382534 - Only reuse the cached path in SVGGeometryElement::GetOrBuildPath() if it has the fill rule we want. Carrying r=longsonr.

Pushed by bballo@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/98355059c8d1 Layerize active masks even with HWA disabled. r=mstange https://hg.mozilla.org/integration/autoland/rev/4923de331172 Move ComputeOffsetToUserSpace() from nsDisplayList.cpp to nsLayoutUtils. r=mstange https://hg.mozilla.org/integration/autoland/rev/8148797eab63 Add a UnionMaybeRects() method to gfx/2d/Rect.h. r=mstange https://hg.mozilla.org/integration/autoland/rev/abc890fafaef Account for the local transform in nsSVGClipPathFrame::GetBBoxForClipPathFrame(). r=longsonr https://hg.mozilla.org/integration/autoland/rev/441b39c10487 Make layout/reftests/svg/clipPath-polygon-elementFromPoint-01.svg a bit more permissive. r=mstange https://hg.mozilla.org/integration/autoland/rev/9487322615de Use clip-rule instead of fill-rule when calculating the bounding box contribution of an SVG geometry frame inside a clip-path. r=longsonr https://hg.mozilla.org/integration/autoland/rev/9c4ef73e29b1 Only reuse the cached path in SVGGeometryElement::GetOrBuildPath() if it has the fill rule we want. r=longsonr https://hg.mozilla.org/integration/autoland/rev/a5cc27bbb7b4 Add nsCSSClipPathInstance::GetBoundingRectForBasicShapeClip(). r=mstange https://hg.mozilla.org/integration/autoland/rev/486bc3b21ec6 Relax the requirement of having a clip with respect to an ASR for nsDisplayMask items. r=mstange https://hg.mozilla.org/integration/autoland/rev/5a6831152e15 Use GetBounds() in nsDisplayMask::ComputeVisibility(). r=mstange https://hg.mozilla.org/integration/autoland/rev/296d10ae6f34 Try to give nsDisplayMask items proper scroll metadata. r=mstange https://hg.mozilla.org/integration/autoland/rev/29a923b7d812 Add a crashtest for the ASR-related assertion failure. r=mstange https://hg.mozilla.org/integration/autoland/rev/ce7f71c2f402 Add an async-scrolling reftest. r=mstange

https://hg.mozilla.org/mozilla-central/rev/98355059c8d1 https://hg.mozilla.org/mozilla-central/rev/4923de331172 https://hg.mozilla.org/mozilla-central/rev/8148797eab63 https://hg.mozilla.org/mozilla-central/rev/abc890fafaef https://hg.mozilla.org/mozilla-central/rev/441b39c10487 https://hg.mozilla.org/mozilla-central/rev/9487322615de https://hg.mozilla.org/mozilla-central/rev/9c4ef73e29b1 https://hg.mozilla.org/mozilla-central/rev/a5cc27bbb7b4 https://hg.mozilla.org/mozilla-central/rev/486bc3b21ec6 https://hg.mozilla.org/mozilla-central/rev/5a6831152e15 https://hg.mozilla.org/mozilla-central/rev/296d10ae6f34 https://hg.mozilla.org/mozilla-central/rev/29a923b7d812 https://hg.mozilla.org/mozilla-central/rev/ce7f71c2f402

Verified fixed in 58 and 59.