83293 - replace function stops the whole script if search is an empty string

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect)

Description

[Marcel Greter]

From Bugzilla Helper:
BuildID: 2001052904

When I call "string".replace('','something') everything stops, no output and no 
debug message. The problem is the empty string. Netscape 4 and IE 
returns "string"

Reproducible: Always
Steps to Reproduce:
enter javascript:alert("string".replace('','')) in the URL Bar, that's it!

Actual Results:  nothing, the running script is terminated without any message
Expected Results:  it should return "string"

Comment 1

[Phil Schwartau]

The correct syntax for String.prototype.replace is not to supply a string 
as the first parameter, but a regular expression: 

Syntax 
  replace(regexp, newSubStr)
  replace(regexp, function)



Nevertheless the ECMA-262 spec Edition 3 allows for conversion of
the first parameter to a RegExp in case it is not already one:

------------------------------------------------------------------------------
15.5.4.11 String.prototype.replace (regexp, replaceValue) 

  If regexp is not an object whose [[Class]] property is "RegExp", 
  it is replaced with the result of the expression new RegExp(regexp). 

  Let string denote the result of converting the 'this' value to a string. 
  If regexp.global is false, string is searched for the first occurrence
  of the regular expression pattern regexp. If regexp.global is true, string 
  is searched for all occurrences of the regular expression pattern regexp. 

  The search is done in the same manner as in String.prototype.match, 
  including the update of regexp.lastIndex. 
------------------------------------------------------------------------------


It appears we do this conversion correctly in NN4.7, but not in Mozilla
on certain edge cases. Details below -

Comment 2

[Phil Schwartau]

Here are some edge cases in both browsers. 
Using Mozilla nightly binary 20010527xx on WinNT.                                                                                                                                                                                                                  

                                                           NN4.7    Mozilla

javascript:alert('abc'.replace('a','Z'))                   'Zbc'     'Zbc'
javascript:alert('abc'.replace(new RegExp('a'),'Z'))       'Zbc'     'Zbc'

javascript:alert('abc'.replace('','Z'))                    'Zabc'    no return!
javascript:alert('abc'.replace(new RegExp(''),'Z'))        'Zabc'    'Zabc'

javascript:alert('abc'.replace(undefined,'Z'))             'abc'     'Zabc'
javascript:alert('abc'.replace(new RegExp(undefined),'Z')) 'abc'     'abc'

javascript:alert('abc'.replace(null,'Z'))                  'abc'     'abc'
javascript:alert('abc'.replace(new RegExp(null),'Z'))      'abc'     'abc'



We can see that NN4.7 correctly treats both syntaxes the same.
But in Mozilla, the empty string and undefined produce different
results in one syntax vs. the other - 

Comment 3

[Phil Schwartau]

I am puzzled by this edge case: 

javascript:alert('abc'.replace( ,'Z'))


It produces this error in both browsers:

  Error: syntax error
  alert('abc'.replace( ,'Z'))
  -----------^


I thought that not providing a parameter to any JavaScript function
was the same as explicitly providing the value undefined. However, 
explicitly providing the value undefined produced no such error above - 

Comment 4

[rogerl (gone)]

per Clayton, push to later.

Comment 5

[Phil Schwartau]

OK, rogerl has explained my last question. The ECMA spec allows for 
omitted variables on the RIGHT side of an argument list, but does
not allow variables on the LEFT to be omitted by preceding the 
comma operator with empty expressions.


EXAMPLE

js> function f(a,b) {return arguments.length}
js> f()
0
js> f(1)
1
js> f(1,2)
2
js> f(1,2,3)
3

      ETC.
      ETC. 


BUT: 

js> f(,2)
74: syntax error:
74: f(,2)
74: ..^

Comment 6

[Phil Schwartau]

Testcase added to JS testsuite:

              js/tests/ecma_3/String/regress-83293.js

Comment 7

[Asa Dotzler [:asa]]

pushing out. 0.9.2 is done. (querying for this string will get you the list of
the 0.9.2 bugs I moved to 0.9.3)

Comment 8

[Kenton Hanson (gone)]

