Open Bug 914919 Opened 6 years ago Updated 2 years ago

"Assertion failure: aXMost >= x" with position:sticky and huge width

Categories

(Core :: Layout: Positioned, defect, critical)

x86_64
macOS
defect
Not set
critical

Tracking

()

REOPENED
mozilla26
Tracking Status
firefox26 --- fixed
firefox27 --- affected

People

(Reporter: jruderman, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files)

Attached file testcase
With:
  user_pref("layout.css.sticky.enabled", true);

Assertion failure: aXMost >= x, at BaseRect.h:285

It might be best to downgrade this to a warning until bug 765861 is fixed.
Attached file stack
I have partially-reduced testcases for most of the other BaseRect.h assertions as well.
Yeah, I don't doubt they're too hard to trigger. I had wondered at the use of MOZ_ASSERT there, so downgrading them for now sounds fine to me.
(In reply to Jesse Ruderman from comment #0)
> It might be best to downgrade this to a warning until bug 765861 is fixed.

I'd lean towards downgrading to (non-fatal) NS_ASSERTION for now.

The assertions are there per a review comment from roc over in bug 741682 comment 37, and I know he prefers non-fatal assertions, so I'm sure he'd be fine with that change.
Assignee: nobody → cford
Attachment #803366 - Flags: review?(dholbert) → review+
Odd to leave something as an assertion when we know how to make it fail. roc, what do you think?
Flags: needinfo?(roc)
Yes, this is a bug that we should fix rather than just downgrading the assertion. However, we have to evaluate the priority, and the reality is that this is probably not high priority. It doesn't seem high priority enough for a fatal assertion at least.
Flags: needinfo?(roc)
Cool. This is checkin-needed, then.
Keywords: checkin-needed
Increased the assertion range to 1-3 for winWidget in https://hg.mozilla.org/integration/mozilla-inbound/rev/6de097e252be since your first Win8 hit three, time will tell whether I should have done it for everything.
And because no attempt to minimize annotations goes unpunished, 1-3 for everyone in http://hg.mozilla.org/integration/mozilla-inbound/rev/557c33ea680d once a Linux32 run did three.
Thanks, philor!
Hrm, is NS_ASSERTION allowed in gfx/2d?
No, good point.

But we don't have non-fatal assertions in MFBT yet. We should. We agreed to add MOZ_NONFATAL_ASSERT a while back.
Depends on: 924768
I'm somewhat tempted to back this out, since it turns out including nsDebug.h before Skia headers can cause some pain, due to a "#define free" that nsDebug inadvertantly pulls in. (see bug 924768 comment 4 and 6).
Sounds fine to me.
OK, backed out per comment 17 (which builds on comment 15): https://hg.mozilla.org/integration/mozilla-inbound/rev/5f9cb0332e3c

Not sure what the best way forward is here. Fortunately, this bug doesn't affect real-world behavior at all (recall that the patch was just an assertion-softening).  But if this bug blocks fuzzers, maybe we should just disable these assertions, or prioritize a MOZ_NONFATAL_ASSERT implementation so that we can soften the assertions in a more kosher way.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Clearing assignee (from when this was formerly-closed), since this doesn't need to be on Corey's plate, since his internship is over. (though Corey, if you have other ideas for how to attack this, feel free to reclaim it. :) Otherwise, someone else can probably take it, depending on prioritization.)
Assignee: corey → nobody
Flags: in-testsuite+ → in-testsuite?
The testcase from comment 0 still reproduces in m-c rev 20171011-f3e939a81ee1, and this comes up fairly often while fuzzing. Can this assertion be softened yet?
You need to log in before you can comment on or make changes to this bug.