Bug 91535 – [Chromium] Out of Memory is observed when a large object is passed to a Web Worker

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

[Dmitry Titov]

When a large object (like an ImageData) is serialized and passed into Web Worker, the MessageEvent that is created for the worker context is small (in V8 mind) but it owns a SerializedScriptValue which can be large. Since V8 does not know about real size of the allocated memeory, it is not scheduling GC for quite a while which can cause OOM in the renderer. 

See http://code.google.com/p/chromium/issues/detail?id=132769 for actual repro case.

The fix is to inform V8 about the actual size of the memory owned by the MessageEvent in that case. Patch is coming.

[Dmitry Titov]

Created attachment 152836 [details]
Proposed patch.

[WebKit Review Bot]

Attachment 152836 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1
Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2328:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2335:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
Total errors found: 2 in 4 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dmitry Titov]

Created attachment 152837 [details]
Fixed style error.

[David Levin]

Comment on attachment 152837 [details]
Fixed style error.

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

> Source/WebCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

Please remove.

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2332
> +    if ((m_externallyAllocatedMemory = m_data.length()))

Whenever I see "if (a=b)" my bug sense goes off. I know you put in the double (( to show it as intention, but it feels like it would be clearer just to do it on another line.

Also there is no need for the "if" really.

m_externallyAllocatedMemory = m_data.length();
v8::V8::AdjustAmountOfExternalAllocatedMemory(m_externallyAllocatedMemory);

> Source/WebCore/bindings/v8/SerializedScriptValue.cpp:2338
> +    if (m_externallyAllocatedMemory)

No need for if here.

> Source/WebCore/dom/MessageEvent.cpp:77
> +    if (m_dataAsSerializedScriptValue)

Can we do this any other place? So that everyone who uses SerializedScriptValue will get this for free without having to know to do this.

If we do need to do something like this, is it possible to add asserts in key places (methods?) in SerializedScriptValue to make sure that this function is called when appropriate?

[Dmitry Titov]

Created attachment 152901 [details]
Updated per cr comments

[Dmitry Titov]

(In reply to comment #4)

Fixed everything, except:

> 
> > Source/WebCore/dom/MessageEvent.cpp:77
> > +    if (m_dataAsSerializedScriptValue)
> 
> Can we do this any other place? So that everyone who uses SerializedScriptValue will get this for free without having to know to do this.
> 
> If we do need to do something like this, is it possible to add asserts in key places (methods?) in SerializedScriptValue to make sure that this function is called when appropriate?

I thought about it and don't see a better solution... The SerializedScriptValue is created on the source thread, then passed through to the Worker - and then finally a MessageEvent is created that has actual JS wrapper and is under GC lifetime control... Also, only at that last moment SSV is in the Worker's V8 context, so the memory adjustment happens for the right heap.

I don't like using USE(V8) things, but we don't have currently a v8-specific way to customize constructing MessageEvent. The code under this ifdef is tiny, so it seems it isn't worth to create one...

I'd be glad to receive better suggestions.

[David Levin]

(In reply to comment #6)
> (In reply to comment #4)
> 
> Fixed everything, except:
> 

> > If we do need to do something like this, is it possible to add asserts in key places (methods?) in SerializedScriptValue to make sure that this function is called when appropriate?

What about adding an assert in SerializedScriptValue::deserialize?

Something like this:
  // Verify that registerMemoryAllocatedWithCurrentScriptContext was called.
  ASSERT(m_externallyAllocatedMemory || !m_data.length());

[David Levin]

Comment on attachment 152901 [details]
Updated per cr comments

Please consider the assert.

[Dmitry Titov]

I looked at the assert and tried couple of variants... It seems to satisfy it, too many cases unrelated to passing SerializedScriptData will have to be modified and cluttered, w/o obvious benefit and in some cases incorrectly adding the memory pressure. The SerializedScriptValue is used in things like History for example...

So left it w/o assert :-(

[WebKit Review Bot]

Comment on attachment 152901 [details]
Updated per cr comments

Rejecting attachment 152901 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2

Last 500 characters of output:
Kit/chromium/third_party/yasm/source/patched-yasm --revision 134927 --non-interactive --force --accept theirs-conflict --ignore-externals' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
48>At revision 134927.

________ running '/usr/bin/python tools/clang/scripts/update.py --mac-only' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'

________ running '/usr/bin/python gyp_webkit' in '/mnt/git/webkit-commit-queue/Source/WebKit/chromium'
Updating webkit projects from gyp files...

Full output: http://queues.webkit.org/results/13273719

[Adam Barth]

    The following ChangeLog files contain OOPS:

        trunk/Source/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.

[Adam Barth]

Comment on attachment 152901 [details]
Updated per cr comments

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

> Source/WebCore/ChangeLog:8
> +        Additional information of the change such as approach, rationale. Please add per-function descriptions below (OOPS!).

This line is causing the trouble.

[Dmitry Titov]

Created attachment 153328 [details]
patch for landing

oops indeed. Removed the OOPS, re-committing.

[WebKit Review Bot]

Comment on attachment 153328 [details]
patch for landing

Rejecting attachment 153328 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 1

ERROR: /mnt/git/webkit-commit-queue/Source/WebCore/ChangeLog neither lists a valid reviewer nor contains the string "Unreviewed" or "Rubber stamp" (case insensitive).

Full output: http://queues.webkit.org/results/13303043

[Dmitry Titov]

Landed manually: http://trac.webkit.org/changeset/123149

[Tony Chang]

Is it possible that this change caused page_cycler_indexeddb to start failing?

http://build.chromium.org/p/chromium.webkit/waterfall?builder=Linux+Perf&builder=Win7+Perf

http://build.chromium.org/p/chromium.webkit/builders/Win7%20Perf/builds/561
http://build.chromium.org/p/chromium.webkit/builders/Linux%20Perf/builds/15723

I don't know much about this test, but this change seems the mostly likely since the other changes around it are Qt specific.

[Dmitry Titov]

Reverted in: http://trac.webkit.org/changeset/123166

[Dmitry Titov]

Created attachment 153610 [details]
Fixed failure on page-cycler

[Dmitry Titov]

Created attachment 153611 [details]
Fixed failure on page-cycler

[Dmitry Titov]

Landed: http://trac.webkit.org/changeset/123269