315783 - Crash on some E4X in GC_MARK_DEBUG builds

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

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?

Comment 1

[Boris Zbarsky [:bzbarsky]]

Can we fix this, pretty-please?  Crashing while debugging random other stuff sucks.  :(

Comment 2

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Attachment: use #text if no qname

Totally untested, I haven't even tried to build it with GC_MARK_DEBUG.  Let me know, bz?

Comment 3

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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.

Comment 4

[Brendan Eich [:brendan]]

Attachment: fix

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 5

[Brendan Eich [:brendan]]

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

Comment 6

[Brendan Eich [:brendan]]

This'll get fixed quickly.

/be

Comment 7

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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 8

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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 9

[Brendan Eich [:brendan]]

Attachment: fix, v2

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 207695 [details] [diff] [review]
fix, v2

r=bzbarsky.  People are posting testcases to layout and DOM bugs that use E4X... ;)

Comment 11

[Brendan Eich [:brendan]]

This is a GC_MARK_DEBUG only change, needed at the limit by anyone investigating memory leaks.

/be

Comment 12

[Brendan Eich [:brendan]]

Fixed everywhere.

/be

Comment 13

[Aiko]

(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?

Comment 14

[Bob Clary [:bc] (inactive)]

verified checked in on trunk, 1.8, 1.8.0.1

Comment 15

[Boris Zbarsky [:bzbarsky]]

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?

Comment 16

[Bob Clary [:bc] (inactive)]

bug 232182 is only on the trunk atm although I've nominated it for 1.8

Comment 17

[Brendan Eich [:brendan]]

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

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

[David Baron :dbaron:]

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.

Comment 20

[Brendan Eich [:brendan]]

Fixed on 1.8 branch with js1.7 landing.

/be