Last Comment Bug 324688 - reports that XML.ignoreWhitespace is true but acts as if it were false, in a XPCOM callback
: reports that XML.ignoreWhitespace is true but acts as if it were false, in a ...
: testcase, verified1.8.0.2, verified1.8.1
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: x86 Windows XP
P3 normal (vote)
: mozilla1.9alpha1
Assigned To: Blake Kaplan (:mrbkap)
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2006-01-25 11:26 PST by Nickolay_Ponomarev
Modified: 2007-04-30 10:12 PDT (History)
5 users (show)
brendan: blocking1.8.1+
dveditz: blocking1.8.0.2+
bob: in‑testsuite+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

testcase (1.02 KB, application/vnd.mozilla.xul+xml)
2006-01-25 11:27 PST, Nickolay_Ponomarev
no flags Details
Proposed fix (2.78 KB, patch)
2006-01-30 12:04 PST, Blake Kaplan (:mrbkap)
no flags Details | Diff | Splinter Review
Better fix (4.27 KB, patch)
2006-01-30 13:18 PST, Blake Kaplan (:mrbkap)
brendan: review-
Details | Diff | Splinter Review
Best fix (4.17 KB, patch)
2006-01-30 14:43 PST, Blake Kaplan (:mrbkap)
brendan: review+
Details | Diff | Splinter Review
What I checked in (4.54 KB, patch)
2006-01-31 16:26 PST, Blake Kaplan (:mrbkap)
brendan: review+
brendan: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review

Description User image Nickolay_Ponomarev 2006-01-25 11:26:35 PST
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:

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 User image Nickolay_Ponomarev 2006-01-25 11:27:30 PST
Created attachment 209603 [details]
Comment 2 User image Brendan Eich [:brendan] 2006-01-25 14:20:24 PST 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?

Comment 3 User image Nickolay_Ponomarev 2006-01-25 14:51:40 PST
sorry, this is the if I was talking about:
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
js> new XML("<a/> ")

js> XML.ignoreWhitespace=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 User image Brendan Eich [:brendan] 2006-01-25 15:12:25 PST
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?

Comment 5 User image Nickolay_Ponomarev 2006-01-25 16:17:59 PST
> 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 User image Blake Kaplan (:mrbkap) 2006-01-30 12:04:03 PST
Created attachment 210163 [details] [diff] [review]
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 User image Blake Kaplan (:mrbkap) 2006-01-30 13:18:05 PST
Created attachment 210171 [details] [diff] [review]
Better fix

Brendan instructed me in the proper way to make getters work correctly.
Comment 8 User image Brendan Eich [:brendan] 2006-01-30 13:48:06 PST
Comment on attachment 210171 [details] [diff] [review]
Better fix

> xml_setting_setter(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
> {
>     JSBool b;
>     uint8 flag;
>     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;
>+    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.

Comment 9 User image Blake Kaplan (:mrbkap) 2006-01-30 14:43:52 PST
Created attachment 210181 [details] [diff] [review]
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 User image Brendan Eich [:brendan] 2006-01-30 16:56:11 PST
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.

Comment 11 User image Blake Kaplan (:mrbkap) 2006-01-31 16:26:33 PST
Created attachment 210298 [details] [diff] [review]
What I checked in
Comment 12 User image Blake Kaplan (:mrbkap) 2006-01-31 16:27:40 PST
Fix checked into trunk.
Comment 13 User image Brendan Eich [:brendan] 2006-01-31 16:31:05 PST
Comment on attachment 210298 [details] [diff] [review]
What I checked in

Thanks, nominating for and plusing for 1.8.1.

Comment 14 User image Brendan Eich [:brendan] 2006-01-31 16:32:25 PST
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

Comment 15 User image Bob Clary [:bc:] 2006-02-13 12:20:32 PST
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 User image Daniel Veditz [:dveditz] 2006-02-16 12:48:54 PST
Comment on attachment 210298 [details] [diff] [review]
What I checked in

approved for 1.8.0 branch, a=dveditz for drivers
Comment 17 User image Blake Kaplan (:mrbkap) 2006-02-22 15:49:43 PST
Fix checked into the 1.8 branches.
Comment 18 User image Bob Clary [:bc:] 2006-03-02 11:58:05 PST
v ff 20060302 win/linux/mac
Comment 19 User image Bob Clary [:bc:] 2006-12-22 06:38:55 PST
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 User image Bob Clary [:bc:] 2007-04-30 10:12:06 PDT
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

Note You need to log in before you can comment on or make changes to this bug.