1320364 - Fix "ASSERTION: How did we getting here, then?: 'aFrame->GetParent()->StyleContext()->GetPseudo() == nsCSSAnonBoxes::mozAnonymousBlock"

Assignee

Description

•

8 years ago

This is an assertion failure while using gradient mask on xul.
nsSVGIntegrationUtils::UsingEffectsForFrame at [1] always return false for the nsXULScrollFrame with gradient mask

  if (nsSVGIntegrationUtils::UsingEffectsForFrame(aFrame)) {
    aFrame->Properties().
      Set(nsIFrame::PreEffectsBBoxProperty(), new nsRect(r));
    r = nsSVGIntegrationUtils::ComputePostEffectsVisualOverflowRect(aFrame, r);
  }


[1] http://searchfox.org/mozilla-central/source/layout/generic/nsFrame.cpp#6455

u459114

Assignee

Updated

•

8 years ago

Blocks: mask-image

Comment hidden (mozreview-request)

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

u459114

Assignee

Comment 2

•

8 years ago

There are two ways to fix this assertion
1. In nsStyleImageLayers::Layer::CalcDifference[1]
   By add the following line
   hint |= nsChangeHint_UpdateOverflow;
   to trigger reflow, so that we recompute PreEffectsBBoxProperty during overflow computation
2. GetPreEffectsVisualOverflowRect[2]
   Please see the change in the patch.
   Correct assertion itself.


[1] http://searchfox.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#2887
[2] http://searchfox.org/mozilla-central/source/layout/svg/nsSVGIntegrationUtils.cpp#113

u459114

Assignee

Updated

•

8 years ago

Attachment #8814820 - Flags: review?(cam)

Cameron McCormack (:heycam)

Comment 3

•

8 years ago

mozreview-review

Comment on attachment 8814820 [details]
Bug 1320364 - Correct NS_ASSERTION failure condition in GetPreEffectsVisualOverflowRect.

https://reviewboard.mozilla.org/r/95922/#review96296

One disadvantage of this approach is that if we have non-image masks on the same element, and those non-image masks changed, and we somehow did not generate nsChangeHint_UpdateOverflow for them, the assertion here would not run and we would miss it.

I was going to suggest asserting at the end of this function, that the value returned from GetVisualOverflowRect() is the one that we would get from PreEffectsBBoxProperty if we were to set it, but I think that would be difficult to extract out of nsIFrame::FinishAndStoreOverflow.

Even if mImage.IsEmpty() returns true for SVG mask references, I guess this means we will skip the assertion if, for example, we we a single SVG mask reference before the style change, and a single gradient mask after the style change.

Still, I'm not sure what we could be doing better, if we want to avoid the unnecessary UpdateOverflow work.  I don't think we want to do that only in DEBUG builds, since that would make debug and release builds too different.  We could have a new change hint that sets PreEffectsBBoxProperty to the same value as GetVisualOverflowRect() (and which if returned in conjunction with UpdateOverflow, would be ignored).  But again this seems like a lot of work just to keep this assertion working.

