Bug 203990 – MessageEvent should tell its memory cost to GC

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Konstantin Kaefer]

Created attachment 383109 [details]
Memory leak with web workers

Safari leaks memory when sending objects with `postMessage`. It happens when the message isn't accessed in any way on the receiver's side. As soon as the message object is referenced, even without modifying or actually reading its contents, the memory leak goes away. This applies to both webworkers and MessageChannel.

In the attached reproduction HTML pages, memory use grows to several gigabytes in the span of a few seconds. Uncommenting the `message.data;` statement removes the memory leak.

[Konstantin Kaefer]

Created attachment 383110 [details]
Memory leak with MessageChannel

[Konstantin Kaefer]

A similar bug was reported in https://bugs.webkit.org/show_bug.cgi?id=194268.

The current bug occurs with or without buffer transfer.

[Radar WebKit Bug Importer]

<rdar://problem/57059365>

[youenn fablet]

Testing the MessageChannel repro page, page consumes up to 8GB of memory, then GC kicks in and it continues over and over. This seems fine, not sure whether we could try to optimise things here.

Similarly, the web workers page is not crashing, although it eats up to 30GB of memory until GC happens and memory is down to a few MBs and does not increase anymore.
Not exactly sure why this behaviour happens, there might be things we could improve.
This does not seem critical though.

[Konstantin Kaefer]

Thanks for looking into this. Using 8 GB of memory might work on a desktop machine, but it generally doesn't work well on iOS devices, in particular because they don't have swap space. While I generally don't observe page crashes on desktop Safari, I have seen a few on iOS.

[Yusuke Suzuki]

Created attachment 400990 [details]
Patch

[Mark Lam]

Comment on attachment 400990 [details]
Patch

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

LGTM but can you rebase the bindings results?  It's a bit difficult to see the changes to the emitted finishCreation()s.  It'd be easier to audit it from the bindings test results.  Thanks.

> Source/WebCore/ChangeLog:3
> +        Safari leaks memory of objects send through postMessage

/send/sent/?

> Source/WebCore/ChangeLog:11
> +           it does not tell this memory pressure to GC. This patch adds ReportExtraMemoryCost to MessageEvent.idl and memoryCost

I suggest /tell this/communicate/.

> Source/WebCore/ChangeLog:17
> +           But it can be also created from toJSNewlyCreated for Event through EventFactory. If the latter path is taken, we didn't properly

/also created/also be created/
/didn't properly/won't properly/

> Source/WebCore/ChangeLog:19
> +           IDL code should follow to this convention.

/follow to this/follow this/.

[Yusuke Suzuki]

Comment on attachment 400990 [details]
Patch

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

>> Source/WebCore/ChangeLog:11
>> +           it does not tell this memory pressure to GC. This patch adds ReportExtraMemoryCost to MessageEvent.idl and memoryCost
> 
> I suggest /tell this/communicate/.

Fixed.

>> Source/WebCore/ChangeLog:17
>> +           But it can be also created from toJSNewlyCreated for Event through EventFactory. If the latter path is taken, we didn't properly
> 
> /also created/also be created/
> /didn't properly/won't properly/

Fixed.

>> Source/WebCore/ChangeLog:19
>> +           IDL code should follow to this convention.
> 
> /follow to this/follow this/.

Fixed.

[Yusuke Suzuki]

Created attachment 400992 [details]
Patch

[Mark Lam]

Comment on attachment 400992 [details]
Patch

r=me

[Konstantin Kaefer]

It looks like this patch also addresses https://bugs.webkit.org/show_bug.cgi?id=204515 and fixes the exact problem described there.

[Yusuke Suzuki]

(In reply to Konstantin Kaefer from comment #11)
> It looks like this patch also addresses
> https://bugs.webkit.org/show_bug.cgi?id=204515 and fixes the exact problem
> described there.

Right! Nice.

[Yusuke Suzuki]

*** Bug 204515 has been marked as a duplicate of this bug. ***

[Alex Christensen]

Comment on attachment 400992 [details]
Patch

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

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3640
> +    m_memoryCost = computeMemoryCost();

Couldn't this go in the initializers instead of the body of the constructor?
Also, wouldn't it be better to compute this lazily, in case it is never requested?

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3647
> +    m_memoryCost = computeMemoryCost();

ditto

> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3679
> +    size_t cost = m_data.size();

Should we add sizeof(*this)?

[Yusuke Suzuki]

Comment on attachment 400992 [details]
Patch

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

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3640
>> +    m_memoryCost = computeMemoryCost();
> 
> Couldn't this go in the initializers instead of the body of the constructor?
> Also, wouldn't it be better to compute this lazily, in case it is never requested?

Lazy calculation does not work because this memoryCost function can be called concurrently from GC concurrent marking thread, and SerializedScriptValue's fields can be sometimes nulled after the creation.
If we call this function lazily, this can cause GC crash.

I think the current form is better since it is consistent to the other constructors. For example, in `SerializedScriptValue::SerializedScriptValue(Vector<uint8_t>&& buffer, const Vector ...)` constructor, we cannot call this function as a part of initializers since the body is calculating fields that computeMemoryCost relies on.
Since computeMemoryCost() depends on the other fields of SerializedScriptValue, putting this in body can prevent a bug like this.

SerializedScriptValue::SerializedScriptValue(Vector<uint8_t>&& buffer, std::unique_ptr<ArrayBufferContentsArray> arrayBufferContentsArray)
    : m_data(WTFMove(buffer))
    , m_arrayBufferContentsArray(WTFMove(arrayBufferContentsArray))
    , m_memoryCost(computeMemoryCost())
{
}

And if some change reorders fields,

SerializedScriptValue::SerializedScriptValue(Vector<uint8_t>&& buffer, std::unique_ptr<ArrayBufferContentsArray> arrayBufferContentsArray)
    : m_data(WTFMove(buffer))
    , m_memoryCost(computeMemoryCost())
    , m_arrayBufferContentsArray(WTFMove(arrayBufferContentsArray))
{
}

this leads to a bug.

>> Source/WebCore/bindings/js/SerializedScriptValue.cpp:3679
>> +    size_t cost = m_data.size();
> 
> Should we add sizeof(*this)?

It does not need to take it. For example, StringImpl::costDuringGC is not adding sizeof(StringImpl) to the result. 
memoryCost function does not need to be precise. The goal of this function is getting rough estimated size and reporting it to GC as a memory-pressure, and the critical part of this function is that we should report super-large-sized cost.

[EWS]

Committed r262581: <https://trac.webkit.org/changeset/262581>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 400992 [details].