Last Comment Bug 331678 - jsxml.c needs to root better during QName creation
: jsxml.c needs to root better during QName creation
Status: RESOLVED FIXED
[patch]
: fixed1.8.0.4, fixed1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Brendan Eich [:brendan]
:
Mentors:
Depends on:
Blocks: js1.6rc1
  Show dependency treegraph
 
Reported: 2006-03-25 02:07 PST by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2006-06-16 19:11 PDT (History)
6 users (show)
dbaron: blocking1.8.1?
dveditz: blocking1.8.0.4+
bob: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (3.45 KB, patch)
2006-03-25 02:11 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Splinter Review
use a local root if possible, plus style nits (2.75 KB, patch)
2006-03-25 12:02 PST, Brendan Eich [:brendan]
mrbkap: review+
brendan: approval‑branch‑1.8.1+
dveditz: approval1.8.0.4+
Details | Diff | Splinter Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-03-25 02:07:45 PST
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.
Comment 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-03-25 02:09:59 PST
Ignore the "ToAttributeName" at the bottom of that stack.
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-03-25 02:11:20 PST
Created attachment 216199 [details] [diff] [review]
patch
Comment 3 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2006-03-25 02:12:52 PST
Note that I only saw one of the last two, but the other looked too similar not to patch the same way.
Comment 4 Brendan Eich [:brendan] 2006-03-25 12:02:04 PST
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
Comment 5 Brendan Eich [:brendan] 2006-03-25 13:54:14 PST
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
Comment 6 Bob Clary [:bc:] 2006-03-26 08:57:08 PST
(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.
Comment 7 Daniel Veditz [:dveditz] 2006-04-03 12:19:10 PDT
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
Comment 8 Brendan Eich [:brendan] 2006-04-22 23:19:28 PDT
Fixed on 1.8.0 branch.

/be
Comment 9 Bob Clary [:bc:] 2006-05-22 21:38:43 PDT
David, do you have that e4x test case available?

Note You need to log in before you can comment on or make changes to this bug.