915475 - Assertion failure: "Non-display SVG do not maintain visual overflow rects" with position:sticky and SVG requiredFeatures

Status: RESOLVED FIXED

(Core :: Layout: Positioned, defect)

Description

[Jesse Ruderman]

Attachment: testcase

Assertion failure: !(mState & (nsFrameState(1) << (43))) || !(mState & (nsFrameState(1) << (53))) (Non-display SVG do not maintain visual overflow rects), at /Users/jruderman/trees/mozilla-central/layout/generic/nsFrame.cpp:5150

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Corey Ford [:coyotebush]]

So I avoid doing much with sticky positioned SVG text frames, but I guess I need to be more careful about what I add to my OverflowChangedTracker.

Comment 3

[Corey Ford [:coyotebush]]

Presumably I need to avoid updating overflow of things that don't maintain overflow, either in OverflowChangedTracker itself or when adding frames to it. Not sure whether nsIFrame::HasOverflowAreas is right here, or whether I should do the same check as that assertion.

Comment 4

[Daniel Holbert [:dholbert]]

I don't think you want HasOverflowAreas.  Probably best to just check frame->GetStateBits() & NS_FRAME_IS_NONDISPLAY, and exempt all nondisplay frames.

Probably makes sense to do this check in the spot where we append to StickyScrollContainer::mFrames, and reject nondisplay frames at that point.

Comment 5

[Daniel Holbert [:dholbert]]

(Specifically, I think we should add !(frame->GetStateBits() & NS_FRAME_IS_NONDISPLAY) to the list of things we check when we call AddFrame for the ssc, in nsFrame::Init.)

Comment 6

[Daniel Holbert [:dholbert]]

(We probably want to add the same check in nsFrame::DestroyFrom, for symmetry, but it doesn't matter as much there because calling RemoveFrame() on un-tracked frames will just silently fail.)

Comment 7

[Corey Ford [:coyotebush]]

Attachment: Don't sticky position frames with NS_FRAME_IS_NONDISPLAY.

That works. (This applies on top of the patch from bug 904197.)

Comment 8

[Corey Ford [:coyotebush]]

(In reply to Corey Ford [:corey] from comment #7)
> (This applies on top of the patch from bug 904197.)

I should probably reorder those patches, since this one should be higher priority to land (& perhaps uplift to aurora).

Comment 9

[Corey Ford [:coyotebush]]

Attachment: Don't sticky position frames with NS_FRAME_IS_NONDISPLAY.

Like so. Mind running this by Try and/or requesting uplift, if you think either of those are warranted?

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Corey Ford [:corey] from comment #8)
> I should probably reorder those patches, since this one should be higher
> priority to land (& perhaps uplift to aurora).

Agreed. (though fwiw I'm intending to look into fixing the 1px-off issue on the other bug's reftests in the next few days, so that it can land.)

(In reply to Corey Ford [:corey] from comment #9)
> Like so. Mind running this by Try and/or requesting uplift, if you think
> either of those are warranted?

Will do both.

Comment 11

[Daniel Holbert [:dholbert]]

https://tbpl.mozilla.org/?tree=Try&rev=bbe69b85e8e2

Comment 12

[Daniel Holbert [:dholbert]]

Once more, asking for unit tests this time: https://tbpl.mozilla.org/?tree=Try&rev=c1a548a6a87b

Comment 13

[Daniel Holbert [:dholbert]]

Landed on inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/94b97b38878f

Comment 14

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #10)
> (In reply to Corey Ford [:corey] from comment #9)
> > Like so. Mind running this by Try and/or requesting uplift, if you think
> > either of those are warranted?
> 
> Will do both.

In retrospect, I think probably not worth requesting uplift for this, given that
 (a) the feature is disabled by default (on Aurora at least; won't be disabled on Nightly for much longer)
 (b) for any people who opt to turn on the pref: no one's going to hit this in real-world content
 (c) for any people who *both* turn on the pref and actually *do* hit content that triggers this: I don't think it's a particularly dangerous situation / scary assertion. I think it's just a case where we're spinning our wheels and wasting a few CPU cycles.

Comment 15

[Corey Ford [:coyotebush]]

(In reply to Daniel Holbert [:dholbert] from comment #14)
>  (c) for any people who *both* turn on the pref and actually *do* hit
> content that triggers this: I don't think it's a particularly dangerous
> situation / scary assertion. I think it's just a case where we're spinning
> our wheels and wasting a few CPU cycles.

And crashing. But you're right that it's quite unlikely to be hit.

Comment 16

[Corey Ford [:coyotebush]]

Oh, but not in opt builds, right.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/94b97b38878f