Bug 3294 – String.prototype.replace() fails with function as second param

• Component: JavaScriptCore
• Status: VERIFIED FIXED

[Gavin Kistner]

ECMAScript-262 rev3 section 15.5.4.11 (page 102) allows the second parameter to the replace() method 
of String objects to be a function. However, JavaScriptCore does not support this feature (at least in the 
build of Safari 2.0.412), instead calling .toString() on the second parameter supplied to the replace() 
method, and using that as the return string.

The attached file "ReplaceTest.html" (also viewable at http://phrogz.net/JS/replaceTest.html) performs five 
tests of the replace method.

The severity is normal because a workaround exists - similar functionality can be achieved (more 
painfully) by using a regexp with the 'g' flag and repeatedly calling .exec, working through the original 
string and building the output.

[Gavin Kistner]

Created attachment 2127 [details]
5 replace() tests, including one for this bug.

[Gavin Kistner]

(This bug was copied from Apple's Bug Reporter #3749350)

[Anders Carlsson]

Created attachment 2489 [details]
Handle the second argument being a function

Here's a patch that makes the test cases work for me.

[Darin Adler]

Comment on attachment 2489 [details]
Handle the second argument being a function

I think the args for the replacement function should be declared inside each if
statement. There's no reason to keep the args list around between invocations.

And if you name it args, then the code is going to be a bit smaller too.

Does this do the right thing when the replacement function raises a JavaScript
exception?

Extra space after substr before "(".

Minor issues; this looks pretty good.

[Anders Carlsson]

Created attachment 2504 [details]
Address comments

Here's a new patch that addresses the issues.

Regarding exceptions, the following code has been tested in kjs and found to be
working:

	function test(s) {
		throw ReferenceError;
	}
	
	try {
		testStr="hello";
		result = testStr.replace("hello", test);
	} catch (e) {
		debug(e);
	}

and gives the result
--> (Internal Function)

If this isn't the correct behavior, then I suspect the same error applies for
compareWithCompareFunctionForQSort in array_object.cpp

[Anders Carlsson]

Created attachment 2505 [details]
Remove the space after substr

Sorry, forgot the space after substr

[Darin Adler]

Comment on attachment 2505 [details]
Remove the space after substr

r=me, given that we can land the attached test as a layout test.

[Joost de Valk (AlthA)]

Fixed in 10.4.2

[John Sullivan]

This appears fixed in tip of tree CVS, but does not appear fixed in 10.4.2 (contradicting the comment from 
Joost de Valk).

[Darin Adler]

In Radar as <rdar://problem/4290603> String.replace() does not support a function as a parameter 
(3294).