Closed Bug 324688 Opened 14 years ago Closed 14 years ago

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


(Core :: JavaScript Engine, defect, P3)

Windows XP





(Reporter: asqueella, Assigned: mrbkap)


(Keywords: testcase, verified1.8.0.2, verified1.8.1, Whiteboard: [patch][rft-dl])


(2 files, 3 obsolete files)

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)
Attached file testcase 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?

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

> 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.)
Attached patch Proposed fix (obsolete) — Splinter Review
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.
Assignee: general → mrbkap
Attachment #210163 - Flags: review?(brendan)
Priority: -- → P3
Target Milestone: --- → mozilla1.9alpha
Whiteboard: [patch]
Attached patch Better fix (obsolete) — Splinter Review
Brendan instructed me in the proper way to make getters work correctly.
Attachment #210163 - Attachment is obsolete: true
Attachment #210171 - Flags: review?(brendan)
Attachment #210163 - Flags: review?(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;
>     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.

Attachment #210171 - Flags: review?(brendan) → review-
Attached patch Best fix (obsolete) — Splinter Review
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.
Attachment #210171 - Attachment is obsolete: true
Attachment #210181 - Flags: review?(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.

Attachment #210181 - Flags: review?(brendan) → review+
Fix checked into trunk.
Closed: 14 years ago
Resolution: --- → FIXED
Comment on attachment 210298 [details] [diff] [review]
What I checked in

Thanks, nominating for and plusing for 1.8.1.

Attachment #210298 - Flags: review+
Attachment #210298 - Flags: branch-1.8.1+
Attachment #210298 - Flags: approval1.8.0.2?
Attachment #210181 - Attachment is obsolete: true
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

Flags: blocking1.8.1+
Flags: blocking1.8.0.2?
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.
Flags: testcase+
Flags: blocking1.8.0.2? → blocking1.8.0.2+
Comment on attachment 210298 [details] [diff] [review]
What I checked in

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #210298 - Flags: approval1.8.0.2? → approval1.8.0.2+
Fix checked into the 1.8 branches.
Whiteboard: [patch] → [patch][rft-dl]
v ff 20060302 win/linux/mac
Keywords: fixed1.8.1
Keywords: fixed1.8.1
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
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
You need to log in before you can comment on or make changes to this bug.