Bug 91535 - [Chromium] Out of Memory is observed when a large object is passed to a Web Worker
Summary: [Chromium] Out of Memory is observed when a large object is passed to a Web W...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-07-17 13:32 PDT by Dmitry Titov
Modified: 2012-07-20 15:56 PDT (History)
8 users (show)

See Also:


Attachments
Proposed patch. (5.67 KB, patch)
2012-07-17 14:42 PDT, Dmitry Titov
no flags Details | Formatted Diff | Diff
Fixed style error. (5.67 KB, patch)
2012-07-17 14:51 PDT, Dmitry Titov
levin: review-
Details | Formatted Diff | Diff
Updated per cr comments (5.64 KB, patch)
2012-07-17 19:05 PDT, Dmitry Titov
levin: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch for landing (6.19 KB, patch)
2012-07-19 12:57 PDT, Dmitry Titov
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Fixed failure on page-cycler (6.52 KB, patch)
2012-07-20 15:43 PDT, Dmitry Titov
levin: review+
dimich: commit-queue-
Details | Formatted Diff | Diff
Fixed failure on page-cycler (5.84 KB, patch)
2012-07-20 15:49 PDT, Dmitry Titov
levin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Titov 2012-07-17 13:32:19 PDT
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.
Comment 1 Dmitry Titov 2012-07-17 14:42:00 PDT
Created attachment 152836 [details]
Proposed patch.
Comment 2 WebKit Review Bot 2012-07-17 14:45:14 PDT
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.
Comment 3 Dmitry Titov 2012-07-17 14:51:30 PDT
Created attachment 152837 [details]
Fixed style error.
Comment 4 David Levin 2012-07-17 15:09:16 PDT
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?
Comment 5 Dmitry Titov 2012-07-17 19:05:23 PDT
Created attachment 152901 [details]
Updated per cr comments
Comment 6 Dmitry Titov 2012-07-17 19:10:26 PDT
(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.
Comment 7 David Levin 2012-07-17 19:18:29 PDT
(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());
Comment 8 David Levin 2012-07-17 19:19:30 PDT
Comment on attachment 152901 [details]
Updated per cr comments

Please consider the assert.
Comment 9 Dmitry Titov 2012-07-18 18:56:19 PDT
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 :-(
Comment 10 WebKit Review Bot 2012-07-18 19:43:57 PDT
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
Comment 11 Adam Barth 2012-07-18 21:56:49 PDT
    The following ChangeLog files contain OOPS:

        trunk/Source/WebCore/ChangeLog

    Please don't ever say "OOPS" in a ChangeLog file.
Comment 12 Adam Barth 2012-07-18 21:57:13 PDT
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.
Comment 13 Dmitry Titov 2012-07-19 12:57:31 PDT
Created attachment 153328 [details]
patch for landing

oops indeed. Removed the OOPS, re-committing.
Comment 14 WebKit Review Bot 2012-07-19 13:00:38 PDT
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
Comment 15 Dmitry Titov 2012-07-19 14:56:23 PDT
Landed manually: http://trac.webkit.org/changeset/123149
Comment 16 Tony Chang 2012-07-19 16:41:26 PDT
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.
Comment 17 Dmitry Titov 2012-07-20 12:51:01 PDT
Reverted in: http://trac.webkit.org/changeset/123166
Comment 18 Dmitry Titov 2012-07-20 15:43:40 PDT
Created attachment 153610 [details]
Fixed failure on page-cycler
Comment 19 Dmitry Titov 2012-07-20 15:49:35 PDT
Created attachment 153611 [details]
Fixed failure on page-cycler
Comment 20 Dmitry Titov 2012-07-20 15:56:44 PDT
Landed: http://trac.webkit.org/changeset/123269