1325038 - Hit NS_ASSERTION in PreEffectsVisualOverflowCollector::GetPreEffectsVisualOverflowRect

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[u459114]

1. Visit https://jsfiddle.net/u7v4waq2/1/
2. Hover text that says "Wrapped sentence"

static nsRect GetPreEffectsVisualOverflowRect(nsIFrame* aFrame) {
    //...
    NS_ASSERTION(aFrame->GetParent()->StyleContext()->GetPseudo() ==
                   nsCSSAnonBoxes::mozAnonymousBlock,
                 "How did we getting here, then?");
  }

Actual results:
the first assertion in GetPreEffectsVisualOverflowRect failed.

Expect results:
No assertion failed.

Comment 1

[u459114]

We won't see this assertion by by replace filter by mask, this is because this if statement:
https://hg.mozilla.org/mozilla-central/file/39ca4e6d2eb2/layout/svg/nsSVGIntegrationUtils.cpp#l279

Comment 2

[u459114]

Attachment: Bug 1325038 - Part 1. Correct assertion logic.

In the test case of bug 1324809:
1. A span is been broken into two continuation frames: FA and FB. FA is the first
connituation
2. Adding a filter effect to this span.
3. FA::FinishAndStoreOverflow is called. This function will call ComputeEffect:
  if (nsSVGIntegrationUtils::UsingEffectsForFrame(aFrame)) {
    aFrame->Properties().
    Set(nsIFrame::PreEffectsBBoxProperty(), new nsRect(r)); // Now FA has
                                                            // PreEffectsBBoxProperty
                                                            // but FB does not
                                                            // have yet.
    // ComputePostEffectsVisualOverflowRect will iterate all continuations from
    // FA to FB. At this moment, FB does not carry PreEffectsBBoxProperty,
    // assertion failure.
    r = nsSVGIntegrationUtils::ComputePostEffectsVisualOverflowRect(aFrame, r);
  }
4. FB::FinishAndStoreOverflow is called. But already too late.

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

Comment 3

[u459114]

Attachment: Bug 1325038 - Part 2. Reftest for bug 1324809.

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

Comment 4

[u459114]

Comment on attachment 8825425 [details]
Bug 1325038 - Part 2. Reftest for bug 1324809.

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

Comment 5

[Cameron McCormack (:heycam)]

Comment on attachment 8825424 [details]
Bug 1325038 - Part 1. Correct assertion logic.

https://reviewboard.mozilla.org/r/103590/#review104450

::: layout/svg/nsSVGIntegrationUtils.cpp:113
(Diff revision 1)
>      // With image masks, there is one more exception.
>      //

Can you adjust this to say there are two exceptions.

::: layout/svg/nsSVGIntegrationUtils.cpp:121
(Diff revision 1)
> +    // Bug 1325038 show us another exception. For continuation siblings,
> +    // except the first one, you can not guarantee each of them carry
> +    // a PreEffectsBBoxProperty property.

Nit: no need to mention the bug number here.  (Usually we leave bug numbers in the source for bugs which are still open and need to be fixed.)  Someone interested can see which bug we added this comment from by looking at the blame.

::: layout/svg/nsSVGIntegrationUtils.cpp:124
(Diff revision 1)
> +    DebugOnly<nsIFrame*> firstFrame =
> +      nsLayoutUtils::FirstContinuationOrIBSplitSibling(aFrame);

Using DebugOnly<> like this won't avoid FirstContinuationOrIBSplitSibling from being called in non-DEBUG builds.  So, put this in an #ifdef DEBUG block and drop the DebugOnly<>.

Or: make the assertion just call nsLayoutUtils::IsFirstContinuationOrIBSplitSibling directly.

Comment 6

[Cameron McCormack (:heycam)]

Comment on attachment 8825425 [details]
Bug 1325038 - Part 2. Reftest for bug 1324809.

https://reviewboard.mozilla.org/r/103592/#review104452

::: layout/reftests/svg/filter-on-continuation-box-01.html:17
(Diff revision 2)
> +      document.getElementById("myspan").style.filter = "opacity(100%)";
> +      document.documentElement.classList.remove("reftest-wait");

Is it worth making the test cause a change in appearance, e.g. use opacity(50%) and set |opacity: 0.5| in the reference?  Otherwise, we could just make this a crashtest.

Comment 7

[u459114]

Comment on attachment 8825424 [details]
Bug 1325038 - Part 1. Correct assertion logic.

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

Comment 8

[u459114]

Comment on attachment 8825425 [details]
Bug 1325038 - Part 2. Reftest for bug 1324809.

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

Comment 9

[u459114]

Comment on attachment 8825425 [details]
Bug 1325038 - Part 2. Reftest for bug 1324809.

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

Comment 10

[u459114]

Comment on attachment 8825425 [details]
Bug 1325038 - Part 2. Reftest for bug 1324809.

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

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9d8c6f724256
Part 1. Correct assertion logic. r=heycam
https://hg.mozilla.org/integration/autoland/rev/f1690763f5a4
Part 2. Reftest for bug 1324809. r=heycam

Comment 12

[Phil Ringnalda (:philor)]

Backed out in https://hg.mozilla.org/integration/autoland/rev/bcacc567af8d - 4 of 10 runs on Win7 debug hit https://treeherder.mozilla.org/logviewer.html#?job_id=68047747&repo=autoland

Comment 13

[u459114]

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

Comment 14

[u459114]

Comment on attachment 8825424 [details]
Bug 1325038 - Part 1. Correct assertion logic.

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

Comment 15

[u459114]

Comment on attachment 8825425 [details]
Bug 1325038 - Part 2. Reftest for bug 1324809.

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

Comment 16

[u459114]

Comment on attachment 8825424 [details]
Bug 1325038 - Part 1. Correct assertion logic.

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

Comment 17

[u459114]

Comment on attachment 8825425 [details]
Bug 1325038 - Part 2. Reftest for bug 1324809.

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

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/164dd8cbfe85
Part 1. Correct assertion logic. r=heycam
https://hg.mozilla.org/integration/autoland/rev/b02dbc8e2208
Part 2. Reftest for bug 1324809. r=heycam

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/164dd8cbfe85
https://hg.mozilla.org/mozilla-central/rev/b02dbc8e2208