Assertion failure: FrameMaintainsOverflow(this) (Non-display SVG do not maintain visual overflow rects), at layout/generic/nsFrame.cpp:5263

Thanks for filing this, Jesse. I just independently ran into this in bug 1194480 -- my patch over there gives another route to triggering this same problem. So, I'd like to fix this soonish. The underlying problem here seems to be that nsChangeHint_UpdateSubtreeOverflow triggers UpdateOverflow calls for a subtree, which makes non-display SVG nodes in the subtree complain. Perhaps we should be bailing out at some point when we're queuing up descendants for overflow-updates, if those descendants don't have overflow rects. (also, bookkeeping note: this change hint was introduced in bug 727125, shortly before this bug here was filed, so I'm pretty sure this assertion-failure is a regression from bug 727125. Marking as such.)

In particular, AddSubtreeToOverflowTracker should probably check FrameMaintainsOverflow before adding a frame. (FrameMaintainsOverflow is static to nsFrame.cpp right now, but maybe it should move to nsLayoutUtils.)

This first patch does not change behavior. It does the following: - Promotes FrameMaintainsOverflow to be an nsIFrame accessor (placed alongside a bunch of similar convenience methods that mostly check state bits), so that we'll be able to call it from another file in the next patch. - Simplifies the FrameMaintainsOverflow implementation to use HasAllStateBits. - Tweaks RestyleManager.cpp to use the newly-public FrameMaintainsOverflow in place of an explicit state-bit-check. (Side note: when working on this, I was wondering about the SVG bit that's checked in FrameMaintainsOverflow, and I asked myself "What about *non*-SVG frames that have NS_FRAME_IS_NONDISPLAY? Should they return true or false from FrameMaintainsOverflow?" But happily, that's a non-issue, because I don't think any such frames exist. At least, the NONDISPLAY bit is documented as being SVG-specific; and a quick MXR skim seems to confirm this. So, this all means FrameMaintainsOverflow's NS_FRAME_SVG_LAYOUT bit-check might be redundant. But it doesn't cost anything extra, and I'd rather not shake things up too much for no reason, so I'm leaving it in for the time being.)

This patch makes RestyleManager::AddSubtreeToOverflowTracker return early, for frames that don't maintain overflow areas. We do need to perform this check at that level of granularity (as opposed to, say, inside of AddSubtreeToOverflowTracker's child-loop), because the attached testcase triggers a call to AddSubtreeToOverflowTracker where the passed-in frame *is itself* a non-display SVG frame. (We could add a FrameMaintainsOverflow check in the caller, but it's simpler to just have a single early-return base case IMO, as done in this patch.) I'm including two crashtests -- one based on the attached testcase here, and one based on my attached testcase in bug 1194480. I verified each of these crashtests assert fatally when loaded as crashtests, and those fatal assertions go away when this "part 2" code-fix is applied. (Of course, the second crashtest is fine on current trunk; it only asserts fatally with bug 1194480's patch applied.)

Is it true that frames that don't maintain overflow cannot have descendants that do? (If it's not, then this patch probably isn't valid in general for UpdateSubtreeOverflow, although it might well still be valid today because we only use the hint for text-decoration and text-decoration only really propagates to certain descendants.)

Good question. I thought that it was true, but on inspection with a testcase, it looks like it's not true. nsFrame::Init *does* copy the "IS_NONDISPLAY" state bit from parent to child, but it doesn't copy the "SVG_LAYOUT" bit. So if I have a tree like <clipPath><foreignObject><html:div>, then the inner <div> gets IS_NONDISPLAY but not SVG_LAYOUT. So, the inner <div> will return true from FrameMaintainsOverflow, despite the fact that its parent does not. SO: This all means that I need to adjust part 2 so that the "FrameMaintainsOverflow" check only controls whether we add *this* frame -- not whether we add our descendants. I think.

Here's an updated version of part 2, just skipping the frame (but not its children) when FrameMaintainsOverflow returns false. crashtests still pass locally.

(I removed the deindentation from the AddFrame args, too, since they (barely) fit in 80 characters without any iffy-deindentation.)

Thanks for the review! I'll give this & bug 1194480 a final try run later today, and land them together if things still look good.

Try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9ab03d382684

https://hg.mozilla.org/integration/mozilla-inbound/rev/812aac13e8ab https://hg.mozilla.org/integration/mozilla-inbound/rev/24ded1d0386a

https://hg.mozilla.org/mozilla-central/rev/812aac13e8ab https://hg.mozilla.org/mozilla-central/rev/24ded1d0386a