354541 - responseText.trim is not a function

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Reto Laemmler]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.0.7) Gecko/20060909 Firefox/1.5.0.7
Build Identifier: FireFox 2.0 rc1

www.xcellery.com uses AJAX.NET from Michael Schwartz which uses the trim function from javascript. Sign in as demo user on www.xcellery.com brings "responseText.trim is not a function" when loading the My Documents page. This doesn't happen with any of the previous versions of FireFox.

Reproducible: Always

Steps to Reproduce:
1. go to www.xcellery.com
2. click on "Sign in as Demo user"


Actual Results:  
you see the error "responseText.trim is not a function" in a message box. 

Expected Results:  
it should load the workbooks in the "My Documents" page.

This only happens with FireFox 2 rc1.

Comment 1

[:Gavin Sharp [email: [e-mail]]]

Are you affiliated with www.xcellery.com? Is there any chance you could help out with identifying the problematic code? The site uses a rather large amount of JavaScript, pinpointing the bit that changed between Firefox 1.5 and Firefox 2.0 RC 1 isn't trivial.

You could also try to find a regression range by binary searching through the -mozilla1.8 builds available at http://archive.mozilla.org/pub/firefox/nightly/ .

Comment 2

[Reto Laemmler]

Yes I run www.xcellery.com. We use a AJAX.net adapter from http://ajaxpro.info/ 
That adapter generates all the javascript from the ASP.NET webservice interface. I logged a bug against his API as well. See http://www.codeplex.com/WorkItem/List.aspx?ProjectName=AjaxPro

Per my understanding the trim function is not part of the Javascript standard but used to be supported by all the previous versions of FF. I will ask my developer how comfortable he is to pinpoint this issue. I also hope to hear from Michael Schwartz.

Comment 3

[Reto Laemmler]

Yes I run www.xcellery.com. We use a AJAX.net adapter from http://ajaxpro.info/ 
That adapter generates all the javascript from the ASP.NET webservice interface. I logged a bug against his API as well. See http://www.codeplex.com/WorkItem/List.aspx?ProjectName=AjaxPro

Per my understanding the trim function is not part of the Javascript standard but used to be supported by all the previous versions of FF. I will ask my developer how comfortable he is to pinpoint this issue. I also hope to hear from Michael Schwartz.

Comment 4

[Adam Guthrie]

This regressed sometime between 2006-07-06-04 and 2006-07-07-04. Check-ins: http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=PhoenixTinderbox&branch=MOZILLA_1_8_BRANCH&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2006-07-06+03&maxdate=2006-07-07+05&cvsroot=%2Fcvsroot

JS1.7 landing looks like a likely regressor.

Comment 5

[Biju]

Here is what I found, on http://www.xcellery.com/MyDocumentsView.aspx

javascript:alert([].clear)
works 

but following gives undefined
javascript:alert(" A ".trim)
javascript:alert(String.prototype.trim)
javascript:alert(String.format)  

javascript:try{alert(" A ".trim())}catch(e){alert(e)}
give " A ".trim not a function


Only place I see  responseText.trim() used is in
http://www.xcellery.com/ajaxpro/core.ashx


