Bug 163766 - bmalloc api should crash on failure to allocate when !isBmallocEnabled.
Summary: bmalloc api should crash on failure to allocate when !isBmallocEnabled.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: bmalloc (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-10-20 16:43 PDT by Mark Lam
Modified: 2016-10-20 17:19 PDT (History)
1 user (show)

See Also:

Attachments
proposed patch. (2.11 KB, patch)
2016-10-20 16:55 PDT, Mark Lam 		keith_miller: review+
Details | Formatted Diff | Diff
View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-10-20 16:43:51 PDT 
Patch coming.
Comment 1 Mark Lam 2016-10-20 16:55:03 PDT 
Created attachment 292285 [details]
proposed patch.
Comment 2 Keith Miller 2016-10-20 17:00:23 PDT 
Comment on attachment 292285 [details]
proposed patch.

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

r=me with comments

> Source/bmalloc/bmalloc/Allocator.cpp:107
> +        auto result = realloc(object, newSize);

nit: auto seems a little unnecessary here

> Source/bmalloc/bmalloc/Allocator.cpp:197
> +        auto result = malloc(size);

nit: ditto
Comment 3 Filip Pizlo 2016-10-20 17:00:41 PDT 
Comment on attachment 292285 [details]
proposed patch.

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

R=me.  I'm assuming that there's a reason why we need to crash.

> Source/bmalloc/ChangeLog:3
> +        bmalloc api should crash on failure to allocate when GuardMalloc is used.

I recommend changing the title or adding a clarification that the crash-on-failure is for the !isBmallocEnabled case, which is broader than the GuardMalloc case.
Comment 4 Mark Lam 2016-10-20 17:19:37 PDT 
Thanks for the review.  I've changed the autos to void *s.  I've also changed the title, and added a comment in the ChangeLog to explain why crashing in bmalloc is what we want when we fail to allocate.

Landed in r207646: <http://trac.webkit.org/r207646>.