Closed Bug 1320364 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: Web Painting, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: u459114, Assigned: u459114)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

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
Blocks: mask-image
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
Attachment #8814820 - Flags: review?(cam)
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?)
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
(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
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
(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.
(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)
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)
(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!
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+
Pushed by cku@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/68870b70c466
Correct NS_ASSERTION failure condition in GetPreEffectsVisualOverflowRect. r=heycam
https://hg.mozilla.org/mozilla-central/rev/68870b70c466
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
Component: Layout: View Rendering → Layout: Web Painting
You need to log in before you can comment on or make changes to this bug.