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.
<rdar://problem/29808005>
Created attachment 302043 [details] Patch
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
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.
Created attachment 302055 [details] Patch
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.
Comment on attachment 302055 [details] Patch Clearing flags on attachment: 302055 Committed r212609: <http://trac.webkit.org/changeset/212609>
All reviewed patches have been landed. Closing bug.
(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&&.
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.
(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.
(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.