474016 - Fix rounding issue in transform mochitest "test_bug435293-skew.html" on Linux tinderbox

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[cmtalbert]

The CSS Transform tests seem to leak very badly on windows.  They have been disabled now, but we need to understand this and fix the leaks.

Marking blocking ? because this should be fixed before we ship 3.1 as the css transform code landed in 3.1 already.

Comment 1

[cmtalbert]

Also, a note to self.  Before repushing these tests, change the skew tests so that they pass with a margin of error instead of relying on exact values that might change due to rounding.

Comment 2

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

Was there info in the log on which objects leaked?

Comment 3

[Daniel Holbert [:dholbert]]

Yes. Log here:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1232132249.1232136279.5912.gz

However, the next 4 cycles after the leaky one (ignoring the NSS build-failure cycle) all ended up being green, with no leaks, and with the transform tests still included.

So, I tend to think the transform tests weren't responsible after all, and these leaks were a sporadic failure... dbaron, what do you think?

Comment 4

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

Sounds like it.

But you do need to fix the failure due to rounding that showed up on Linux.

Comment 5

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

Would be good to get these relanded sooner rather than later just so we can confirm that there's not some random leak here...

Comment 6

[cmtalbert]

Attachment: Fix the rounding issue

Daniel, 

Can you give this a quick once over.  I wasn't able to test this on my windows VM box because it went down for some reason on Tuesday and hasn't come back yet. (bug 474818).  However, since the memory leaks appeared to be random, we are probably ok to have another go at checking this in if it meets with your approval.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 358361 [details] [diff] [review]
Fix the rounding issue

>+  is((+tformValues[0]), 1, "Test2: skewy: param 0 is 1");
>+  ok(verifyRounded(tformValues[1], 1.73205), "Test2: skewy: Rounded param 1 is in bounds");
>+  is((+tformValues[2]), 0, "Test2: skewy: param 2 is 0");

Should be "Rounded param 2" (not 1) there.

Other than that, looks good to me.  Thanks for fixing this!

Comment 8

[Daniel Holbert [:dholbert]]

Comment on attachment 358361 [details] [diff] [review]
Fix the rounding issue

er... sorry, I quoted the wrong spot above. I meant this part:

>+  is(tformValues[0], 1, "Test8: Test skew with negative degrees-param 0 is 1");
>+  ok(verifyRounded(tformValues[1], 3.73206), "Test8: Rounded param 2 is in bounds");
>+  is((+tformValues[2]), -1, "Test8: param 2 is -1");

You mean "Rounded param 1", not "Rounded param 2" in the second line there.

Comment 9

[Daniel Holbert [:dholbert]]

I'd also get rid of "Test skew with negative degrees-" in the first line of the quoted chunk above, just for consistency with what you did in Test 1 and Test 2.

Comment 10

[cmtalbert]

Made those changes and pushed to m-c:
changeset:   24179:ab9665e638d6
tag:         tip
user:        Clint Talbert <[e-mail]>
date:        Sat Jan 24 16:20:14 2009 -0800
summary:     Bug 474016: Fixing rounding errors on css transform mochitests r=dholbert


--> FIXED

Comment 11

[cmtalbert]

Linux tinderbox still fails :( Changes backed out.
--> REOPENED

Comment 12

[Daniel Holbert [:dholbert]]

Log for failing cycle:
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1232843039.1232846678.11674.gz

Comment 13

[Daniel Holbert [:dholbert]]

(In reply to comment #11)
> Linux tinderbox still fails :( Changes backed out.

FWIW, I just tried re-applying that patch with "hg export ab9665e638d6 | patch -p1", and all three re-enabled mochitests pass on two Linux machines that I tested.  Both machines are Dell desktops, one running Ubuntu 8.10 and the other running Ubuntu 9.04 alpha.

Comment 14

[Daniel Holbert [:dholbert]]

Updating bug summary, because the leaks mentioned in the original summary turned out to be known-sporadic by comment 3, and this bug has been about the rounding issue ever since that.

Comment 15

[cmtalbert]

Attachment: The final version of the rounding fix patch

Carrying forward dholbert's review, as this is a small change.

So, for all those who might some day make the same mistake, the issue here is the floating point arithmetic in JS is not exactly what you'd think it is.  We have to add a "toFixed" call to the difference calculation to be sure we only compare the numbers we care about, not the cruft that creeps in due to the IEEE floating point representation of our difference.

The only difference with the previous version of the patch is that the "skew" test's rounded function is changed thus:
function verifyRounded(aVal, aTrueVal) {
  return (Math.abs(aVal - aTrueVal).toFixed(5) <= 0.00001);
}

Comment 16

[cmtalbert]

Pushed 25605:1fd024d66d70.  Crossing fingers for a green.  When I get that, I'll mark this as fixed.

Comment 17

[cmtalbert]

We got green!  --> FIXED