jsxml.c needs to root better during QName creation

RESOLVED FIXED

Status

()

Core
JavaScript Engine
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: dbaron, Assigned: brendan)

Tracking

({fixed1.8.0.4, fixed1.8.1})

Trunk
x86
Linux
fixed1.8.0.4, fixed1.8.1
Points:
---
Bug Flags:
blocking1.8.1 ?
blocking1.8.0.4 +
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

11 years ago
I have an E4X testcase that I just ran under WAY_TOO_MUCH_GC, and hit a few GC hazards in jsxml.c.  The first one's obvious (js_NewObject can do last-ditch allocation), and the stack where the second one can cause GC is:

js_GC (/home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:2148)
js_NewGCThing (/home/dbaron/builds/trunk/mozilla/js/src/jsgc.c:668)
js_NewObject (/home/dbaron/builds/trunk/mozilla/js/src/jsobj.c:2021)
NewXMLObject (/home/dbaron/builds/trunk/mozilla/js/src/jsxml.c:7338)
js_GetXMLObject (/home/dbaron/builds/trunk/mozilla/js/src/jsxml.c:7373)
GetProperty (/home/dbaron/builds/trunk/mozilla/js/src/jsxml.c:4062)
ToAttributeName

I'll attach a patch; the first chunk is rather odd.  Perhaps it should be done with locking instead.
(Reporter)

Comment 1

11 years ago
Ignore the "ToAttributeName" at the bottom of that stack.
(Reporter)

Updated

11 years ago
Status: NEW → ASSIGNED
Flags: blocking1.8.1?
Flags: blocking1.8.0.3?
Whiteboard: [patch]
(Reporter)

Comment 2

11 years ago
Created attachment 216199 [details] [diff] [review]
patch
Attachment #216199 - Flags: review?(brendan)
(Reporter)

Comment 3

11 years ago
Note that I only saw one of the last two, but the other looked too similar not to patch the same way.
(Assignee)

Comment 4

11 years ago
Created attachment 216238 [details] [diff] [review]
use a local root if possible, plus style nits

Thanks dbaron, I'll take this on if you don't mind since it's all my fault.

Nits: /* Major comments use sentences and have one blank line in front. */; s/JSBool result/JSBool ok/.

/be
Assignee: dbaron → brendan
Attachment #216199 - Attachment is obsolete: true
Attachment #216238 - Flags: review?(mrbkap)
Attachment #216238 - Flags: approval1.8.0.3?
Attachment #216238 - Flags: approval-branch-1.8.1+
Attachment #216199 - Flags: review?(brendan)

Updated

11 years ago
Attachment #216238 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 5

11 years ago
Fixed on trunk and 1.8 branch.

Bob, I forgot to link the last jsxml.c-patching bug up to js1.6rc1, but I don't know where rc1 stands.  Neither is a blocker for rc1, but both could go into js1.6 and do the world some favors.  Comments?

/be
Blocks: 309169
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Keywords: fixed1.8.1
Resolution: --- → FIXED

Comment 6

11 years ago
(In reply to comment #5)

I'll get them in. I will grab the patches from the latest additions today, confirm they build then cut the branch later today. I have been remiss in getting rc1 out the door but will push hard today.
Flags: blocking1.8.0.3? → blocking1.8.0.3+
Comment on attachment 216238 [details] [diff] [review]
use a local root if possible, plus style nits

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #216238 - Flags: approval1.8.0.3? → approval1.8.0.3+
(Assignee)

Comment 8

11 years ago
Fixed on 1.8.0 branch.

/be
Keywords: fixed1.8.0.3

Comment 9

11 years ago
David, do you have that e4x test case available?

Updated

11 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.