.trim() is defined at
http://www.xcellery.com/ajaxpro/prototype.ashx
at Object.extend(String.prototype, {


you can also test same thing at http://www.xcellery.com/


if you include 
http://www.xcellery.com/ajaxpro/prototype.ashx
on a test page "  aaa ".trim() it works fine


can it be a problem with 
eval("r.value = " + responseText + "*/");
at "createResponse"  in http://www.xcellery.com/ajaxpro/core.ashx


BTW:
  var xx = document.all ? document.all["yyy"] : document.getElementById("yyy");

//  can be written as just 
  var xx = document.getElementById("yyy");

Comment 6

[Martijn Wargers (dead)]

Attachment: testcase

This testcase is pretty much minimized.
You should get an alert('hallo'), but that doesn't happen with current trunk/1.8.1 branch builds.
Removing the </script><script>, that separates the script blocks makes the bug go away.

Comment 7

[Martijn Wargers (dead)]

Not sure if it should block 1.8.1, but it seems like a bad bug to me.

Comment 8

[Igor Bukanov]

Attachment: Even smaller test case

If in the test case one comment out "case String" in tmp function that is never called, the bug goes away. It looks like a bug with the compiler/emitter.

Comment 9

[Mike Beltzner [:beltzner, not reading bugmail]]

JS folks: can we get a good understanding of the impact of this?

Comment 10

[Brendan Eich [:brendan]]

Attachment: all-in-one js shell testcase

Working on a patch.

Any use of a standard class constructor (String in this case) in a case label of a switch statement will cause the existing String (and therefore String.prototype) value to be lost.  We should fix this regression ASAP.

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: fix regression from patch for bug 293782

The changes to fix bug 293782 (see also bug 137000) included this regression, which looks for any identifier used in a switch case expression at compile time, as a hidden atom.  Such a hidden atom lookup should happen only if the object on the compile-time scope chain is a function object.

Per ECMA-262, function objects do *not* have properties for their formals or locals.  But SpiderMonkey descends from Mocha, the original JS runtime (Netscape 2, 1995), where such reflection was done from the beginning.

Hidden atoms are a cheap way to conserve long-standing code that stores formal parameters and local variables as properties of compiler-created function objects, instead of using equivalent but hidden property name mapping and value storage data structures and code.  IOW, only the atoms are hidden -- the code and data structures are shared.

So hidden atoms were a win, but this silly bug crept in on their back.  It was always a bad idea to look for hidden atoms on arbitrary objects.  Dunno what I was thinking.  Since a hidden atom has the same chars as its unhidden counterpart, lazy class init code that resolves String and hidden(String) will initialize the String class twice, which results in this bug's testcase in the added prototype method disappearing.

/be

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 240497 [details] [diff] [review]
fix regression from patch for bug 293782

Looking for shaver sr since he r/a'd for bug 293782.

Asking igor to r+ in case mrbkap is not available, and just for extra review.

/be

Comment 13

[Brendan Eich [:brendan]]

Reporter, thanks for finding this in the nick of time.  Our testsuite didn't cover this case.  It's trickier than I described, because you need two separate script tags, or (in the js shell testcase) two compilation units in general, one where String.prototype is decorated and a later one where String is used in a switch case label.

/be

Comment 14

[Igor Bukanov]

Comment on attachment 240497 [details] [diff] [review]
fix regression from patch for bug 293782

Any way to assert the following comment, "Any hidden property must be a formal arg or local var," ?

Comment 15

[Brendan Eich [:brendan]]

(In reply to comment #14)
> (From update of attachment 240497 [details] [diff] [review] [edit])
> Any way to assert the following comment, "Any hidden property must be a formal
> arg or local var," ?

Sure, JS_ASSERT(sprop->getter == js_GetArgument || sprop->getter == js_GetLocalVariable). I'll add it and test a bunch.

/be

Comment 16

[Brendan Eich [:brendan]]

Attachment: fix, with assertion

I'm committing this to the trunk.

/be

Comment 17

[Brendan Eich [:brendan]]

Comment on attachment 240506 [details] [diff] [review]
fix, with assertion

Going for 1.8.1 approval.  The pending review requests are just for good peering in the long run; testing in addition to Igor's r+ is enough for 1.8.1 rc2.  More review is always good, but it shouldn't be required before the patch is approved based on testing and existing review.

/be

Comment 18

[Brendan Eich [:brendan]]

Fixed on trunk:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.214; previous revision: 3.213
done

/be

Comment 19

[Mike Schroepfer]

Comment on attachment 240506 [details] [diff] [review]
fix, with assertion

Approved for RC2.

Comment 20

[Brendan Eich [:brendan]]

Fixed on the 1.8 branch:

Checking in jsemit.c;
/cvsroot/mozilla/js/src/jsemit.c,v  <--  jsemit.c
new revision: 3.128.2.51; previous revision: 3.128.2.50
done

/be

Comment 21

[Bob Clary [:bc] (inactive)]

Checking in regress-354541-01.js;
/cvsroot/mozilla/js/tests/js1_5/String/regress-354541-01.js,v  <--  regress-354541-01.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/String/regress-354541-02.js,v
done
Checking in regress-354541-02.js;
/cvsroot/mozilla/js/tests/js1_5/String/regress-354541-02.js,v  <--  regress-354541-02.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/String/regress-354541-03.js,v
done
Checking in regress-354541-03.js;
/cvsroot/mozilla/js/tests/js1_5/String/regress-354541-03.js,v  <--  regress-354541-03.js
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/js/tests/js1_5/String/regress-354541-04.js,v
done
Checking in regress-354541-04.js;
/cvsroot/mozilla/js/tests/js1_5/String/regress-354541-04.js,v  <--  regress-354541-04.js
initial revision: 1.1
done

Comment 22

[Bob Clary [:bc] (inactive)]

verified fixed 1.8 1.9 20061002 windows/linux 1.8 macppc

Comment 23

[Bob Clary [:bc] (inactive)]

syncing cvs to the test versions I have been running: do not perform the test result comparisons for skipped tests.

Checking in regress-354541-03.js;
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-354541-03.js,v  <--  regress-354541-03.js
new revision: 1.2; previous revision: 1.1
done
Checking in regress-354541-04.js;
/cvsroot/mozilla/js/tests/js1_5/extensions/regress-354541-04.js,v  <--  regress-354541-04.js
new revision: 1.2; previous revision: 1.1
done