373875 - Large heights are serialized using exponential notation

Status: RESOLVED WORKSFORME

(Core :: CSS Parsing and Computation, defect, P4)

Description

[Jesse Ruderman]

Attachment: testcase

The markup in the testcase has "18000000px" as the div's height, but when I ask for the style attribute, I get "1.8e+7px".  That doesn't work as input to the CSS parser.

This bug made it difficult to reduce bug 373868, because serializing the DOM made the bug go away.

Comment 1

[Daniel Holbert [:dholbert]]

Diagnosed it with GDB.  Here's the problem:

At nsCSSDeclaration.cpp:464, there's a call to AppendFloat, which receives a float value of 18000000 and appends the string "1.8e+7". So AppendFloat is the culprit.

Drilling down, the AppendFloat implementation is in the file xpcom/string/src/nsStringObsolete.cpp.  There are two implementations of the function -- one that takes a float, and one that takes a double, and they both are simply wrappers to Modified_cnvtf() (defined in the same file), with the double version having higher precision. (the third argument = precison)

One hack-ish fix is to simply cast the float to a double in the call to AppendFloat at nsCSSDeclaration.cpp:464.  This fixes jesse's test case, but it breaks if you use an even larger number, like 18000000000000000px.  It seems like it just comes down to the size of the input number vs. the 3rd argument passed to Modified_cnvtf() (which is a measure of precision).

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: patch

Here's the simple fix, which just makes us use a double rather than a float so that things that fit in 32-bit integers round-trip correctly.  We probably ought to switch to storing integers at some point, but this removes the practical problems with not doing so.

We probably also ought to ensure we never serialize to scientific notation...

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Actually, I should put this on a separate bug; it's really unrelated.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I filed it as bug 470769.

Comment 5

[Kevin Conaway]

Is there a time table to fix this issue? We are running up against this bug because we are using a JS library that is creating a "grid" widget with a scroll
bar.  The library tries to size the scroll bar appropriately based on the
number of entries in the data store.

The library sets the height of the scroll bar widget to be 40671324px which
firefox then converts to 4.06713e+7px

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

No, but it's worth knowing that somebody actually hit it in the real world; it actually somewhat surprises me.

Comment 7

[Kevin Conaway]

Why does that surprise you?

If this won't be fixed, do you have any recommendations for ways to work around this issue?

Comment 8

[Federico De Faveri]

This problem is present also in our project (I think we are using the same grid library). There is a plan to resolve it?

Comment 9

[Jesse Ruderman]

This bug gets in my way pretty often :(

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note that the original testcase no longer shows the bug because we've changed what getAttribute("style") does, but the bug still exists when the serialization code is triggered.

Comment 12

[Jesse Ruderman]

In addition to annoying me, this bug sometimes prevents my DOM fuzzer from making new testcases based on existing crashtests.

Comment 13

[Daniel Holbert [:dholbert]]

I seem to recall hearing about someone -- Anthony Jones, I think? -- speaking about cleaning this up recently.

Comment 14

[Sunil]

Is there an update on this bug?

Moreover, if the value (incorrectly) uses scientific notation, will the value returned will be in 'standard' form? Is that within the scope of this bug?

Comment 15

[Anthony Jones (:ajones, :kentuckyfriedtakahe, :k17e)]

I do not have an update. I expect values that exceed the precision would contine to be represented as strings in e notation.

Comment 16

[Jesse Ruderman]

Sunil, I don't think we will make our CSS parser handle exponential notation as input, because that would be against the spec.

Comment 17

[Sergi Mansilla [:sergi] (Telenor) [PTO]]

I am running into this bug and Firefox doesn't accept the style as valid (assigning to style.height). Is there any known work-around to this?

Comment 18

[Jesse Ruderman]

I guess this is no longer a problem now that bug 964529 is resolved. (As long as you're not exporting the serialized CSS to another browser.)

Comment 19

[Sunil]

Unfortunately in our 'crazy' use case, that's exactly what we do, export it from Firefox to another browser, hence fix 964529 is not sufficient. Will still prefer the returned value to not have scientific notation.

Comment 20

[pnparam]

Is this bug not resolved yet? I got the same issue on the current latest FF 44.0.2 Will add an attachment for testing.

Comment 21

[pnparam]

Attachment: Simple test case for the css large height issue.

Simple test case  for the CSS large height issue which is still happening in Mozilla Firefox 44.0.2 - the latest stable release.

Comment 22

[Steven Vachon]

This issue was created 12 years ago. Please take notice.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

This seems to work, probably fixed by the style system rewrite in FF57?

Comment 24

[Steven Vachon]

Still broken on my FF67.0.2

Comment 25

[Emilio Cobos Álvarez (:emilio)]

What do you see broken? I don't see any exponential serialization in the attached test-cases, could you attach a new one?

Comment 26

[Steven Vachon]

They're rendered as height: 0px

Comment 27

[Emilio Cobos Álvarez (:emilio)]

It's not clear to me how that's related to this bug. That looks like a different bug (bug 1518433 in particular).

This bug looks fixed to me.

Comment 28

[Steven Vachon]

Okay, then yes, this bug is indeed fixed.