proposed patch
Index: mozilla/js/src/jsstr.c
===================================================================
RCS file: /cvsroot/mozilla/js/src/jsstr.c,v
retrieving revision 3.48
diff -r3.48 jsstr.c
919c919
<     JSBool ok;
---
>     JSBool ok = JS_FALSE;
946c946,949
<             re = js_NewRegExpOpt(cx, NULL, src, opt, forceFlat);
---
>             if (src->length == 0)
>                 re = js_NewRegExp(cx, NULL, cx->runtime->emptyString, 0, JS_FALSE);
>             else
>                 re = js_NewRegExpOpt(cx, NULL, src, opt, forceFlat);

Comment 9

[Kenton Hanson (gone)]

Previous comment contains proposed patch

Comment 10

[rogerl (gone)]

r=rogerl

Comment 11

[Brendan Eich [:brendan]]

Sorry, that's not the right patch -- the problem is that js_NewRegExp overloads
its null return value to signify error and flat+empty string non-error cases. 
My patch coming up (NB: diff -u format, best for patches).

/be

Comment 12

[Brendan Eich [:brendan]]

It's important to pass opt to js_NewRegExpOpt in the empty src case, too. 
Consider

    'hi'.replace('', 'bye', 'g');

which should result in 'byehbyeibye', but with the bad patch, results in
'byehi'.

Also, there's no need to initialize ok to JS_FALSE -- it's no big deal, but the
control flow clearly sets ok before any uses.

/be

Comment 13

[Brendan Eich [:brendan]]

Attachment: disambiguate null return from js_NewRegExp to mean error, as usual; respect optional flags in replace's third parameter

Comment 14

[Phil Schwartau]

*** Bug 92942 has been marked as a duplicate of this bug. ***

Comment 15

[Brendan Eich [:brendan]]

kenton, rogerl, how about r= for the attached patch?  shaver, sr=?

/be

Comment 16

[rogerl (gone)]

r=rogerl

Comment 17

[Mike Shaver (:shaver emeritus)]

sr=shaver

Comment 18

[Jim Ley]

I hate to say this, for fear of killing the fixes, but the quotation from the 
ECMA-262 Ed. 3 is incorrect, that is one the one in the released final draft I 
believe, the actual standard - My copy dated "09-02-00 14,32" (assumed Swiss 
date so 2000-02-09) contains:

------------------------------------------------------------------------------

15.5.4.11 String.prototype.replace (searchValue, replaceValue) 

Let string denote the result of converting the this value to a string.
If searchValue is a regular expression (an object whose [[Class]]
property is "RegExp"), do the following: If searchValue.global is
false, then search string for the first match of the regular
expression searchValue. If searchValue.global is true, then search
string for all matches of the regular expression searchValue. Do the
search in the same manner as in String.prototype.match, including the
update of searchValue.lastIndex. Let m be the number of left capturing
parentheses in searchValue (NCapturingParens as specified in
15.10.2.1). 

If searchValue is not a regular expression, let searchString be
ToString(searchValue) and search string for the first occurrence of
searchString. Let m be 0.

------------------------------------------------------------------------------

This is different to the NN4.7 situation:

                                            NN4.7    ECMAScript
javascript:alert("a$a".replace('$','Z'))    'a$aZ'      'aZa'


Jim.

Comment 19

[Phil Schwartau]

Jim is correct - there is a difference on this point between the 
"ECMAScript Edition 3 Final Draft" (dated 13 October 1999) and the 
"ECMAScript Edition 3 Standard" (dated December 1999).

The latter is available at:

           http://www.ecma.ch/ecma1/STAND/ECMA-262.HTM

Comment 20

[Phil Schwartau]

This means that if str, strA, strB are strings, we should NOT expect 

   str.replace(strA, strB) == str.replace(new RegExp(strA),strB)



I will modify or replace this testcase accordingly:

               js/tests/ecma_3/String/regress-83293.js



The original problem in this bug still seems valid, however.
Paraphrasing it from above: 


>  When I call "abc".replace('','Z') everything stops, no output and 
>  no debug message. The problem is the empty string. Netscape 4 and IE 
>  return "Zabc"

Comment 21

[Phil Schwartau]

It is notable that in the final version of the spec, String.prototype.replace 
differs completely from String.prototype.match and String.prototype.search 
when the "searchValue" does not have [[Class]] == "RegExp".

