Closed
Bug 315783
Opened 18 years ago
Closed 18 years ago
Crash on some E4X in GC_MARK_DEBUG builds
Categories
(Core :: JavaScript Engine, defect, P1)
Core
JavaScript Engine
Tracking
()
VERIFIED
FIXED
mozilla1.9alpha1
People
(Reporter: bzbarsky, Assigned: brendan)
References
()
Details
(Keywords: fixed1.8.1, Whiteboard: GC_MARK_DEBUG-only code change to fix)
Attachments
(1 file, 2 obsolete files)
1.62 KB,
patch
|
bzbarsky
:
review+
brendan
:
approval1.8.0.1+
brendan
:
approval1.8.1+
|
Details | Diff | Splinter Review |
See testcase in URL field. The problem is that xml_mark_vector does: #ifdef GC_MARK_DEBUG char buf[100]; JSXMLQName *qn = elt->name; JS_snprintf(buf, sizeof buf, "%s::%s", qn->uri ? JS_GetStringBytes(qn->uri) : "*", JS_GetStringBytes(qn->localName)); #else The problem is that in some cases |elt| is a text node, as I understood brendan. So elt->name is null, and this code crashes. Perhaps we should simply use "#text" for the name when elt is a text node?
![]() |
Reporter | |
Comment 1•18 years ago
|
||
Can we fix this, pretty-please? Crashing while debugging random other stuff sucks. :(
Flags: blocking1.9a1?
Totally untested, I haven't even tried to build it with GC_MARK_DEBUG. Let me know, bz?
Updated•18 years ago
|
Attachment #207692 -
Flags: review+
Comment on attachment 207692 [details] [diff] [review] use #text if no qname Requesting branch approval: zero change for release builds, but fixes a crash when using leak-tracking GC_MARK_DEBUG tool.
Attachment #207692 -
Flags: approval1.8.1?
Attachment #207692 -
Flags: approval1.8.0.1?
Assignee | ||
Comment 4•18 years ago
|
||
Sorry, lost track of this bug. If the patch works for you (what sites or XUL JS are you hitting that use E4X?), please check it in r=you, or tell me to do it. /be
Attachment #207693 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 5•18 years ago
|
||
Comment on attachment 207692 [details] [diff] [review] use #text if no qname lists and comments are not #text, but this will do too. mrbkap, shaver: take a look at my alterna-patch. No need to request approval for NPOTB stuff. /be
Attachment #207692 -
Flags: approval1.8.1?
Attachment #207692 -
Flags: approval1.8.0.1?
Assignee | ||
Comment 6•18 years ago
|
||
This'll get fixed quickly. /be
Assignee: general → brendan
Flags: blocking1.9a1?
Comment on attachment 207693 [details] [diff] [review] fix Yeah, much better. Let's do this on the branches instead of my half-assed null-check.
Attachment #207693 -
Flags: superreview+
Attachment #207693 -
Flags: approval1.8.1?
Attachment #207693 -
Flags: approval1.8.0.1?
Comment on attachment 207693 [details] [diff] [review] fix NPOTB, natch. Marking r- based on bz's test result as well, but preserving my sr+ of failure for the record!
Attachment #207693 -
Flags: review?(bzbarsky)
Attachment #207693 -
Flags: review-
Attachment #207693 -
Flags: approval1.8.1?
Attachment #207693 -
Flags: approval1.8.0.1?
Assignee | ||
Comment 9•18 years ago
|
||
Attachment #207692 -
Attachment is obsolete: true
Attachment #207693 -
Attachment is obsolete: true
Attachment #207695 -
Flags: review?(bzbarsky)
![]() |
Reporter | |
Comment 10•18 years ago
|
||
Comment on attachment 207695 [details] [diff] [review] fix, v2 r=bzbarsky. People are posting testcases to layout and DOM bugs that use E4X... ;)
Attachment #207695 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 11•18 years ago
|
||
This is a GC_MARK_DEBUG only change, needed at the limit by anyone investigating memory leaks. /be
Status: NEW → ASSIGNED
Flags: blocking1.8.1+
Flags: blocking1.8.0.1+
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Whiteboard: GC_MARK_DEBUG-only code change to fix
Target Milestone: --- → mozilla1.9alpha
Assignee | ||
Updated•18 years ago
|
Attachment #207695 -
Flags: approval1.8.1+
Attachment #207695 -
Flags: approval1.8.0.1+
Assignee | ||
Comment 12•18 years ago
|
||
Fixed everywhere. /be
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Keywords: fixed1.8.0.1,
fixed1.8.1
Resolution: --- → FIXED
Comment 13•18 years ago
|
||
(In reply to comment #10) > r=bzbarsky. People are posting testcases to layout and DOM bugs that use > E4X... ;) Sorry about that, when I wrote it I didn't knew it would crash your build. I assume now that this bug is fixed it's safe to use E4X in testcases?
Updated•18 years ago
|
Keywords: fixed1.8.0.1,
fixed1.8.1
Updated•18 years ago
|
Keywords: fixed1.8.0.1,
fixed1.8.1
Updated•18 years ago
|
Flags: testcase-
![]() |
Reporter | |
Comment 15•18 years ago
|
||
So... this doesn't actually build on the 1.8 branch, because js_DeflateStringToBuffer only takes 3 args, not 5, there. I've hacked it locally to sorta work, but I guess the real issue is that the UTF8 changes are not on that branch?
Keywords: verified1.8.0.1,
verified1.8.1
Comment 16•18 years ago
|
||
bug 232182 is only on the trunk atm although I've nominated it for 1.8
Assignee | ||
Comment 17•18 years ago
|
||
Why are people backporting individual patches from the trunk? JS1.7 is coming and it contains all of these changes. Hand-merging patches in random order makes extra work, versionitis, and bug habitat. It has high opportunity cost too (bz, you should be doing something else, and I mean that in the best way ;-). /be
![]() |
Reporter | |
Comment 18•18 years ago
|
||
Hey, the only reason I cared about this was because it was blocking me working on other things. ;) See comment 1. I still have it hacked locally so I can debug leaks on the branch if I need to...
Comment 19•18 years ago
|
||
Minusing as a blocker, but the approvals still stand. Please do check in to the branch, but we're not going to bug you about it every triage cycle anymore.
Flags: blocking1.8.1+ → blocking1.8.1-
You need to log in
before you can comment on or make changes to this bug.
Description
•