Closed Bug 315783 Opened 18 years ago Closed 18 years ago
Crash on some E4X in GC
_MARK _DEBUG builds
See testcase in URL field. The problem is that xml_mark_vector does: #ifdef GC_MARK_DEBUG char buf; 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?
Can we fix this, pretty-please? Crashing while debugging random other stuff sucks. :(
Totally untested, I haven't even tried to build it with GC_MARK_DEBUG. Let me know, bz?
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.
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
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
This'll get fixed quickly. /be
Assignee: general → brendan
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.
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!
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+
This is a GC_MARK_DEBUG only change, needed at the limit by anyone investigating memory leaks. /be
Status: NEW → ASSIGNED
OS: Linux → All
Priority: -- → P1
Hardware: PC → All
Whiteboard: GC_MARK_DEBUG-only code change to fix
Target Milestone: --- → mozilla1.9alpha
Fixed everywhere. /be
(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?
verified checked in on trunk, 1.8, 188.8.131.52
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?
bug 232182 is only on the trunk atm although I've nominated it for 1.8
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
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...
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-
Fixed on 1.8 branch with js1.7 landing. /be
You need to log in before you can comment on or make changes to this bug.