Closed Bug 357392 Opened 13 years ago Closed 13 years ago

jsdtoa.c - locks not released in some error cases

Categories

(Core :: JavaScript Engine, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: gavin.reaney, Assigned: gavin.reaney)

References

Details

(Keywords: fixed1.8.0.10, fixed1.8.1.1, Whiteboard: [need testcase])

Attachments

(1 file)

JS_strtod doesn't release the dtoa lock if the function exits via the 'nomem' label. Similar problems exist in JS_dtobasestr. This could lead to a hang in OOM conditions.
Attached patch Proposed fixSplinter Review
Added calls to release the lock. Also fixed a few cases where word0(rv) = x was being used instead of set_word0(rv, x)
Please ask Brendan for a review.
Comment on attachment 242864 [details] [diff] [review]
Proposed fix

Ready for review now (was just double checking the code first etc).
Attachment #242864 - Flags: review?(brendan)
Assignee: general → gavin
Comment on attachment 242864 [details] [diff] [review]
Proposed fix

r=me, thanks.

/be
Attachment #242864 - Flags: review?(brendan)
Attachment #242864 - Flags: review+
Attachment #242864 - Flags: approval1.8.1.1?
Blocks: js1.7src
Flags: blocking1.8.1.1?
will take on branch, doesn't immediately seem to be a blocker
Flags: wanted1.8.1.x+
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1-
Comment on attachment 242864 [details] [diff] [review]
Proposed fix

a=mconnor on behalf of drivers for 1.8.1.1 checkin
Attachment #242864 - Flags: approval1.8.1.1? → approval1.8.1.1+
Fixed on trunk and 1.8 branch:

Checking in jsdtoa.c;
/cvsroot/mozilla/js/src/jsdtoa.c,v  <--  jsdtoa.c
new revision: 3.38; previous revision: 3.37
done

Checking in jsdtoa.c;
/cvsroot/mozilla/js/src/jsdtoa.c,v  <--  jsdtoa.c
new revision: 3.33.2.2; previous revision: 3.33.2.1
done

/be
Status: NEW → RESOLVED
Closed: 13 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
Flags: in-testsuite-
Flags: wanted1.8.1.x+
Blocks: 351739
Attachment #242864 - Flags: approval1.8.1.2?
The bug exists on 1.8.0 branch as is.
Flags: blocking1.8.1.2?
Comment on attachment 242864 [details] [diff] [review]
Proposed fix

Igor:  Did you mean to ask for 1.8.0.10 approval?  Just want to make sure before approving this patch.  Let us know.  Thanks!
Attachment #242864 - Flags: approval1.8.1.2? → approval1.8.0.10?
Comment on attachment 242864 [details] [diff] [review]
Proposed fix

The patch for the bug applies as-is for 1.8.0 branch.
Comment on attachment 242864 [details] [diff] [review]
Proposed fix

Approved for 1.8.0 branch, a=jay for drivers.  Igor says this patch can land there as is.  Thanks!
Attachment #242864 - Flags: approval1.8.0.10? → approval1.8.0.10+
Whiteboard: [checkin needed (1.8.0 branch)]
I committed the patch from comment 1 to MOZILLA_1_8_0_BRANCH:

Checking in jsdtoa.c;
/cvsroot/mozilla/js/src/jsdtoa.c,v  <--  jsdtoa.c
new revision: 3.33.10.2; previous revision: 3.33.10.1
done
Keywords: fixed1.8.0.10
Whiteboard: [checkin needed (1.8.0 branch)]
Flags: blocking1.8.1.2? → blocking1.8.0.10+
Whiteboard: [need testcase]
gavin, can you verify this fix against the 1_8_0 branch?  It sounds like we could use a developer's to check this unless you know of a easy way for QA to verify.  Thanks.
(In reply to comment #13)
> gavin, can you verify this fix against the 1_8_0 branch?  It sounds like we
> could use a developer's to check this unless you know of a easy way for QA to
> verify.  Thanks.
> 

I've run a build from the 1.8.0 branch and stepped through the code in the affected functions using the visual studio debugger. Locks are correctly released when handling error conditions.
You need to log in before you can comment on or make changes to this bug.