Closed Bug 622060 Opened 14 years ago Closed 14 years ago

jsreftest.html?test=ecma_3/Number/15.7.4.3-02.js | application timed out after 330 seconds with no output

Categories

(Core :: JavaScript Engine, defect)

Product:
Core
Component:
JavaScript Engine
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
blocker

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: philor, Assigned: billm)

References

Details

(Keywords: intermittent-failure, Whiteboard: [test which aborts the suite][fixed-in-tracemonkey])

Attachments

(1 file)

fix
820 bytes, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
Started on the merge from mozilla-central, and so far has been every time (all two of them) on Windows debug. I think Waldo said something about this test earlier today, so he may already know what's up.

http://tinderbox.mozilla.org/showlog.cgi?log=TraceMonkey/1293688596.1293691268.1566.gz
Rev3 WINNT 6.1 tracemonkey debug test jsreftest on 2010/12/29 21:56:36
s: talos-r3-w7-015

REFTEST TEST-START | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma_3/Number/15.7.4.3-02.js
++DOMWINDOW == 29 (125BB918) [serial = 2326] [outer = 046D2118]
BUGNUMBER: 446494
STATUS: num.toLocaleString should handle exponents
TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma_3/Number/15.7.4.3-02.js | application timed out after 330 seconds with no output
What I know about it is that it's perma-fail with jstests.py with the JS shell.  I don't know anything about browser failures offhand.
This looks like some sort of buffer overflow in num_toLocaleString. I get Valgrind warnings in Linux.
Assignee: general → wmccloskey
Attached patch fix — — Splinter Review 
This fixes the valgrind issue. I haven't tested it in Windows, but I suspect it's the same problem.

In order to make sure this fix is right, here are my assumptions. This is where the problem happens:

    buflen = digits + (*nint ? strlen(nint + 1) : 0);
    if (*nint == '.')
        buflen += decimalLength;

I'm assuming that it does strlen(nint+1) because it wants to skip the decimal point, since that length is added on the third line. However, when we run with the input '1e-10', there is no decimal point, so we allocate a buffer that's 1 byte too small.

If there's some other reason for doing strlen(nint+1), then my fix is wrong. However, it passes jit-tests and jstests, and it works on the example in bug 614931.

As an aside, is there any way we can make this code safer? There are all sorts of places it could go wrong through off-by-one errors and the like. Is there any possibility of using a C++ string object, or would that be too slow?
Attachment #500402 - Flags: review?(bzbarsky)
Comment on attachment 500402 [details] [diff] [review]
fix

Yeah, this looks right.  Can you add a test?
Attachment #500402 - Flags: review?(bzbarsky) → review+
But because it is (and remains, I just stopped tbplbot-spamming so you could hear yourselves think) permaorange and stops the run, so we haven't actually run the rest of the jsreftests on debug Windows even once since the merge landed, can you land the fix first, and then think about whether you can add a test?
Severity: normal → blocker
Whiteboard: [orange] → [orange][test which aborts the suite]
http://hg.mozilla.org/tracemonkey/rev/e0fc487c23f4

Strangely, this test was already failing on my machine. I couldn't think of any more tests to add. Instead, I added assertions that should fire if we ever try to write beyond the end of the buffer.
Whiteboard: [orange][test which aborts the suite] → [orange][test which aborts the suite][fixed-in-tracemonkey]
http://hg.mozilla.org/mozilla-central/rev/e0fc487c23f4
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Whiteboard: [orange][test which aborts the suite][fixed-in-tracemonkey] → [test which aborts the suite][fixed-in-tracemonkey]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: