Don't ignore OOM exceptions.

Don't ignore OOM exceptions.

A few years back, we wanted to show an infobar if V8 runs out of memory.
However, there are many places that don't check whether V8 is OOM and
just crash if they get empty handles back. Also, there isn't really much
we can savely do other than crash.

BUG=347124, 347724
R=dcarney@chromium.org,haraken@chromium.org,yurys@chromium.org

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

[haraken, 2014-02-26 22:45:04]

LGTM

Also you might want to consider removing handleOutOfMemory() in V8Binding.cpp.
It won't be necessary after this change.

[yurys, 2014-02-27 11:09:32]

Don't we want to show such infobar/some other indicator this time. Without it
the crash due to OOM will be indistinguishable from the user standpoint. Knowing
that a page has crashed due to OOM may be a valuable feedback for the site's
author.

Also, please coordinate this change with PMs as last time it was blocked on
their side if I remember correctly.

[jochen (gone - plz use gerrit), 2014-02-27 12:04:09]

On 2014/02/27 11:09:32, yurys wrote:
> Don't we want to show such infobar/some other indicator this time. Without it
> the crash due to OOM will be indistinguishable from the user standpoint.
Knowing
> that a page has crashed due to OOM may be a valuable feedback for the site's
> author.

It's not changing the behavior - we currently crash anyway.

Also, it was decided a while back that for security reasons, we should always
crash in OOM situations (e.g. malloc() will also just crash)


> Also, please coordinate this change with PMs as last time it was blocked on
> their side if I remember correctly.


which PM?

[yurys, 2014-02-27 12:24:25]

On 2014/02/27 12:04:09, jochen (OOO from March 1) wrote:
> On 2014/02/27 11:09:32, yurys wrote:
> > Don't we want to show such infobar/some other indicator this time. Without
it
> > the crash due to OOM will be indistinguishable from the user standpoint.
> Knowing
> > that a page has crashed due to OOM may be a valuable feedback for the site's
> > author.
> 
> It's not changing the behavior - we currently crash anyway.
> 
If a page exhausts V8's memory we will disable scripts in that page
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
and it may continue operating as if scripts were disabled in the browser. I
don't advocate this approach but in this case we don't get Aw, snap!
immediately. In case this is the only page in the render process the user may
not even notice that there was OOM.


> Also, it was decided a while back that for security reasons, we should always
> crash in OOM situations (e.g. malloc() will also just crash)
> 
Yeah, I know, OOM in case of malloc() have always lead to crash, but V8 crashes
were intentionally treated differently.

> 
> > Also, please coordinate this change with PMs as last time it was blocked on
> > their side if I remember correctly.
> 
> 
> which PM?
I don't remember who vetoed my patces (might be brakowski) and since it was
several years ago I cannot find it my gmail history :(

[jochen (gone - plz use gerrit), 2014-02-27 12:36:23]

On 2014/02/27 12:24:25, yurys wrote:
> On 2014/02/27 12:04:09, jochen (OOO from March 1) wrote:
> > On 2014/02/27 11:09:32, yurys wrote:
> > > Don't we want to show such infobar/some other indicator this time. Without
> it
> > > the crash due to OOM will be indistinguishable from the user standpoint.
> > Knowing
> > > that a page has crashed due to OOM may be a valuable feedback for the
site's
> > > author.
> > 
> > It's not changing the behavior - we currently crash anyway.
> > 
> If a page exhausts V8's memory we will disable scripts in that page
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> and it may continue operating as if scripts were disabled in the browser. I
> don't advocate this approach but in this case we don't get Aw, snap!
> immediately. In case this is the only page in the render process the user may
> not even notice that there was OOM.

That's the intention, but in fact, we just crash. Either we OOM inside V8, then
V8 invokes V8::FatalProcessOutOfMemory() unconditionally, or we OOM during an V8
API call, then all but the three locations that actually invoke
handleOutOfMemory() will just crash (see the referenced bug).

> 
> 
> > Also, it was decided a while back that for security reasons, we should
always
> > crash in OOM situations (e.g. malloc() will also just crash)
> > 
> Yeah, I know, OOM in case of malloc() have always lead to crash, but V8
crashes
> were intentionally treated differently.
> 
> > 
> > > Also, please coordinate this change with PMs as last time it was blocked
on
> > > their side if I remember correctly.
> > 
> > 
> > which PM?
> I don't remember who vetoed my patces (might be brakowski) and since it was
> several years ago I cannot find it my gmail history :(

[yurys, 2014-02-27 13:42:30]

On 2014/02/27 12:36:23, jochen (OOO from March 1) wrote:
> On 2014/02/27 12:24:25, yurys wrote:
> > On 2014/02/27 12:04:09, jochen (OOO from March 1) wrote:
> > > On 2014/02/27 11:09:32, yurys wrote:
> > > > Don't we want to show such infobar/some other indicator this time.
Without
> > it
> > > > the crash due to OOM will be indistinguishable from the user standpoint.
> > > Knowing
> > > > that a page has crashed due to OOM may be a valuable feedback for the
> site's
> > > > author.
> > > 
> > > It's not changing the behavior - we currently crash anyway.
> > > 
> > If a page exhausts V8's memory we will disable scripts in that page
> >
>
(https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...)
> > and it may continue operating as if scripts were disabled in the browser. I
> > don't advocate this approach but in this case we don't get Aw, snap!
> > immediately. In case this is the only page in the render process the user
may
> > not even notice that there was OOM.
> 
> That's the intention, but in fact, we just crash. Either we OOM inside V8,
then
> V8 invokes V8::FatalProcessOutOfMemory() unconditionally, or we OOM during an
V8
> API call, then all but the three locations that actually invoke
> handleOutOfMemory() will just crash (see the referenced bug).
> 
Since scripting is disabled for the page there should no V8 API calls. They may
well happen though as e.g. several pages may share same process and scripting
will be disabled only in one of them.

[jochen (gone - plz use gerrit), 2014-03-12 15:00:49]

The CQ bit was checked by jochen@chromium.org

[commit-bot: I haz the power, 2014-03-12 15:00:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/jochen@chromium.org/181843003/40001

[jochen (gone - plz use gerrit), 2014-03-12 15:01:21]

Glen approved the ui change.

[commit-bot: I haz the power, 2014-03-12 16:11:47]

Change committed as 169029