The default bug view has changed. See this FAQ.

jsdtoa.c - locks not released in some error cases

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: Gavin Reaney, Assigned: Gavin Reaney)

Tracking

({fixed1.8.0.10, fixed1.8.1.1})

Trunk
x86
Windows XP
fixed1.8.0.10, fixed1.8.1.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.8.1.1 -
blocking1.8.0.10 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [need testcase])

Attachments

(1 attachment)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

11 years ago
Created attachment 242864 [details] [diff] [review]
Proposed fix

Added calls to release the lock. Also fixed a few cases where word0(rv) = x was being used instead of set_word0(rv, x)

Comment 2

11 years ago
Please ask Brendan for a review.
(Assignee)

Comment 3

11 years ago
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)

Updated

11 years ago
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?

Updated

11 years ago
Blocks: 355044
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
Last Resolved: 11 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite-
Flags: wanted1.8.1.x+

Updated

10 years ago
Blocks: 351739

Updated

10 years ago
Attachment #242864 - Flags: approval1.8.1.2?

Comment 8

10 years ago
The bug exists on 1.8.0 branch as is.
Flags: blocking1.8.1.2?

Comment 9

10 years ago
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 10

10 years ago
Comment on attachment 242864 [details] [diff] [review]
Proposed fix

The patch for the bug applies as-is for 1.8.0 branch.

Comment 11

10 years ago
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)]

Comment 12

10 years ago
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

Updated

10 years ago
Whiteboard: [checkin needed (1.8.0 branch)]
Flags: blocking1.8.1.2? → blocking1.8.0.10+

Updated

10 years ago
Whiteboard: [need testcase]

Comment 13

10 years ago
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.
(Assignee)

Comment 14

10 years ago
(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.