The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in mozilla1.9alpha1

Status

()

Core
JavaScript Engine
P3
normal
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Nickolay_Ponomarev, Assigned: mrbkap)

Tracking

({testcase, verified1.8.0.2, verified1.8.1})

Trunk
mozilla1.9alpha1
x86
Windows XP
testcase, verified1.8.0.2, verified1.8.1
Points:
---
Bug Flags:
blocking1.8.1 +
blocking1.8.0.2 +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [patch][rft-dl])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
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)
(Reporter)

Comment 1

11 years ago
Created attachment 209603 [details]
testcase
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
(Reporter)

Comment 3

11 years ago
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?
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
(Reporter)

Comment 5

11 years ago
> 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.)
(Assignee)

Comment 6

11 years ago
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.
Assignee: general → mrbkap
Status: NEW → ASSIGNED
Attachment #210163 - Flags: review?(brendan)
(Assignee)

Updated

11 years ago
Priority: -- → P3
Target Milestone: --- → mozilla1.9alpha
(Assignee)

Updated

11 years ago
Whiteboard: [patch]
(Assignee)

Comment 7

11 years ago
Created attachment 210171 [details] [diff] [review]
Better fix

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;
> 
>     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
Attachment #210171 - Flags: review?(brendan) → review-
(Assignee)

Comment 9

11 years ago
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.
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.

/be
Attachment #210181 - Flags: review?(brendan) → review+
(Assignee)

Comment 11

11 years ago
Created attachment 210298 [details] [diff] [review]
What I checked in
(Assignee)

Comment 12

11 years ago
Fix checked into trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
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
Attachment #210298 - Flags: review+
Attachment #210298 - Flags: branch-1.8.1+
Attachment #210298 - Flags: approval1.8.0.2?
(Assignee)

Updated

11 years ago
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 1.8.0.2.

/be
Flags: blocking1.8.1+
Flags: blocking1.8.0.2?

Comment 15

11 years ago
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.
Status: RESOLVED → VERIFIED
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+
(Assignee)

Comment 17

11 years ago
Fix checked into the 1.8 branches.
Keywords: fixed1.8.0.2, fixed1.8.1

Updated

11 years ago
Whiteboard: [patch] → [patch][rft-dl]

Comment 18

11 years ago
v ff 1.8.0.1/1.8/1.9 20060302 win/linux/mac
Keywords: fixed1.8.0.2, fixed1.8.1 → verified1.8.0.2, verified1.8.1

Updated

11 years ago
Keywords: fixed1.8.1

Updated

11 years ago
Keywords: fixed1.8.1

Comment 19

10 years ago
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

10 years ago
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.