All users were logged out of Bugzilla on October 13th, 2018

"ASSERTION: Leadings should be non-negative" with ruby, huge margin

RESOLVED FIXED in Firefox 40

Status

()

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jruderman, Assigned: xidorn)

Tracking

(Blocks: 1 bug, {assertion, testcase})

Trunk
mozilla40
x86_64
Mac OS X
assertion, testcase
Points:
---

Firefox Tracking Flags

(firefox40 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

4 years ago
Created attachment 8581275 [details]
testcase

###!!! ASSERTION: Leadings should be non-negative (because adding ruby annotation can only increase the size): 'startLeading >= 0 && endLeading >= 0', file layout/generic/nsRubyFrame.cpp, line 376
(Reporter)

Comment 1

4 years ago
Created attachment 8581276 [details]
stack
(Assignee)

Comment 2

4 years ago
Probably we should use NS_ASSERTION instead of MOZ_ASSERT there, it is possible to make it negative via overflow size, and I don't think this could really cause any additional security problem. It should just behave like setting a too large line-height.
(Assignee)

Comment 3

4 years ago
Created attachment 8585282 [details] [diff] [review]
patch
Assignee: nobody → quanxunzhen
Attachment #8585282 - Flags: review?(dholbert)
Comment on attachment 8585282 [details] [diff] [review]
patch

(In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #2)
> Probably we should use NS_ASSERTION instead of MOZ_ASSERT there

We do already use NS_ASSERTION, it looks like. (which is non-fatal)  Note also that NS_NOTREACHED is just a version of NS_ASSERTION -- so even with your patch, we'll still assert on the attached testcase.

> it is possible to make it negative via overflow size, and I don't think this could
> really cause any additional security problem. It should just behave like
> setting a too large line-height.

Yeah -- as long as there's no security risk, I don't think it's worth even explicitly handling integer overflow here. There are too many places to try to explicitly handle all of them, so we're generally just fine with having busted layout with stupid-huge margin values.

Jesse has a bug filed, on setting a flag in a document if we detect huge property-values, so we can soften assertions like this one in that sort of case. You might really want to convert the existing NS_ASSERTION to a NS_WARN_IF_FALSE, with an XXX comment referencing that bug number, so we can come back & convert it back to an assertion once that bug is fixed.

I can't find that bug at the moment, but Jesse might have it handy?
Attachment #8585282 - Flags: review?(dholbert) → review-
(Jesse, do you know the bug I'm thinking of at the end of comment 4?)
Flags: needinfo?(jruderman)
(Assignee)

Comment 6

4 years ago
(In reply to Daniel Holbert [:dholbert] from comment #4)
> Comment on attachment 8585282 [details] [diff] [review]
> patch
> 
> (In reply to Xidorn Quan [:xidorn] (UTC+11) from comment #2)
> > Probably we should use NS_ASSERTION instead of MOZ_ASSERT there
> 
> We do already use NS_ASSERTION, it looks like. (which is non-fatal)  Note
> also that NS_NOTREACHED is just a version of NS_ASSERTION -- so even with
> your patch, we'll still assert on the attached testcase.

Ah... I thought it was a MOZ_ASSERT. Yes, you are right, it has already been NS_ASSERTION now, so this patch is meaningless.

> > it is possible to make it negative via overflow size, and I don't think this could
> > really cause any additional security problem. It should just behave like
> > setting a too large line-height.
> 
> Yeah -- as long as there's no security risk, I don't think it's worth even
> explicitly handling integer overflow here. There are too many places to try
> to explicitly handle all of them, so we're generally just fine with having
> busted layout with stupid-huge margin values.
> 
> Jesse has a bug filed, on setting a flag in a document if we detect huge
> property-values, so we can soften assertions like this one in that sort of
> case. You might really want to convert the existing NS_ASSERTION to a
> NS_WARN_IF_FALSE, with an XXX comment referencing that bug number, so we can
> come back & convert it back to an assertion once that bug is fixed.

That sounds reasonable.

> I can't find that bug at the moment, but Jesse might have it handy?
Ah, looks like bug 765861 is the bug I was thinking of.  Xidorn, mind tweaking the patch to soften the assertion to NS_WARN_IF_FALSE, and add a comment mentioning bug 765861?

Something like what I've got here:
http://mxr.mozilla.org/mozilla-central/source/layout/generic/nsFlexContainerFrame.cpp#3379
Flags: needinfo?(jruderman) → needinfo?(quanxunzhen)
(Assignee)

Comment 8

4 years ago
Created attachment 8585868 [details] [diff] [review]
patch
Attachment #8585282 - Attachment is obsolete: true
Flags: needinfo?(quanxunzhen)
Attachment #8585868 - Flags: review?(dholbert)
Comment on attachment 8585868 [details] [diff] [review]
patch

Looks good. Can you include the testcase as a crashtest?

r=me with that
Attachment #8585868 - Flags: review?(dholbert) → review+
https://hg.mozilla.org/mozilla-central/rev/49d1782a9ad5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
status-firefox40: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.