Closed Bug 354541 Opened 13 years ago Closed 13 years ago

responseText.trim is not a function

Categories

(Core :: JavaScript Engine, defect, P1, major)

defect

Tracking

()

VERIFIED FIXED
mozilla1.8.1

People

(Reporter: rlaemmler, Assigned: brendan)

References

()

Details

(Keywords: regression, testcase, verified1.8.1)

Attachments

(4 files, 1 obsolete file)

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.
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/ .
Component: Location Bar and Autocomplete → General
Keywords: regression
QA Contact: location.bar → general
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.
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.
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.
Assignee: nobody → general
Component: General → JavaScript Engine
OS: Windows XP → All
Product: Firefox → Core
QA Contact: general → general
Hardware: PC → All
Version: unspecified → 1.8 Branch
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");
Attached file 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.
Not sure if it should block 1.8.1, but it seems like a bad bug to me.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: in-testsuite?
Flags: blocking1.8.1?
Keywords: testcase
Version: 1.8 Branch → Trunk
Attached file 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.
JS folks: can we get a good understanding of the impact of this?
Flags: blocking1.8.1? → blocking1.8.1+
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
Assignee: general → brendan
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: --- → mozilla1.8.1
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
Attachment #240497 - Flags: review?(mrbkap)
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
Attachment #240497 - Flags: superreview?(shaver)
Attachment #240497 - Flags: review?(igor.bukanov)
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 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," ?
Attachment #240497 - Flags: review?(igor.bukanov) → review+
(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
I'm committing this to the trunk.

/be
Attachment #240506 - Flags: superreview?(shaver)
Attachment #240506 - Flags: review+
Attachment #240497 - Attachment is obsolete: true
Attachment #240497 - Flags: superreview?(shaver)
Attachment #240497 - Flags: review?(mrbkap)
Attachment #240497 - Flags: review+
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
Attachment #240506 - Flags: review?(mrbkap)
Attachment #240506 - Flags: approval1.8.1?
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
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Comment on attachment 240506 [details] [diff] [review]
fix, with assertion

Approved for RC2.
Attachment #240506 - Flags: approval1.8.1? → approval1.8.1+
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
Keywords: fixed1.8.1
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
Flags: in-testsuite? → in-testsuite+
verified fixed 1.8 1.9 20061002 windows/linux 1.8 macppc
Status: RESOLVED → VERIFIED
Attachment #240506 - Flags: superreview?(shaver)
Attachment #240506 - Flags: review?(mrbkap)
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
You need to log in before you can comment on or make changes to this bug.