Handle allocation failure with PartitionAllocReturnNull.

Handle allocation failure with PartitionAllocReturnNull.

if the browser is in OOM situation and PartitionAllocReturnNull is set,
it returns null instead of OOM crash.

BUG=374087
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=175091

[JungJik, 2014-05-20 10:52:03]

Handle allocation failure with PartitionAllocReturnNull.

if the browser is in OOM situation and PartitionAllocReturnNull is set, it
returns null instead of crash.

BUG=374087

[eseidel, 2014-05-29 00:09:12]

cevans or tsepez are the correct reviewers for partition alloc changes.

They should ideally set themselves up in WATCHLISTs.

[JungJik, 2014-05-29 02:25:55]

On 2014/05/29 00:09:12, eseidel wrote:
> cevans or tsepez are the correct reviewers for partition alloc changes.
> 
> They should ideally set themselves up in WATCHLISTs.

thanks eseidel, I should have cared this early.
In addition, about the issue, currently we set
maxTotalSizeOfDiscardableEntries(http://goo.gl/73bs2z) as 256MB.
I think it's high for all devices and this value should be adjusted for low end
devices.
Or the browser should consider the remain memory usage and adapt it to the
value.
otherwise when we move to the page which has lots of GIF, we will get a crash.

[Chris Evans, 2014-05-29 04:05:25]

Thanks for fixing my TODO :) You rock.

LGTM if you land with two minor tweaks.

https://codereview.chromium.org/295803005/diff/1/Source/wtf/PartitionAlloc.cpp
File Source/wtf/PartitionAlloc.cpp (right):

https://codereview.chromium.org/295803005/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:332: if (!superPage) {
We might as well make that UNLIKELY(!superPage). (The compiler vs. macro
expansion may require you to use UNLIKELY(superPage == 0), I can't quite
remember ;-)

https://codereview.chromium.org/295803005/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:683: if (!rawNewPage) {
We should make this UNLIKELY(!rawNewPage)

https://codereview.chromium.org/295803005/diff/1/Source/wtf/PartitionAlloc.cp...
Source/wtf/PartitionAlloc.cpp:684: if (returnNull)
I think the only way we can get rawNewPage == NULL is if returnNull is set.

So we don't need to check it again, or RELEASE_ASSERT().

The whole block could be just:

if (UNLIKELY(!rawNewPage)) {
    ASSERT(returnNull);
    return 0;
}

[JungJik, 2014-05-29 12:53:28]

On 2014/05/29 04:05:25, Chris Evans wrote:
> Thanks for fixing my TODO :) You rock.
> 
> LGTM if you land with two minor tweaks.
> 
> https://codereview.chromium.org/295803005/diff/1/Source/wtf/PartitionAlloc.cpp
> File Source/wtf/PartitionAlloc.cpp (right):
> 
>
https://codereview.chromium.org/295803005/diff/1/Source/wtf/PartitionAlloc.cp...
> Source/wtf/PartitionAlloc.cpp:332: if (!superPage) {
> We might as well make that UNLIKELY(!superPage). (The compiler vs. macro
> expansion may require you to use UNLIKELY(superPage == 0), I can't quite
> remember ;-)
> 
>
https://codereview.chromium.org/295803005/diff/1/Source/wtf/PartitionAlloc.cp...
> Source/wtf/PartitionAlloc.cpp:683: if (!rawNewPage) {
> We should make this UNLIKELY(!rawNewPage)
> 
>
https://codereview.chromium.org/295803005/diff/1/Source/wtf/PartitionAlloc.cp...
> Source/wtf/PartitionAlloc.cpp:684: if (returnNull)
> I think the only way we can get rawNewPage == NULL is if returnNull is set.
> 
> So we don't need to check it again, or RELEASE_ASSERT().
> 
> The whole block could be just:
> 
> if (UNLIKELY(!rawNewPage)) {
>     ASSERT(returnNull);
>     return 0;
> }

Thanks Chris Evans for LGTM.
I googled that you mentioned "(The compiler vs. macro may 
require you to use UNLIKELY(superPage == 0)".
However I couldn't find the reference about it.
Do you mean this? (http://goo.gl/GhnMfb)
Usually when the macro expands, the operator's priority 
is important. and '!' is higher than '=='.
So I use 'UNLIKELY(!superPage)'.  If I am wrong, 
please let me know.  And thanks again for the comment.

[Chris Evans, 2014-05-29 17:06:10]

On 2014/05/29 12:53:28, JungJik wrote:
> On 2014/05/29 04:05:25, Chris Evans wrote:
> > Thanks for fixing my TODO :) You rock.
> > 
> > LGTM if you land with two minor tweaks.
> > 
> >
https://codereview.chromium.org/295803005/diff/1/Source/wtf/PartitionAlloc.cpp
> > File Source/wtf/PartitionAlloc.cpp (right):
> > 
> >
>
https://codereview.chromium.org/295803005/diff/1/Source/wtf/PartitionAlloc.cp...
> > Source/wtf/PartitionAlloc.cpp:332: if (!superPage) {
> > We might as well make that UNLIKELY(!superPage). (The compiler vs. macro
> > expansion may require you to use UNLIKELY(superPage == 0), I can't quite
> > remember ;-)
> > 
> >
>
https://codereview.chromium.org/295803005/diff/1/Source/wtf/PartitionAlloc.cp...
> > Source/wtf/PartitionAlloc.cpp:683: if (!rawNewPage) {
> > We should make this UNLIKELY(!rawNewPage)
> > 
> >
>
https://codereview.chromium.org/295803005/diff/1/Source/wtf/PartitionAlloc.cp...
> > Source/wtf/PartitionAlloc.cpp:684: if (returnNull)
> > I think the only way we can get rawNewPage == NULL is if returnNull is set.
> > 
> > So we don't need to check it again, or RELEASE_ASSERT().
> > 
> > The whole block could be just:
> > 
> > if (UNLIKELY(!rawNewPage)) {
> >     ASSERT(returnNull);
> >     return 0;
> > }
> 
> Thanks Chris Evans for LGTM.
> I googled that you mentioned "(The compiler vs. macro may 
> require you to use UNLIKELY(superPage == 0)".
> However I couldn't find the reference about it.
> Do you mean this? (http://goo.gl/GhnMfb)
> Usually when the macro expands, the operator's priority 
> is important. and '!' is higher than '=='.
> So I use 'UNLIKELY(!superPage)'.  If I am wrong, 
> please let me know.  And thanks again for the comment.

LGTM :D

[Tom Sepez, 2014-05-29 17:20:25]

lgtm

[JungJik, 2014-05-29 22:09:25]

The CQ bit was checked by jungjik.lee@samsung.com

[commit-bot: I haz the power, 2014-05-29 22:10:36]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/jungjik.lee@samsung.com/295803005/20001

[commit-bot: I haz the power, 2014-05-30 00:59:33]

Change committed as 175091