Closed
Bug 914919
Opened 11 years ago
Closed 3 years ago
"Assertion failure: aXMost >= x" with position:sticky and huge width
Categories
(Core :: Layout: Positioned, defect)
Tracking
()
RESOLVED
WORKSFORME
mozilla26
People
(Reporter: jruderman, Unassigned)
References
Details
(Keywords: assertion, testcase)
Attachments
(3 files)
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.
Reporter | ||
Comment 1•11 years ago
|
||
Reporter | ||
Comment 2•11 years ago
|
||
I have partially-reduced testcases for most of the other BaseRect.h assertions as well.
Comment 3•11 years ago
|
||
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.
Comment 4•11 years ago
|
||
(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.
Comment 5•11 years ago
|
||
Attachment #803366 -
Flags: review?(dholbert)
Updated•11 years ago
|
Assignee: nobody → cford
Updated•11 years ago
|
Attachment #803366 -
Flags: review?(dholbert) → review+
Updated•11 years ago
|
Keywords: checkin-needed
Comment 6•11 years ago
|
||
Let's double-check. https://tbpl.mozilla.org/?tree=Try&rev=5cb6567a32a5
Keywords: checkin-needed
Reporter | ||
Comment 7•11 years ago
|
||
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)
Comment 10•11 years ago
|
||
Flags: in-testsuite+
Keywords: checkin-needed
Comment 11•11 years ago
|
||
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.
Comment 12•11 years ago
|
||
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.
Comment 13•11 years ago
|
||
Thanks, philor!
Comment 14•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e67dad88f860
https://hg.mozilla.org/mozilla-central/rev/6de097e252be
https://hg.mozilla.org/mozilla-central/rev/557c33ea680d
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Comment 15•11 years ago
|
||
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.
Comment 17•11 years ago
|
||
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).
Comment 18•11 years ago
|
||
Sounds fine to me.
Comment 19•11 years ago
|
||
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
status-firefox26:
--- → fixed
status-firefox27:
--- → affected
Resolution: FIXED → ---
Comment 20•11 years ago
|
||
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
Updated•11 years ago
|
Flags: in-testsuite+ → in-testsuite?
Comment 21•7 years ago
|
||
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?
Comment 22•3 years ago
|
||
Hey Jesse,
Can you still reproduce this issue or should we close it?
Flags: needinfo?(jruderman)
Comment 23•3 years ago
|
||
Looks like the assertion in question was removed in bug 1428348.
Status: REOPENED → RESOLVED
Closed: 11 years ago → 3 years ago
Flags: needinfo?(jruderman)
Resolution: --- → WORKSFORME
Comment 24•3 years ago
|
||
(and I confirmed that I don't get any assertions in a debug build with the attached testcase)
You need to log in
before you can comment on or make changes to this bug.
Description
•