Last Comment Bug 496316 - E4X: Namespace variable resolution fails ("xmlns is undefined") if defined in function context
: E4X: Namespace variable resolution fails ("xmlns is undefined") if defined in...
Status: RESOLVED FIXED
fixed-in-tracemonkey
: regression
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: 1.9.1 Branch
: All All
: P2 critical (vote)
: mozilla1.9.1
Assigned To: Brendan Eich [:brendan]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: tomtom upvar2
  Show dependency treegraph
 
Reported: 2009-06-04 04:44 PDT by Matteo Ferretti
Modified: 2009-06-04 21:08 PDT (History)
9 users (show)
brendan: wanted1.9.1?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
essentially a one-line fix (537 bytes, patch)
2009-06-04 07:27 PDT, Brendan Eich [:brendan]
mrbkap: review+
igor: review+
Details | Diff | Splinter Review

Description Matteo Ferretti 2009-06-04 04:44:26 PDT
User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_5_6; en-us) AppleWebKit/528.16 (KHTML, like Gecko) Version/4.0 Safari/528.16
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1pre) Gecko/20090602 Shiretoko/3.5pre

In the new XULRunner build seems that E4X doesn't set correctly the xmlns attribute.

If we try to set "lang" attribute on xmlns, will be raised an ReferenceError ('xmlns is not defined") if this code is executed into a function.

This error is reproducible in Firefox (last build), both of Windows and OS X. Executing this line on Error Console:

 (function() {var xmlns = new Namespace("xml", "http://www.w3.org/XML/1998/namespace"); var xml = <data />; xml.@xmlns::lang = "en-US"; alert(xml.toXMLString());})()

will raise an error, meanwhile:

 var xmlns = new Namespace("xml", "http://www.w3.org/XML/1998/namespace"); var xml = <data />; xml.@xmlns::lang = "en-US"; alert(xml.toXMLString());

will be fine.

Reproducible: Always

Steps to Reproduce:
1. Open Error Console on Firefox
2. Evaluate: (function() {var xmlns = new Namespace("xml", "http://www.w3.org/XML/1998/namespace"); var xml = <data />; xml.@xmlns::lang = "en-US"; alert(xml.toXMLString());})()
3. The ReferenceError "xmlns is not defined" will be raised.
Actual Results:  
a ReferenceError that said xmlns is not defined

Expected Results:  
no throwing error, xmlns should be defined and lang attribute set as well; at the end, the alert() should be displayed the correct XML code with namespace and lang.
Comment 1 Wladimir Palant 2009-06-04 05:24:54 PDT
The short version of the above:

|xml.@xmlns::foo = "bar"| fails if xmlns was declared inside a function rather than globally. This regressed on 1.9.1 branch between 20090417 and 20090418.

Tweaking summary and confirming.
Comment 2 Wladimir Palant 2009-06-04 05:59:20 PDT
The regression range is 37d1bb0c86b5 to 7b39d6b3b56d. I ruled out bug 481514 and bug 479198, backing them out doesn't fix the issue. Most likely candidate is bug 452498 then.
Comment 3 Wladimir Palant 2009-06-04 06:05:28 PDT
Marking as blocking bug 452498 given the similarities to bug 496134. Requesting "blocking-1.9.1" given that this is a major regression in E4X functionality.
Comment 4 Brendan Eich [:brendan] 2009-06-04 07:27:32 PDT
Created attachment 381529 [details] [diff] [review]
essentially a one-line fix
Comment 5 Brendan Eich [:brendan] 2009-06-04 07:28:51 PDT
The contributed (BEA, originally Crossgain) E4X test coverage seems pretty poor...

/be
Comment 6 Brendan Eich [:brendan] 2009-06-04 07:30:28 PDT
Wanted not blocking, since a workaround is to add an eval or with to the function. Fix essentially does that, is safe, so we should take.

/be
Comment 7 Brendan Eich [:brendan] 2009-06-04 07:32:36 PDT
Workaround in action. First the bug:

$ cat /tmp/xmlns.js
(function() {var xmlns = new Namespace("xml", "http://www.w3.org/XML/1998/namespace"); var xml = <data />; xml.@xmlns::lang = "en-US"; print(xml.toXMLString());})()
$ ./Darwin_DBG.OBJ/js /tmp/xmlns.js
/tmp/xmlns.js:1: ReferenceError: xmlns is not defined

Now the workaround:

$ cat /tmp/xmlns2.js
(function() {eval('42'); var xmlns = new Namespace("xml", "http://www.w3.org/XML/1998/namespace"); var xml = <data />; xml.@xmlns::lang = "en-US"; print(xml.toXMLString());})()
$ ./Darwin_DBG.OBJ/js /tmp/xmlns2.js
<data xml:lang="en-US"/>

/be
Comment 8 Brendan Eich [:brendan] 2009-06-04 08:27:56 PDT
Comment on attachment 381529 [details] [diff] [review]
essentially a one-line fix

Igor is up now and mrbkap is not, so looking for r+ from Igor -- this is an easy one, I want to get it into tm ASAP.

/be
Comment 9 Igor Bukanov 2009-06-04 08:56:51 PDT
Comment on attachment 381529 [details] [diff] [review]
essentially a one-line fix

It seems there is a related issue with JSOP_DEFXMLNS: it sets fp->xmlNamespace only when fp->varobj is null. Thus, if a Call object for a lightweight would be requested via, for example, a debugger, then the second "default xml namespace" would not be visisble as it would set the namespace in the call object while js_GetDefaultXMLNamespace would continue to use fp->xmlNamespace. Marking any function using "default xml namespace" would allow to address this and eliminate AFAICS fp->xmlNamespace. But this is for another bug.
Comment 10 Igor Bukanov 2009-06-04 08:58:11 PDT
(In reply to comment #9)
> Marking any function using "default xml namespace" would allow to address this and eliminate AFAICS fp->xmlNamespace. But this is for another bug.

I mean marking any function using "default xml namespace" as heavyweight.
Comment 11 Brendan Eich [:brendan] 2009-06-04 09:01:40 PDT
(In reply to comment #9)
> (From update of attachment 381529 [details] [diff] [review])
> It seems there is a related issue with JSOP_DEFXMLNS: it sets fp->xmlNamespace
> only when fp->varobj is null. Thus, if a Call object for a lightweight would be
> requested via, for example, a debugger, then the second "default xml namespace"
> would not be visisble as it would set the namespace in the call object while
> js_GetDefaultXMLNamespace would continue to use fp->xmlNamespace. Marking any
> function using "default xml namespace" would allow to address this and
> eliminate AFAICS fp->xmlNamespace. But this is for another bug.

Already in my patch queue for bug 471425.

/be
Comment 12 Brendan Eich [:brendan] 2009-06-04 10:59:13 PDT
http://hg.mozilla.org/tracemonkey/rev/fb21a3a12180

/be
Comment 13 Andreas Wuest 2009-06-04 15:23:09 PDT
Brendan, thanks a lot for the quick turnaround!

Can we safely take this patch and apply it to our 1.9.1b4 XULRunner, or do we also need some changes that landed after b4 in 1.9.1?
Comment 14 Brendan Eich [:brendan] 2009-06-04 15:42:13 PDT
(In reply to comment #13)
> Brendan, thanks a lot for the quick turnaround!
> 
> Can we safely take this patch and apply it to our 1.9.1b4 XULRunner, or do we
> also need some changes that landed after b4 in 1.9.1?

Should apply cleanly.

/be

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