552334 - Negative z-index broken when an element with a very high z-index is added to a page

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2) Gecko/20100115 Firefox/3.6
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.1; en-GB; rv:1.9.2) Gecko/20100115 Firefox/3.6

If a div has a negative z-index and another div on the page has a very high z-index then the div with the negative z-index is moved to the front.

If (abs(negative z-index) + another divs z-index) >= 2147483649 then the div with the negative z-index is brought into the foreground


Reproducible: Always

Steps to Reproduce:
Load the test case and click the button.

Comment 1

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Attachment: Test case, load and click the button

Comment 2

[Ria Klaassen (not reading all bugmail)]

Seems a regression from Firefox 3.5.

Comment 3

[Boris Zbarsky [:bzbarsky]]

roc, any idea what's up here?

Note that 2147483649 == 0x80000001 so what's happening here is that the difference of the z-indices overflows a signed 32-bit integer.  Since we look at the difference when sorting by z-index, what's happening is that the difference ends up looking like a negative number instead of a very large positive one.  See the IsZOrderLEQ function at http://hg.mozilla.org/mozilla-central/file/e26e8db7e8e7/layout/base/nsDisplayList.cpp#l987

That said, this code hasn't changed in a good long while.... I wonder why anything changed at all from 3.5.

Also note that with a tiny bit more effort on the part of the testcase you could just overflow 32-bit signed integers directly in the z-index literals, and then things will just break period (probably in all browsers).

Comment 4

[Boris Zbarsky [:bzbarsky]]

And the testcase fails for me in 3.5, as expected.  Ria, are you sure about comment 2?

Clearly the first sentence of comment 3 should be ignored, by the way.  ;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

To be clear, we can make IsZOrderLEQ work for all cases when neither z-index value overflows 32-bit signed ints.  The question is whether it matters in practice, since the 32-bit restriction will still be there.  The current code works for all z-index values that fit in 31-bit signed ints....

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We could store the z-index as a float internally.

But it probably makes more sense to clamp z-index values to valid PRInt32s in the style system, and make IsZOrderLEQ work for all PRInt32 values. Make sense?

Comment 7

[Boris Zbarsky [:bzbarsky]]

> makes more sense to clamp z-index values to valid PRInt32s in the style system

Already done.  Numeric values of z-index have to be something that tokenized as an integer, and the !gotDot case in nsCSSScanner::ParseNumber makes sure that the integer I produced satisfies abs(I) <= PR_INT32_MAX.

> and make IsZOrderLEQ work for all PRInt32 values.

Sounds good to me.  Do you want to patch or review?  ;)

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Review!

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: Here we go

Comment 10

[Boris Zbarsky [:bzbarsky]]

Pushed http://hg.mozilla.org/mozilla-central/rev/dd135c502a95

Comment 11

[Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe]]

Tests fine on the the latest nightly.

Thanks guys.