Closed Bug 315783 Opened 16 years ago Closed 15 years ago

Crash on some E4X in GC_MARK_DEBUG builds

Categories

(Core :: JavaScript Engine, defect, P1)

defect

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)

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?
Can we fix this, pretty-please?  Crashing while debugging random other stuff sucks.  :(
Flags: blocking1.9a1?
Attached patch use #text if no qname (obsolete) — Splinter Review
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.
Attachment #207692 - Flags: approval1.8.1?
Attachment #207692 - Flags: approval1.8.0.1?
Attached patch fix (obsolete) — Splinter Review
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)
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?
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?
Attached patch fix, v2Splinter Review
Attachment #207692 - Attachment is obsolete: true
Attachment #207693 - Attachment is obsolete: true
Attachment #207695 - Flags: review?(bzbarsky)
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
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
Attachment #207695 - Flags: approval1.8.1+
Attachment #207695 - Flags: approval1.8.0.1+
Fixed everywhere.

/be
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
(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, 1.8.0.1
Status: RESOLVED → VERIFIED
Flags: testcase-
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
Keywords: fixed1.8.1
You need to log in before you can comment on or make changes to this bug.