Closed Bug 354541 Opened 13 years ago Closed 13 years ago
Text .trim is not a function
877 bytes, text/html
386 bytes, text/html
255 bytes, text/plain
1.72 KB, patch
|Details | Diff | Splinter Review|
Component: Location Bar and Autocomplete → General
QA Contact: location.bar → general
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.
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
Version: 1.8 Branch → Trunk
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
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
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] ) > 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
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
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: 18.104.22.168; previous revision: 22.214.171.124 done /be
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
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.