Bug 164681 - Add debugging facility to limit the max single allocation size.
Summary: Add debugging facility to limit the max single allocation size.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords:
Depends on:
Blocks: 155145 164701
  Show dependency treegraph
 
Reported: 2016-11-12 10:23 PST by Mark Lam
Modified: 2016-11-14 14:22 PST (History)
11 users (show)

See Also:


Attachments
proposed patch. (8.89 KB, patch)
2016-11-12 10:33 PST, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (9.14 KB, patch)
2016-11-13 20:51 PST, Mark Lam
keith_miller: review+
Details | Formatted Diff | Diff
patch for landing. (9.21 KB, patch)
2016-11-14 10:06 PST, Mark Lam
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2016-11-12 10:23:32 PST
This is useful for simulating memory allocation failures on resource constraint devices for testing purposes.
Comment 1 Mark Lam 2016-11-12 10:33:21 PST
Created attachment 294620 [details]
proposed patch.
Comment 2 JF Bastien 2016-11-13 10:25:04 PST
I wasn't clear to me at first that this limits *a single* allocation, not the allocated heap size. I think the wording could be improved there.

Otherwise looks fine.
Comment 3 Mark Lam 2016-11-13 15:37:18 PST
(In reply to comment #2)
> I wasn't clear to me at first that this limits *a single* allocation, not
> the allocated heap size. I think the wording could be improved there.

ChangeLog says "When in use, the max allocation size limit applies to individual allocations."
Comment 4 JF Bastien 2016-11-13 18:05:54 PST
(In reply to comment #3)
> (In reply to comment #2)
> > I wasn't clear to me at first that this limits *a single* allocation, not
> > the allocated heap size. I think the wording could be improved there.
> 
> ChangeLog says "When in use, the max allocation size limit applies to
> individual allocations."

I meant what the --help message says, and the name of the variable.
Comment 5 Mark Lam 2016-11-13 20:19:25 PST
(In reply to comment #4)
> (In reply to comment #3)
> > (In reply to comment #2)
> > > I wasn't clear to me at first that this limits *a single* allocation, not
> > > the allocated heap size. I think the wording could be improved there.
> > 
> > ChangeLog says "When in use, the max allocation size limit applies to
> > individual allocations."
> 
> I meant what the --help message says, and the name of the variable.

I agree that this can be better named.  I'm going to change it to maxSingleAllocationSize.  Updated patch coming.
Comment 6 Mark Lam 2016-11-13 20:51:11 PST
Created attachment 294691 [details]
proposed patch.
Comment 7 JF Bastien 2016-11-13 21:10:40 PST
Comment on attachment 294691 [details]
proposed patch.

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

lgtm

> Source/WTF/wtf/FastMalloc.cpp:60
> +    ASSERT_WITH_MESSAGE((size) <= maxSingleAllocationSize, "Requested size (%zu) exceeds max single allocation size set for testing (%zu)", (size), maxSingleAllocationSize)

Probably doesn't matter, but you have two invocations of size, so potentially bad if it has a side-effect.

> Source/WTF/wtf/FastMalloc.cpp:128
> +        CRASH();

IIUC only the "try" variants are OK with failure? The "fast" variants didn't crash before though, right?
Comment 8 Mark Lam 2016-11-13 22:12:37 PST
Comment on attachment 294691 [details]
proposed patch.

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

>> Source/WTF/wtf/FastMalloc.cpp:60
>> +    ASSERT_WITH_MESSAGE((size) <= maxSingleAllocationSize, "Requested size (%zu) exceeds max single allocation size set for testing (%zu)", (size), maxSingleAllocationSize)
> 
> Probably doesn't matter, but you have two invocations of size, so potentially bad if it has a side-effect.

Yes doesn't matter, but if we do care about this, I can change this macro to only evaluate (size) once.  Let's see what others say.

>> Source/WTF/wtf/FastMalloc.cpp:128
>> +        CRASH();
> 
> IIUC only the "try" variants are OK with failure? The "fast" variants didn't crash before though, right?

From the ChangeLog, "fixed non-bmalloc versions of fastAlignedMalloc() to crash when allocation fails."  Yes, the WebKit convention is that try allocators are supposed to return nullptr on failure to allocate, while non-try versions are to crash on failure to allocate.
Comment 9 Keith Miller 2016-11-14 09:57:40 PST
Comment on attachment 294691 [details]
proposed patch.

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

r=me.

>>> Source/WTF/wtf/FastMalloc.cpp:60
>>> +    ASSERT_WITH_MESSAGE((size) <= maxSingleAllocationSize, "Requested size (%zu) exceeds max single allocation size set for testing (%zu)", (size), maxSingleAllocationSize)
>> 
>> Probably doesn't matter, but you have two invocations of size, so potentially bad if it has a side-effect.
> 
> Yes doesn't matter, but if we do care about this, I can change this macro to only evaluate (size) once.  Let's see what others say.

I'm ok either way but I would personally change it since it can confusing when effects happen twice. Especially since the error message might be printing a different value from the one that actually crashed.
Comment 10 Mark Lam 2016-11-14 10:06:27 PST
Created attachment 294710 [details]
patch for landing.
Comment 11 Mark Lam 2016-11-14 10:07:42 PST
Thanks for the review.  Landed in r208690: <http://trac.webkit.org/r208690>.
Comment 12 Mark Lam 2016-11-14 14:22:12 PST
Build fix for windows debug build landed in r208709: <http://trac.webkit.org/r208709>.