Bug 163996

Summary:

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

Product:

WebKit

Reporter:

Mark Lam <mark.lam>

Component:

JavaScriptCore

Assignee:

Mark Lam <mark.lam>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, cdumez, cmarcelo, commit-queue, dbates, fpizlo, ggaren, jfbastien, keith_miller, msaboff, saam, webkit-bug-importer, ysuzuki

Priority:

Keywords:

InRadar

Version:

WebKit Local Build

Hardware:

Unspecified

OS:

Unspecified

Bug Depends on:

164125

Bug Blocks:

Attachments:

Description	Flags
proposed patch.	none
proposed patch.	ggaren: review+

Description Mark Lam 2016-10-25 16:57:04 PDT

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

Comment 1 Mark Lam 2016-10-25 16:58:12 PDT

<rdar://problem/28263117>

Comment 2 Mark Lam 2016-10-25 17:07:26 PDT

Created attachment 292852 [details]
proposed patch.

Comment 3 Geoffrey Garen 2016-10-25 17:17:51 PDT

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);

Comment 4 Mark Lam 2016-10-25 17:19:06 PDT

(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.

Comment 5 Mark Lam 2016-10-25 17:25:38 PDT

Created attachment 292853 [details]
proposed patch.

Comment 6 Geoffrey Garen 2016-10-25 17:32:54 PDT

Comment on attachment 292853 [details]
proposed patch.

r=me

Comment 7 Mark Lam 2016-10-25 18:19:17 PDT

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