cc'ing Waldemar to be certain that the spec is correct on this point. 
It does not seem right to me - it means, for example, that "a$a".search('$')
and "a$a".replace('$','Z') treat '$' in completely different ways!!!

Note again this change seems to have been made between the Final Draft and 
the Final Standard, dated October 1999 and December 1999, respectively -

Comment 22

[Jim Ley]

It changed between final draft and standard the reasons being given in the 
discussion at:

http://www2.hursley.ibm.com/tc39/tcn9911.htm
(seems to be unavailable at time of writing.)
http://www.google.com/search?
q=cache:f6BtWdjvnNM:www2.hursley.ibm.com/tc39/tcn9911.htm

Jim.

Comment 23

[Waldemar Horwat]

The ECMAScript spec is as the committee intended here.  I wanted to make match
and search behave analogously to the new behavior of replace, but I got
overruled because both Netscape's and Microsoft's implementations had already
implemented the bad behavior of turning string literals into regular
expressions.  Some members of the committee also argued that you can use the
existing indexOf method to search for a string, while there was no corresponding
method to replace on a string.

BTW, the Edition 3 specs at <http://www.mozilla.org/js/language/> are final. 
For some strange reason the one on ECMA's own site isn't quite the final one,
although the differences are extremely minor and do not affect the spec's
meaning -- there are a few broken chapter reference links and other typos. 
We'll fix these and incorporate the errata when we make the ISO spec.

Comment 24

[Phil Schwartau]

Testcase corrected to conform to ECMA-262 Final Edition:

            js/tests/ecma_3/String/regress-83293.js


Currently failing as follows:

'abc'.replace(undefined, 'Z') should be 'abc'; SpiderMonkey gives 'Zabc'.
'abc'.replace('', 'Z') should be 'Zabc'; SpiderMonkey gives no return at all.


The testcase is passing in Rhino -

Comment 25

[Brendan Eich [:brendan]]

I wouldn't hold 0.9.4 for this, but let's get it fixed in 0.9.5.  I'll help in
any way needed.

/be

Comment 26

[Brendan Eich [:brendan]]

What went wrong?  Should I get r= and sr= for my patch in this bug?  Maybe I
should just have taken the bug, since I supplied the patch.  It's a shame this
has gone unfixed for so long.

/be

Comment 27

[Mike Shaver (:shaver emeritus)]

Looks to me like you got r=rogerl and sr=shaver on 07/31 and 08/01 respectively.  

Comment 28

[Brendan Eich [:brendan]]

shaver: I did get old-style r/sr, but my patch only fixes part of the problem. 
I should've checked in, though -- sorry about that.  Cc'ing asa in the hope that
this oversight can be corrected for 0.9.5.  Asa, it's a four-line (effectively)
patch that fixes broken JS functionality.  No effects beyond the replace method,
already r/sr'd and tested.

Phil, is this bug also covering the Zabc instead of abc result from
'abc'.replace(undefined, 'Z') problem?  If so, my patch is not enough to close
this bug.

/be

Comment 29

[Phil Schwartau]

I have filed bug 103351 for the other issue. So we have this coverage:

This bug:
'abc'.replace('', 'Z') should be 'Zabc'; SpiderMonkey gives no return at all.

Bug 103351:
'abc'.replace(undefined, 'Z') should be 'abc'; SpiderMonkey gives 'Zabc'.


So now this bug addresses only the issue originally reported -

Comment 30

[Asa Dotzler [:asa]]

Comment on attachment 42941 [details] [diff] [review]
disambiguate null return from js_NewRegExp to mean error, as usual; respect optional flags in replace's third parameter

a=asa (on behalf of drivers) for checkin to 0.9.5 branch

Comment 31

[Brendan Eich [:brendan]]

Fix checked into MOZILLA_0_9_5_BRANCH and trunk.  Thanks,

/be

Comment 32

[Phil Schwartau]

Verified Fixed in standalone debug and optimized JS shells built 2001-10-08
on WinNT, Linux, and Mac.  Also in debug builds of Mozilla made 2001-10-08.

'abc'.replace('', 'Z') returns 'Zabc'