Bug 164681 – Add debugging facility to limit the max single allocation size.

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Mark Lam]

This is useful for simulating memory allocation failures on resource constraint devices for testing purposes.

[Mark Lam]

Created attachment 294620 [details]
proposed patch.

[JF Bastien]

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.

[Mark Lam]

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

[JF Bastien]

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

[Mark Lam]

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

[Mark Lam]

Created attachment 294691 [details]
proposed patch.

[JF Bastien]

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?

[Mark Lam]

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.

[Keith Miller]

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.

[Mark Lam]

Created attachment 294710 [details]
patch for landing.

[Mark Lam]

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

[Mark Lam]

Build fix for windows debug build landed in r208709: <http://trac.webkit.org/r208709>.