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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jesse Ruderman]

Attachment: 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

Comment 1

[Jesse Ruderman]

Attachment: stack

Comment 2

[Xidorn Quan [:xidorn] UTC+10]

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.

Comment 3

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 4

[Daniel Holbert [: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?

Comment 5

[Daniel Holbert [:dholbert]]

(Jesse, do you know the bug I'm thinking of at the end of comment 4?)

Comment 6

[Xidorn Quan [:xidorn] UTC+10]

(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?

Comment 7

[Daniel Holbert [:dholbert]]

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

Comment 8

[Xidorn Quan [:xidorn] UTC+10]

Attachment: patch

Comment 9

[Daniel Holbert [:dholbert]]

Comment on attachment 8585868 [details] [diff] [review]
patch

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

r=me with that

Comment 10

[Xidorn Quan [:xidorn] UTC+10]

https://hg.mozilla.org/integration/mozilla-inbound/rev/49d1782a9ad5

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/49d1782a9ad5