301574 - Bogus error message when calling XML constructor with E4X disabled

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Wladimir Palant]

Try the following simple code:

var xml = XML('<xml/>');

On an HTML page (E4X disabled by default) it will produce an exception: "invalid
XML markup". This is obviously not the case, same code works fine with E4X
enabled. I see two solutions. One would be changing the error message to
describe the problem properly. The other would be allowing the XML constructor
even if E4X is not enabled. I would actually prefer the latter, as there is no
reason to forbid the explicit XML constructor - is doesn't suffer from backwards
compatibility issues like the XML initialiser.

Comment 1

[Wladimir Palant]

Attachment: Testcase

This testcase currently displays the message: "Got an exception: SyntaxError:
invalid XML markup". Instead it should either say "XML object created" (XML
constructor works despite E4X initialisers being deactivated) or display some
proper error message.

Comment 2

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: throw an error

Something like this would fix this, I think.

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 190068 [details] [diff] [review]
throw an error

Better fix coming.

Comment 4

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: enable XML parsing for the XML constructor

Comment 5

[Brendan Eich [:brendan]]

Comment on attachment 190144 [details] [diff] [review]
enable XML parsing for the XML constructor

Style nites only:

> #if JS_HAS_XML_SUPPORT
>-        if (JS_HAS_XML_OPTION(cx) &&
>-            pn2->pn_type == TOK_DOT &&
>+        if (pn2->pn_type == TOK_DOT &&
>             pn2->pn_op != JSOP_GETMETHOD) {

Pull that second operand of && up onto the same line, so it doesn't feel all
depressed and draggy, ok?

> static JSBool
> XML(JSContext *cx, JSObject *obj, uintN argc, jsval *argv, jsval *rval)
> {
>     jsval v;
>     JSXML *xml, *copy;
>     JSObject *xobj, *vobj;
>     JSClass *clasp;
>+    uint32 oldopts;

Put right after v's decl, to match order of use/set?

>     v = argv[0];
>     if (JSVAL_IS_NULL(v) || JSVAL_IS_VOID(v))
>         v = STRING_TO_JSVAL(cx->runtime->emptyString);
> 
>+    /* Toggle on XML support since the script has explicitly requested it. */
>+    oldopts = cx->options;
>+    JS_SetOptions(cx, oldopts | JSOPTION_XML);
>+
>     xobj = ToXML(cx, v);
>+
>+    JS_SetOptions(cx, oldopts);
>+
>     if (!xobj)
>         return JS_FALSE;

Too much vertical space is as bad as too little.  I'd crunch at least the if
(!xobj) with the JS_SetOptions call before it, and perhaps move the empty line
to after the return JS_FALSE.  Tradition would crunch that way *and* eliminate
the blank lines around the xobj = ToXML(cx, v), but maybe it's time for a
change.

>     *rval = OBJECT_TO_JSVAL(xobj);
>     xml = (JSXML *) JS_GetPrivate(cx, xobj);
> 
>     if ((cx->fp->flags & JSFRAME_CONSTRUCTING) && !JSVAL_IS_PRIMITIVE(v)) {
>         vobj = JSVAL_TO_OBJECT(v);
>         clasp = OBJ_GET_CLASS(cx, vobj);
>         if (clasp == &js_XMLClass ||
>             (clasp->flags & JSCLASS_DOCUMENT_OBSERVER)) {

r+a=me with those nits picked, thanks.

/be

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked in.

Comment 7

[Bob Clary [:bc] (inactive)]

Checking in regress-301574.js;
/cvsroot/mozilla/js/tests/js1_6/Regress/regress-301574.js,v  <--  regress-301574.js
initial revision: 1.1
done

Comment 8

[Bob Clary [:bc] (inactive)]

verified fixed 1.8.x and trunk.