Bug 163996 – String.prototype.replace() should throw an OutOfMemoryError when using too much memory.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

Currently, it just crashes with an allocation failure, which is correct behavior but not too friendly.

[Mark Lam]

<rdar://problem/28263117>

[Mark Lam]

Created attachment 292852 [details]
proposed patch.

[Geoffrey Garen]

Comment on attachment 292852 [details]
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=292852&action=review

> Source/JavaScriptCore/runtime/StringPrototype.cpp:462
> +            THROW_AND_RETURN_IF_FAIL(exec, scope, sourceRanges.tryConstructAndAppend(lastIndex, result.start - lastIndex));

I don't really like this idiom because it buries the real code inside the error handling macro.

How about this:

if (!sourceRanges.tryConstructAndAppend(lastIndex, result.start - lastIndex))
    OUT_OF_MEMORY(exec, scope);

[Mark Lam]

(In reply to comment #3)
> Comment on attachment 292852 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292852&action=review
> 
> > Source/JavaScriptCore/runtime/StringPrototype.cpp:462
> > +            THROW_AND_RETURN_IF_FAIL(exec, scope, sourceRanges.tryConstructAndAppend(lastIndex, result.start - lastIndex));
> 
> I don't really like this idiom because it buries the real code inside the
> error handling macro.
> 
> How about this:
> 
> if (!sourceRanges.tryConstructAndAppend(lastIndex, result.start - lastIndex))
>     OUT_OF_MEMORY(exec, scope);

Sure.  I'll make the change.

[Mark Lam]

Created attachment 292853 [details]
proposed patch.

[Geoffrey Garen]

Comment on attachment 292853 [details]
proposed patch.

r=me

[Mark Lam]

Thanks for the review.  Landed in r207861: <http://trac.webkit.org/r207861>.