Closed
Bug 315783
Opened 19 years ago
Closed 19 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•19 years ago
|
||
Can we fix this, pretty-please? Crashing while debugging random other stuff sucks. :(
Flags: blocking1.9a1?
Comment 2•19 years ago
|
||
Totally untested, I haven't even tried to build it with GC_MARK_DEBUG. Let me know, bz?
Updated•19 years ago
|
Attachment #207692 -
Flags: review+
Comment 3•19 years ago
|
||
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•19 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•19 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•19 years ago
|
||
This'll get fixed quickly.
/be
Assignee: general → brendan
Flags: blocking1.9a1?
Comment 7•19 years ago
|
||
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 8•19 years ago
|
||
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•19 years ago
|
||
Attachment #207692 -
Attachment is obsolete: true
Attachment #207693 -
Attachment is obsolete: true
Attachment #207695 -
Flags: review?(bzbarsky)
![]() |
Reporter | |
Comment 10•19 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•19 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•19 years ago
|
Attachment #207695 -
Flags: approval1.8.1+
Attachment #207695 -
Flags: approval1.8.0.1+
Assignee | ||
Comment 12•19 years ago
|
||
Fixed everywhere.
/be
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Keywords: fixed1.8.0.1,
fixed1.8.1
Resolution: --- → FIXED
Comment 13•19 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•19 years ago
|
Keywords: fixed1.8.0.1,
fixed1.8.1
Updated•19 years ago
|
Keywords: fixed1.8.0.1,
fixed1.8.1
Updated•19 years ago
|
Flags: testcase-
![]() |
Reporter | |
Comment 15•19 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•19 years ago
|
||
bug 232182 is only on the trunk atm although I've nominated it for 1.8
Assignee | ||
Comment 17•19 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•19 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...
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
•