Bug 168548 – Recursive MessagePort.postMessage() calls causes tab to become unresponsive

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Chris Dumez]

Recursive MessagePort.postMessage() calls causes tab to become unresponsive.

Test case: http://jsbin.com/vizujisijo/1/edit?html,output

Firefox and Chrome behave correctly and let the timer fire.

[Chris Dumez]

<rdar://problem/29808005>

[Chris Dumez]

Created attachment 302043 [details]
Patch

[Darin Adler]

Comment on attachment 302043 [details]
Patch

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

The use of the verb "get" in the function names in these classes goes against WebKit coding style traditions. We normally use "take" for this kind of operation.

> Source/WebCore/dom/MessagePort.cpp:152
> +    bool contextIsWorker = is<WorkerGlobalScope>(*m_scriptExecutionContext);

Not sure this is an important optimization.

> Source/WebCore/dom/MessagePortChannel.h:66
> +        class EventData {
> +            WTF_MAKE_FAST_ALLOCATED;
> +        public:
> +            EventData(RefPtr<SerializedScriptValue>&& message, std::unique_ptr<MessagePortChannelArray>&&);
> +
> +            RefPtr<SerializedScriptValue> message() { return m_message; }
> +            std::unique_ptr<MessagePortChannelArray> channels() { return WTFMove(m_channels); }
> +
> +        private:
> +            RefPtr<SerializedScriptValue> m_message;
> +            std::unique_ptr<MessagePortChannelArray> m_channels;
> +        };

This class was just moved, but I see problems with it:

1) this is just a struct, and there is no need to make it a class
2) the channels() function is a destructive move, but the name of the function does not make that clear; but irrelevant if we just change this into a struct
3) the message() function returns a RefPtr causing unnecessary reference count churn; should return a raw pointer or const RefPtr&, but irrelevant if we just change this into a struct

[Chris Dumez]

Comment on attachment 302043 [details]
Patch

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

> Source/WTF/wtf/MessageQueue.h:189
> +    inline auto MessageQueue<DataType>::getAllMessages() -> Deque<std::unique_ptr<DataType>>

Will use takeAllMessages() naming instead.

>> Source/WebCore/dom/MessagePort.cpp:152
>> +    bool contextIsWorker = is<WorkerGlobalScope>(*m_scriptExecutionContext);
> 
> Not sure this is an important optimization.

Seems unfortunate to do a virtual function call for every loop iteration, unless the compiler is somehow able to hoist the check out of the loop.

>> Source/WebCore/dom/MessagePortChannel.h:66
>> +        };
> 
> This class was just moved, but I see problems with it:
> 
> 1) this is just a struct, and there is no need to make it a class
> 2) the channels() function is a destructive move, but the name of the function does not make that clear; but irrelevant if we just change this into a struct
> 3) the message() function returns a RefPtr causing unnecessary reference count churn; should return a raw pointer or const RefPtr&, but irrelevant if we just change this into a struct

Ok, I'll use a struct.

[Chris Dumez]

Created attachment 302055 [details]
Patch

[Darin Adler]

Comment on attachment 302055 [details]
Patch

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

> Source/WebCore/dom/MessagePortChannel.h:59
> +            EventData(Ref<SerializedScriptValue>&& message, std::unique_ptr<MessagePortChannelArray>&& channels)
> +                : message(WTFMove(message))
> +                , channels(WTFMove(channels))
> +            { }

I guess this is needed on Windows? I’d expect we could just use aggregate initialization syntax on other platforms.

[WebKit Commit Bot]

Comment on attachment 302055 [details]
Patch

Clearing flags on attachment: 302055

Committed r212609: <http://trac.webkit.org/changeset/212609>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

(In reply to comment #6)
> Comment on attachment 302055 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302055&action=review
> 
> > Source/WebCore/dom/MessagePortChannel.h:59
> > +            EventData(Ref<SerializedScriptValue>&& message, std::unique_ptr<MessagePortChannelArray>&& channels)
> > +                : message(WTFMove(message))
> > +                , channels(WTFMove(channels))
> > +            { }
> 
> I guess this is needed on Windows? I’d expect we could just use aggregate
> initialization syntax on other platforms.

I could not get make_unique<EventData>(a, b) to work without this constructor.
MessageQueue requires me to pass a unique_ptr<EventData>&& instead of an EventData&&.

[Darin Adler]

Comment on attachment 302055 [details]
Patch

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

>>> Source/WebCore/dom/MessagePortChannel.h:59
>>> +            { }
>> 
>> I guess this is needed on Windows? I’d expect we could just use aggregate initialization syntax on other platforms.
> 
> I could not get make_unique<EventData>(a, b) to work without this constructor.
> MessageQueue requires me to pass a unique_ptr<EventData>&& instead of an EventData&&.

The syntax I would expect to work is make_unique<EventData>({ a, b }), with braces.

[Chris Dumez]

(In reply to comment #10)
> Comment on attachment 302055 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=302055&action=review
> 
> >>> Source/WebCore/dom/MessagePortChannel.h:59
> >>> +            { }
> >> 
> >> I guess this is needed on Windows? I’d expect we could just use aggregate initialization syntax on other platforms.
> > 
> > I could not get make_unique<EventData>(a, b) to work without this constructor.
> > MessageQueue requires me to pass a unique_ptr<EventData>&& instead of an EventData&&.
> 
> The syntax I would expect to work is make_unique<EventData>({ a, b }), with
> braces.

I tried dropping the constructor and making the following change:
-    bool wasEmpty = m_channel->m_outgoingQueue->appendAndCheckEmpty(std::make_unique<EventData>(WTFMove(message), WTFMove(channels)));
+    bool wasEmpty = m_channel->m_outgoingQueue->appendAndCheckEmpty(std::make_unique<EventData>({ WTFMove(message), WTFMove(channels) }));

This gives me an error too:
In file included from /Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/default/PlatformMessagePortChannel.cpp:1:
In file included from /Volumes/Data/WebKit/OpenSource/Source/WebCore/WebCorePrefix.h:89:
In file included from /Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.12.xctoolchain/usr/bin/../include/c++/v1/algorithm:628:
/Applications/Xcode.app/Contents/Developer/Toolchains/OSX10.12.xctoolchain/usr/bin/../include/c++/v1/memory:3141:32: error: no matching constructor for initialization of 'WebCore::MessagePortChannel::EventData'
/Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/default/PlatformMessagePortChannel.cpp:88:74: note: in instantiation of function template specialization 'std::__1::make_unique<WebCore::MessagePortChannel::EventData, WTF::RefPtr<WebCore::SerializedScriptValue>, std::__1::unique_ptr<WTF::Vector<std::__1::unique_ptr<WebCore::MessagePortChannel, std::__1::default_delete<WebCore::MessagePortChannel> >, 1, WTF::CrashOnOverflow, 16>, std::__1::default_delete<WTF::Vector<std::__1::unique_ptr<WebCore::MessagePortChannel, std::__1::default_delete<WebCore::MessagePortChannel> >, 1, WTF::CrashOnOverflow, 16> > > >' requested here
In file included from /Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/default/PlatformMessagePortChannel.cpp:33:
In file included from /Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/default/PlatformMessagePortChannel.h:33:
/Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/MessagePortChannel.h:55:16: note: candidate constructor (the implicit move constructor) not viable: requires 1 argument, but 2 were provided
/Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/MessagePortChannel.h:55:16: note: candidate constructor (the implicit copy constructor) not viable: requires 1 argument, but 2 were provided
/Volumes/Data/WebKit/OpenSource/Source/WebCore/dom/MessagePortChannel.h:55:16: note: candidate constructor (the implicit default constructor) not viable: requires 0 arguments, but 2 were provided
1 error generated.

[Darin Adler]

(In reply to comment #11)
> (In reply to comment #10)
> > >>> Source/WebCore/dom/MessagePortChannel.h:59
> > >>> +            { }
> > >> 
> > >> I guess this is needed on Windows? I’d expect we could just use aggregate initialization syntax on other platforms.
> > > 
> > > I could not get make_unique<EventData>(a, b) to work without this constructor.
> > > MessageQueue requires me to pass a unique_ptr<EventData>&& instead of an EventData&&.
> > 
> > The syntax I would expect to work is make_unique<EventData>({ a, b }), with
> > braces.

Oops, good point, I was wrong. This will work:

    make_unique<EventData>(EventData { a, b })

but relies on the compiler to optimize enough that the move has no additional cost.