Closed Bug 1146114 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect)

x86_64
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla40
Tracking Status
firefox40 --- fixed

People

(Reporter: jruderman, Assigned: xidorn)

References

(Blocks 1 open bug)

Details

(Keywords: assertion, testcase)

Attachments

(3 files, 1 obsolete file)

Attached file 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
Attached file stack
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.
Attached patch patch (obsolete) — Splinter Review
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)
(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)
Attached patch patchSplinter Review
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
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla40
You need to log in before you can comment on or make changes to this bug.