1365831 - dynamic layout change that resizes conditionally processed outer SVG triggers "expected aContainer to be NS_FRAME_IS_DIRTY" assertion

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: Replace assertion that non-display SVG containers are only reflowed with NS_FRAME_IS_DIRTY with a real test of the condition

The primary patch in bug 1308876 causes frames to be reflowed less often
with NS_FRAME_IS_DIRTY, particularly when multiple passes of reflow are
required for the frame or one of its ancestors (which is generally the
case for a document that ends up not having scrollbars).  This change
causes this assert to fire on various SVG tests such as
layout/reftests/svg/svg-integration/conditions-outer-svg-01.xhtml .
This happens because the outer SVG with conditional processing (in this
test, systemLanguage="x") is reflowed due to its parent resizing,
without NS_FRAME_IS_DIRTY set.  This is a relatively normal thing to
happen during reflow; we just didn't have any tests that exercise it.

This patch adds a crashtest that triggers the assertion through the same
mechanism, but with a dynamic change, rather than depending on the
non-dirty reflow triggered by bug 1308876.  (I confirmed locally that
this test does trigger the assertion without this patch, when run in the
crashtest harness.)

I think fundamentally the assertion isn't valid, and we should instead
be testing the condition that it asserts.  (But I'm a little concerned
that maybe we need resize reflows to be propagated through?)

MozReview-Commit-ID: D8hjAbjKyuL

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #1)
> I think fundamentally the assertion isn't valid, and we should instead
> be testing the condition that it asserts.  (But I'm a little concerned
> that maybe we need resize reflows to be propagated through?)

To clarify this:  I think the two obvious options are:
 (1) remove the assertion, and replace it with the test, as this patch does
 (2) remove the assertion, and replace it with nothing

Comment 3

[Cameron McCormack (:heycam)]

Comment on attachment 8868844 [details] [diff] [review]
Replace assertion that non-display SVG containers are only reflowed with NS_FRAME_IS_DIRTY with a real test of the condition

Review of attachment 8868844 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the explanation, which makes sense to me.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=dac386428a8d7140bec1783987d6fee08ed939d8&group_state=expanded

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4808d8fe4ea887a468fc8f1083ee51ef20f4d6ce
Bug 1365831 - Replace assertion that non-display SVG containers are only reflowed with NS_FRAME_IS_DIRTY with a real test of the condition.  r=heycam

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4808d8fe4ea8