324688 - reports that XML.ignoreWhitespace is true but acts as if it were false, in a XPCOM callback

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P3)

Description

[Nickolay_Ponomarev]

In the to be attached testcase, |new XML("<a></a> ")| in a nsITimer observer-callback throws an exception ("syntax error"). The exception is thrown after checking |length| here:
http://lxr.mozilla.org/seamonkey/source/js/src/jsxml.c#2054

It appears that spidermonkey thinks that XML.ignoreWhitespace is false - if I set it to true manually, everything works fine (as well as when that code is called not from a callback). An interesting fact is that XML.ignoreWhitespace returns true.

To test, open the XUL file from a privileged location (such as chrome://browser/content/). (Non-privileged variant throws permission denied exception in the observe method)

Expected: "before", "after" messages in console.
Actual results: "before", "syntax error".

(The original testcase caused the same exception to occur in onStopRequest method of a content handler implementation)

Comment 1

[Nickolay_Ponomarev]

Attachment: testcase

Comment 2

[Brendan Eich [:brendan]]

http://lxr.mozilla.org/seamonkey/source/js/src/jsxml.c#2054 is no good, what's the CVS rev that line number was valid against, or can you give a new lineno for the top revision as of right now (3.81)?

XML.ignoreWhitespace setting does not (AFAIK) affect whether or not a SyntaxError is thrown.  Seems like something else is going wrong here, to do with callbacks via XPCOM.  Is XPConnect using a different scope object, or something?

/be

Comment 3

[Nickolay_Ponomarev]

sorry, this is the if I was talking about: http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/js/src/jsxml.c&rev=3.81#2051
As you can see if |length| is not 1 or 0, an error is reported. |length| appears to be the number of nodes in the parsed string, which includes whitespace nodes iff XML.ignoreWhitespace != true.

> XML.ignoreWhitespace setting does not (AFAIK) affect whether or not a
> SyntaxError is thrown.
I'm not absolutely sure whether it's supposed to affect that per spec, but it certainly has an effect in current (as of a few days ago) impl. Try this in xpcshell:
js> XML.ignoreWhitespace
true
js> new XML("<a/> ")

js> XML.ignoreWhitespace=false
false
js> new XML("<a/> ")
typein:4: SyntaxError: syntax error

> to do with callbacks via XPCOM. Is XPConnect using a different scope object
It appeared like that to me, but I'm not familiar enough with spidermonkey enough to be sure, which is why I reported it like I did. __parent__ is null, should it be / is it related?

Comment 4

[Brendan Eich [:brendan]]

Two bugs here, one possibly in XPConnect, not JS Engine:

1.  ParseXMLSource should not throw SyntaxError if trailing space and XML.ignoreWhitespace is true (it defaults to true).

2.  Something about the scope of XML literal in nsITimer observer-callback, and the like.  Someone needs to debug that (I'm in the middle of another bug, so mrbkap et al. feel free).  Nickolay, which __parent__ (in which object) was null?

/be

Comment 5

[Nickolay_Ponomarev]

> Nickolay, which __parent__ (in which object) was null?
Nevermind, it was something I remembered from the last night, when mrbkap tried to explain like three times that it was a __parent__ of the global object. Duh.

But even so, the problem here is inconsistency (possibly due to problem #2) - XML.ignoreWhitespace returns true in the callback, but the internal state changes if I set it to true manually - causing the constructor not to throw. (Sorry that I'm repeating myself.)

Comment 6

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Proposed fix

There were two problems here: the first (and most obvious one) being that getting a property didn't actually fill the cache as the comment in GetXMLSettingFlags seemed to imply. While trying to fix that, I found that fun_getProperty wasn't actually allowing XML.ignoreComments to return anything other than the passed-in string (surprise!). I've tried to fix that by making sure that *vp is the default value before overriding it, but this changes the behavior of:
function f(a) { f[0] = "bye"; print(f[0]); } f("hi");

(and, additionally, replacing "bye" with undefined causes us to print "hi" again). This could be fixed by creating a fun_setProperty to deny setting these properties.

Comment 7

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Better fix

Brendan instructed me in the proper way to make getters work correctly.

Comment 8

[Brendan Eich [:brendan]]

Comment on attachment 210171 [details] [diff] [review]
Better fix

> xml_setting_setter(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
> {
>     JSBool b;
>     uint8 flag;
> 
>     JS_ASSERT(JSVAL_IS_INT(id));
>     if (!js_ValueToBoolean(cx, *vp, &b))
>         return JS_FALSE;
>+    *vp = BOOLEAN_TO_JSVAL(b);

Now that I re-read ECMA-357, I see that it specifies no such canonicalization of the value set in any XML settings properties.  This applies to prettyIndent as well as to the boolean ones.

>     {js_prettyIndent_str,       XML_PRETTY_INDENT,     JSPROP_PERMANENT,
>                                 NULL, NULL},

Now I'm scared this could interact badly with fun_getProperty.  It doesn't, but might as well put xml_setting_getter here (NULL for the setter is cool, there's no fun_setProperty so the class-default setter is the stub, which does nothing).

> static JSBool
>+FillSettingsCache(JSContext *cx)
>+{
>+    int i;
>+    const char *name;
>+    jsval v;
>+
>+    /* Pull the settings from the constructor into the cache. */
>+    if (cx->xmlSettingFlags & XSF_CACHE_VALID)
>+        return JS_TRUE;

Nit: extra newline here, for one empty line between paragraphs.

More substantively, Fill sounds like what you do when you miss, yet it contains the validity test here.  So this is really Validate -- but that loses an opportunity to avoid the call out of GetBooleanXMLSetting.  Put the VALID bit test there, and call Fill only when filling.

>+static JSBool
> GetBooleanXMLSetting(JSContext *cx, const char *name, JSBool *bp)
> {
>     int i;
>     jsval v;
> 
>+    if (!FillSettingsCache(cx))
>+        return JS_FALSE;
>     if (cx->xmlSettingFlags & XSF_CACHE_VALID) {
>         for (i = 0; xml_static_props[i].name; i++) {
>             if (!strcmp(xml_static_props[i].name, name)) {
>                 *bp = (cx->xmlSettingFlags & JS_BIT(i)) != 0;
>                 return JS_TRUE;
>             }
>         }
>         *bp = JS_FALSE;
>         return JS_TRUE;
>     }
> 
>-    return GetXMLSetting(cx, name, &v) && js_ValueToBoolean(cx, v, bp);
>+    if (!GetXMLSetting(cx, name, &v))
>+       return JS_FALSE;
>+    JS_ASSERT(JSVAL_IS_BOOLEAN(v));
>+    return JSVAL_TO_BOOLEAN(v);
> }

Based on my re-reading of ECMA-357, I think we want to revert to canonicalizing on Get, not on Set.

Thanks for taking this bug.

/be

Comment 9

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: Best fix

This should be all fixed up. The diff -w isn't much better. The patch got a bit ugly after I realized that there's no point in testing for cache validity after calling FillSettingsCache, since that always fills the cache up to the top.

Comment 10

[Brendan Eich [:brendan]]

Comment on attachment 210181 [details] [diff] [review]
Best fix

>+static JSBool
>+xml_setting_getter(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
>+{
>+    return JS_TRUE;
>+}

Nit: define this before xml_setting_setter for symmetry with initializers of the static JSPropertySpecs, and for canonical order's sake.

>+    /* Note: XML_PRETTY_INDENT is not a boolean setting. */
>+    for (i = XML_IGNORE_COMMENTS; i < XML_PRETTY_INDENT; ++i) {

A pox on your C++ monkey-boy ++i pretensions ;-)!  Use i++ in my kitchen, to match here, for instance:

>+static JSBool
>+GetBooleanXMLSetting(JSContext *cx, const char *name, JSBool *bp)
>+{
>+    int i;
>+
>+    if (!(cx->xmlSettingFlags & XSF_CACHE_VALID) && !FillSettingsCache(cx))
>+        return JS_FALSE;
>+
>+    for (i = 0; xml_static_props[i].name; i++) {

r=me with these nits picked.

/be

Comment 11

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: What I checked in

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into trunk.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 210298 [details] [diff] [review]
What I checked in

Thanks, nominating for 1.8.0.2 and plusing for 1.8.1.

/be

Comment 14

[Brendan Eich [:brendan]]

Not sure blocking means much, but what the heck.  We'd be foolish not to fix this and many other such bugs for 1.8.1, and this particular fix is safe enough for 1.8.0.2.

/be

Comment 15

[Bob Clary [:bc] (inactive)]

Checking in regress-324688.js;
/cvsroot/mozilla/js/tests/e4x/XML/regress-324688.js,v  <--  regress-324688.js
initial revision: 1.1

verified with 2006-02-11 trunk debug build.

Comment 16

[Daniel Veditz [:dveditz]]

Comment on attachment 210298 [details] [diff] [review]
What I checked in

approved for 1.8.0 branch, a=dveditz for drivers

Comment 17

[Blake Kaplan (:mrbkap) (inactive)]

Fix checked into the 1.8 branches.

Comment 18

[Bob Clary [:bc] (inactive)]

v ff 1.8.0.1/1.8/1.9 20060302 win/linux/mac

Comment 19

[Bob Clary [:bc] (inactive)]

modified test to delay execution in the browser until page has loaded. I've been running the tests locally with this for some time.

Checking in regress-324688.js;
/cvsroot/mozilla/js/tests/e4x/XML/regress-324688.js,v  <--  regress-324688.js
new revision: 1.3; previous revision: 1.2

Comment 20

[Bob Clary [:bc] (inactive)]

I missed the ending prolog to tell the automation the async part of the test was complete.

/cvsroot/mozilla/js/tests/e4x/XML/regress-324688.js,v  <--  regress-324688.js
new revision: 1.4; previous revision: 1.3