::: layout/svg/nsSVGIntegrationUtils.cpp:121
(Diff revision 1)
> +    bool maybeHasImageMask = false;
> +
> +    const nsStyleSVGReset* effects = aFrame->StyleSVGReset();
> +    for (uint32_t i = 0; i < effects->mMask.mImageCount; i++){

I'd prefer to see this function on nsStyleImageLayers itself, so that you could simplify the change in this file to just adding "aFrame->StyleSVGReset()->mMask.HasNonSVGMask() || " to the assertion condition.

::: layout/svg/nsSVGIntegrationUtils.cpp:124
(Diff revision 1)
> +    // So even we apply a new mask image to this frame, PreEffectsBBoxProperty
> +    // might still left empty.
> +    bool maybeHasImageMask = false;
> +
> +    const nsStyleSVGReset* effects = aFrame->StyleSVGReset();
> +    for (uint32_t i = 0; i < effects->mMask.mImageCount; i++){

Nit: space before "{".

::: layout/svg/nsSVGIntegrationUtils.cpp:126
(Diff revision 1)
> +    bool maybeHasImageMask = false;
> +
> +    const nsStyleSVGReset* effects = aFrame->StyleSVGReset();
> +    for (uint32_t i = 0; i < effects->mMask.mImageCount; i++){
> +      const nsStyleImageLayers::Layer& layer = effects->mMask.mLayers[i];
> +      if (!layer.mImage.IsEmpty()) {

Doesn't mImage.IsEmpty() return false for SVG mask references too?  (Although maybe not after you fix bug 1301245?)

u459114

Assignee

Comment 4

•

8 years ago

mozreview-review-reply

Comment on attachment 8814820 [details]
Bug 1320364 - Correct NS_ASSERTION failure condition in GetPreEffectsVisualOverflowRect.

https://reviewboard.mozilla.org/r/95922/#review96296

if we were to set PreEffectsBBoxProperty, we will get an early return at the beginning of this function. So add an assertion to compare GetVisualOverflowRect() and the value we set to PreEffectsBBoxProperty in the end of this function does not seem to be possible.

If we have a single mask reference, and replace it by a single gradient mask, we will actually run into [1] and trigger reflow. So that should be OK.

[1] http://searchfox.org/mozilla-central/source/layout/style/nsStyleStruct.cpp#2876

Comment hidden (mozreview-request)

Comment on attachment 8814820 [details]
Bug 1320364 - Correct NS_ASSERTION failure condition in GetPreEffectsVisualOverflowRect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95922/diff/1-2/

Comment hidden (mozreview-request)

Comment on attachment 8814820 [details]
Bug 1320364 - Correct NS_ASSERTION failure condition in GetPreEffectsVisualOverflowRect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95922/diff/2-3/

u459114

Assignee

Comment 7

•

8 years ago

(In reply to Cameron McCormack (:heycam) from comment #3)
> Comment on attachment 8814820 [details]
> One disadvantage of this approach is that if we have non-image masks on the
> same element, and those non-image masks changed, and we somehow did not
> generate nsChangeHint_UpdateOverflow for them, the assertion here would not
> run and we would miss it.
Both clip-path and filter change do generate nsChangeHint_UpdateOverflow.
In the last patch, we skip this checking only if we have *only* image or gradient mask

Comment hidden (mozreview-request)

Comment on attachment 8814820 [details]
Bug 1320364 - Correct NS_ASSERTION failure condition in GetPreEffectsVisualOverflowRect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95922/diff/3-4/

Dão Gottwald [:dao]

Comment 9

•

8 years ago

A try run with your patch and mine gives me:

Assertion failure: paintResult.result != DrawResult::SUCCESS, at /home/worker/workspace/build/src/layout/svg/nsSVGIntegrationUtils.cpp:904

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

u459114

Assignee

Comment 10

•

8 years ago

(In reply to Dão Gottwald [:dao] from comment #9)
> A try run with your patch and mine gives me:
> 
> Assertion failure: paintResult.result != DrawResult::SUCCESS, at
> /home/worker/workspace/build/src/layout/svg/nsSVGIntegrationUtils.cpp:904
> 
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=b2f340c0eb83311e78555c329c180d76dbb39f7f

I didn't hit that assertion. That assertion is to tell me that I may have a logic error in CreateAndPaintMaskSurface. You can just remove that assertion first.

Comment hidden (mozreview-request)

Comment on attachment 8814820 [details]
Bug 1320364 - Correct NS_ASSERTION failure condition in GetPreEffectsVisualOverflowRect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95922/diff/4-5/

Dão Gottwald [:dao]

Comment 12

•

8 years ago

(In reply to C.J. Ku[:cjku](UTC+8)[PTO: 12/1~12/5] from comment #10)
> (In reply to Dão Gottwald [:dao] from comment #9)
> > A try run with your patch and mine gives me:
> > 
> > Assertion failure: paintResult.result != DrawResult::SUCCESS, at
> > /home/worker/workspace/build/src/layout/svg/nsSVGIntegrationUtils.cpp:904
> > 
> > https://treeherder.mozilla.org/#/
> > jobs?repo=try&revision=b2f340c0eb83311e78555c329c180d76dbb39f7f
> 
> I didn't hit that assertion. That assertion is to tell me that I may have a
> logic error in CreateAndPaintMaskSurface. You can just remove that assertion
> first.

I'm not sure what you're telling me here. I don't work on layout code (although I could give it a shot if necessary). Should I file another bug on removing the above assertion?

Flags: needinfo?(cku)

Astley Chen (inactive)

Comment 13

•

8 years ago

The assertion code should be removed in below push:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c63c761e089c

I think the try with just your patches should be fine now.

Status: NEW → ASSIGNED

Flags: needinfo?(cku)

Dão Gottwald [:dao]

Comment 14

•

8 years ago

(In reply to Astley Chen [:astley] UTC+8 from comment #13)
> The assertion code should be removed in below push:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/c63c761e089c
> 
> I think the try with just your patches should be fine now.

Looks good:

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

Thanks!

Cameron McCormack (:heycam)

Comment 15

•

8 years ago

mozreview-review

Comment on attachment 8814820 [details]
Bug 1320364 - Correct NS_ASSERTION failure condition in GetPreEffectsVisualOverflowRect.

https://reviewboard.mozilla.org/r/95922/#review97942

::: layout/svg/nsSVGEffects.h:496
(Diff revision 5)
>      /**
>       * @return an array which contains all SVG mask frames.
>       */
>      nsTArray<nsSVGMaskFrame*> GetMaskFrames();
>  
> +    bool MightHasNoneSVGMask() const;

"MightHaveNonSVGMask"

::: layout/svg/nsSVGEffects.cpp:694
(Diff revision 5)
> +  const nsTArray<RefPtr<nsSVGPaintingProperty>>& props = mMask->GetProps();
> +  for (size_t i = 0; i < props.Length(); i++) {
> +    nsSVGMaskFrame* maskFrame =
> +      static_cast<nsSVGMaskFrame *>(props[i]->GetReferencedFrame(
> +                                                nsGkAtoms::svgMaskFrame, &ok));
> +    if (!maskFrame) {
> +      return true;
> +    }
> +  }

There's no need to do the static_cast since we're just checking for null.  With a ranged for loop too, we can probably collapse this down a bit to:

  for (nsSVGPaintingProperty* prop : mMask->GetProps()) {
    if (!(prop->GetReferencedFrame(...))) {
      return true;
    }
  }

::: layout/svg/nsSVGIntegrationUtils.cpp:113
(Diff revision 5)
>      //
>      // If we ever got passed a frame with the PreTransformOverflowAreasProperty
>      // property set, that would be bad, since then our GetVisualOverflowRect()
>      // call would give us the post-effects, and post-transform, overflow rect.
>      //
> -    NS_ASSERTION(aFrame->GetParent()->StyleContext()->GetPseudo() ==
> +    // After enable image mask, we bring is one more exception.

Nit: "After enabling image mask" sounds like it is describing a change in the code, but I think it's better to describe in comments the current state of the code.  So maybe write "With image masks, there is one more exception."

::: layout/svg/nsSVGIntegrationUtils.cpp:118
(Diff revision 5)
> -    NS_ASSERTION(aFrame->GetParent()->StyleContext()->GetPseudo() ==
> +    // After enable image mask, we bring is one more exception.
> +    //
> +    // In nsStyleImageLayers::Layer::CalcDifference, we do not add
> +    // nsChangeHint_UpdateOverflow hint when image mask(not SVG mask) property
> +    // value changed, since replace image mask does not cause layout change.
> +    // So even we apply a new mask image to this frame, PreEffectsBBoxProperty

s/even/even if/

Attachment #8814820 - Flags: review?(cam) → review+

Comment hidden (mozreview-request)

Comment on attachment 8814820 [details]
Bug 1320364 - Correct NS_ASSERTION failure condition in GetPreEffectsVisualOverflowRect.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/95922/diff/5-6/

Pulsebot

Comment 17

•

8 years ago

Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/68870b70c466
Correct NS_ASSERTION failure condition in GetPreEffectsVisualOverflowRect. r=heycam

Carsten Book [:Tomcat]

Comment 18

•

8 years ago

bugherder

https://hg.mozilla.org/mozilla-central/rev/68870b70c466

Status: ASSIGNED → RESOLVED

Closed: 8 years ago

status-firefox53: --- → fixed

Resolution: --- → FIXED

Target Milestone: --- → mozilla53

Nobody; OK to take it and work on it

Updated

•

6 years ago

Component: Layout: View Rendering → Layout: Web Painting