Bug 52447 – [Chromium] Notify embedder and crash WebCore process immediately in case of OOM in v8

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebCore Misc.

[Yury Semikhatsky]

Show OOM notification bar and crash render process immediately in case of OOM in v8. Current approach is to try to disable JavaScript in the page where the fatal error was detected and continue execution.

Corresponding Chromium bug: http://crbug.com/40521

[Yury Semikhatsky]

Created attachment 78942 [details]
Patch

[Yury Semikhatsky]

Chromium-side change: http://codereview.chromium.org/6006006/

[Dimitri Glazkov (Google)]

Comment on attachment 78942 [details]
Patch

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

ok with comments:

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

Can you explain why?

> Source/WebCore/ChangeLog:21
> +        * bindings/v8/V8AbstractEventListener.cpp:
> +        (WebCore::V8AbstractEventListener::invokeEventHandler):
> +        * bindings/v8/V8DOMWindowShell.cpp:
> +        (WebCore::handleFatalErrorInV8):
> +        (WebCore::V8DOMWindowShell::initContextIfNeeded):
> +        * bindings/v8/V8Proxy.cpp:
> +        (WebCore::V8Proxy::runScript):
> +        * bindings/v8/V8Proxy.h:
> +        * bindings/v8/WorkerContextExecutionProxy.cpp:
> +        (WebCore::WorkerContextExecutionProxy::initV8IfNeeded):
> +        (WebCore::WorkerContextExecutionProxy::runScript):
> +        * platform/chromium/ChromiumBridge.h:

I really prefer when the changes are documented (what/why) here to help reviewers and archeologists.

> WebKit/chromium/ChangeLog:11
> +        * public/WebKitClient.h:
> +        (WebKit::WebKitClient::notifyJSOutOfMemory):
> +        * src/ChromiumBridge.cpp:
> +        (WebCore::ChromiumBridge::notifyJSOutOfMemory):

And here too.

[Yury Semikhatsky]

Created attachment 79168 [details]
Patch that I'm going to land.

[Yury Semikhatsky]

(In reply to comment #3)
> (From update of attachment 78942 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=78942&action=review
> 
> ok with comments:
> 
> > Source/WebCore/ChangeLog:8
> > +        No new tests. (OOPS!)
> 
> Can you explain why?
> 
I'd love to write a test for this but I don't see a good way to cause OOM in a quick and reliable way. fast/js/string-concatenate-outofmemory.html once tried to test behavior after OOM but it was eventually disabled because of some problems with its implementation. I don't see other tests for this behavior.


> > Source/WebCore/ChangeLog:21
> > +        * bindings/v8/V8AbstractEventListener.cpp:
> > +        (WebCore::V8AbstractEventListener::invokeEventHandler):
> > +        * bindings/v8/V8DOMWindowShell.cpp:
> > +        (WebCore::handleFatalErrorInV8):
> > +        (WebCore::V8DOMWindowShell::initContextIfNeeded):
> > +        * bindings/v8/V8Proxy.cpp:
> > +        (WebCore::V8Proxy::runScript):
> > +        * bindings/v8/V8Proxy.h:
> > +        * bindings/v8/WorkerContextExecutionProxy.cpp:
> > +        (WebCore::WorkerContextExecutionProxy::initV8IfNeeded):
> > +        (WebCore::WorkerContextExecutionProxy::runScript):
> > +        * platform/chromium/ChromiumBridge.h:
> 
> I really prefer when the changes are documented (what/why) here to help reviewers and archeologists.
> 
Done.

> > WebKit/chromium/ChangeLog:11
> > +        * public/WebKitClient.h:
> > +        (WebKit::WebKitClient::notifyJSOutOfMemory):
> > +        * src/ChromiumBridge.cpp:
> > +        (WebCore::ChromiumBridge::notifyJSOutOfMemory):
> 
> And here too.
Done.

[Yury Semikhatsky]

Created attachment 80753 [details]
Rebaselined patch

[Adam Barth]

Comment on attachment 78942 [details]
Patch

Clearing r+ from obsolete patch.  What more needs to be done for this bug?  Does the newer patch need to be reviewed and/or landed?

[Yury Semikhatsky]

(In reply to comment #7)
> (From update of attachment 78942 [details])
> Clearing r+ from obsolete patch.  What more needs to be done for this bug?  Does the newer patch need to be reviewed and/or landed?

We need to figure out what the Chromium UI for the crashed tabs should look like. Until we come to an agreement on that topic this change doesn't make sense.

[Stephen Chenney]

Assuming fixed as this is an old issue. Reopen in crbug if that's not true.