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

RESOLVED FIXED in mozilla27

Status

()

defect
--
critical
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jruderman, Assigned: coyotebush)

Tracking

(Blocks 1 bug, {assertion, testcase})

Trunk
mozilla27
x86_64
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Posted file 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
Posted file stack
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.
Assignee: nobody → cford
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.
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.
(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.)
(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.)
That works. (This applies on top of the patch from bug 904197.)
Attachment #804796 - Flags: review?(dholbert)
Attachment #804796 - Flags: review?(dholbert) → review+
(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).
Like so. Mind running this by Try and/or requesting uplift, if you think either of those are warranted?
Attachment #804796 - Attachment is obsolete: true
Attachment #806247 - Flags: review+
(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.
Once more, asking for unit tests this time: https://tbpl.mozilla.org/?tree=Try&rev=c1a548a6a87b
Flags: needinfo?(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.
Flags: needinfo?(dholbert)
(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.
Oh, but not in opt builds, right.
https://hg.mozilla.org/mozilla-central/rev/94b97b38878f
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in before you can comment on or make changes to this bug.