[Webkit-unassigned] [Bug 25043] Race condition in MessagePort::clone()
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Tue Jun 9 11:29:49 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=25043
levin at chromium.org changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #31033|review? |review-
Flag| |
------- Comment #2 from levin at chromium.org 2009-06-09 11:29 PDT -------
(From update of attachment 31033)
Two initial comments:
1. This patch does a lot more than what the bug says.
2. It is massive. It would be easier (faster) to review if the individual
things were separate patches instead of doing so much in one patch.
One other metanote if you can hide the constructor of classes and instead
making them have a create method which passes back PassOwnPtr (or PassRefPtr if
that makes sense), that is preferred.
I think it would be good to deal with the MessagePortProxy not deriving from
MessagePortProxyBase isse before I look at it further so r- for that issue for
now.
Here's an initial pass just about style issues (I'll do a more in depth review
to understand it and make comments at that level later):
> diff --git a/JavaScriptCore/ChangeLog b/JavaScriptCore/ChangeLog
> index e3c19af..5affab7 100644
> --- a/JavaScriptCore/ChangeLog
> +++ b/JavaScriptCore/ChangeLog
> @@ -1,3 +1,13 @@
> +2009-06-07 Drew Wilson <atwilson at google.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Added support for multi-threaded MessagePorts.
> +
Needs link to bug.
> diff --git a/JavaScriptCore/wtf/MessageQueue.h b/JavaScriptCore/wtf/MessageQueue.h
> MessageQueue() : m_killed(false) {}
Ideally add space { }
> + bool appendAndCheckEmpty(const DataType&);
This name sounds awkward. Are there any similar methods in wtf (or WebKit)
that could be used as inspiration?
> + template<typename DataType>
> + inline bool MessageQueue<DataType>::appendAndCheckEmpty(const DataType& message)
> + {
> + MutexLocker lock(m_mutex);
> + bool empty = m_queue.isEmpty();
wasEmpty seems better especially when it is returned.
> diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
> index 85cc97f..add365a 100644
> --- a/LayoutTests/ChangeLog
> +++ b/LayoutTests/ChangeLog
> @@ -1,3 +1,24 @@
> +2009-06-07 Drew Wilson <atwilson at google.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Updated MessageChannel/MessagePorts tests to reflect latest spec (close event has been removed).
> + Added more tests of port cloning.
Link to bug.
Also some of these "Added" seem like they are copied from other files. It
would be good to acknowledge that.
> diff --git a/LayoutTests/fast/events/message-channel-gc-2.html-disabled b/LayoutTests/fast/events/message-channel-gc-2.html-disabled
> @@ -6,7 +6,7 @@ function gc()
> {
> if (window.GCController)
> return GCController.collect();
> -
> +
Remove added whitespace.
> for (var i = 0; i < 10000; i++) { // > force garbage collection (FF requires about 9K allocations before a collect)
I don't understand the ">" in the comment. (I see this was copied from another
place, but it still would be good to fix it if you don't understand it either.)
> diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog
> index 19da779..24b3860 100644
> --- a/WebCore/ChangeLog
> +++ b/WebCore/ChangeLog
> @@ -1,3 +1,87 @@
> +2009-06-07 Drew Wilson <atwilson at google.com>
> +
> + Reviewed by NOBODY (OOPS!).
> +
> + Removed obsolete MessagePort.startConversation(), active and onclose APIs.
Bug link
> +
> + Refactored MessagePortProxy into MessagePortProxyBase and a platform-dependent MessagePortProxy implementation. Modified APIs to simplify cross-process implementations by moving the messaging code entirely into the platform-dependent proxy.
Usually changelog comments are wrapped to something shorter (see other comments
in there).
> diff --git a/WebCore/dom/MessagePort.cpp b/WebCore/dom/MessagePort.cpp
> MessagePort::MessagePort(ScriptExecutionContext* scriptExecutionContext)
> : m_entangledPort(0)
> , m_queueIsOpen(false)
> - , m_scriptExecutionContext(scriptExecutionContext)
> - , m_pendingCloseEvent(false)
> {
> - if (scriptExecutionContext)
> - scriptExecutionContext->createdMessagePort(this);
> + ASSERT(scriptExecutionContext);
> + m_scriptExecutionContext = scriptExecutionContext;
Prefer the " , m_scriptExecutionContext(scriptExecutionContext)" style of
initialization where possible.
If a variable can never be NULL, use the & style of declaration.
> @@ -128,47 +63,44 @@ void MessagePort::postMessage(const String& message, ExceptionCode& ec)
>
> void MessagePort::postMessage(const String& message, MessagePort* dataPort, ExceptionCode& ec)
> {
> - if (!m_entangledPort || !m_scriptExecutionContext)
> + ASSERT(m_scriptExecutionContext);
> + if (!m_entangledPort)
> return;
>
> - RefPtr<MessagePort> newMessagePort;
> + PassOwnPtr<MessagePortProxyBase> proxy;
Is it possible to use a OwnPtr here? (For PassRefPtr they would never be
declarated like this. You would use a RefPtr instead.)
> diff --git a/WebCore/dom/MessagePortProxyBase.h b/WebCore/dom/MessagePortProxyBase.h
> + class MessagePortProxyBase {
Do you wish to allow this class to be copied? If no, privately derive from
Noncopyable (wtf/Noncopyable.h). (Think of this just like
DISALLOW_COPY_AND_ASSIGN.)
> + public:
> +
Typically there isn't a blank line here in WK code.
> +
> + MessagePortProxyBase(PassRefPtr<MessagePortProxy>);
> + ~MessagePortProxyBase();
Seems like these should be "protected:"
Typically there *is* a blank line here in WK code (right before
private:/protected:).
> + private:
> +
Typically there isn't a blank line here in WK code.
> diff --git a/WebCore/dom/default/MessagePortProxy.cpp b/WebCore/dom/default/MessagePortProxy.cpp
> +void MessagePortProxy::setRemotePort(MessagePort* port)
> +{
> + MutexLocker lock(m_mutex);
> + // Should never set port if it is already set.
> + if (port)
> + ASSERT(!m_remotePort);
"if assert" looks wierd to me at least.
What about
ASSERT(!port || !m_remotePort);
instead?
> +
> +void MessagePortProxy::postMessageToRemote(PassRefPtr<MessagePortProxyBase::EventData> msg)
"msg" -> "message" avoid abbreviations.
> +bool MessagePortProxy::isProxyFor(MessagePort* port)
> +{
> + RefPtr<MessagePortProxy> remote = entangledProxy();
> + if (!remote)
> + return false;
> + return port == remote->remotePort();
or
return remote && port == remote->remotePort()
your choice.
> +void MessagePortProxy::close()
> +{
> + RefPtr<MessagePortProxy> remote = entangledProxy();
> + if (remote) {
> + closeInternal();
> + remote->closeInternal();
> + }
Consider using an early return pattern (which is more preferred in general in
WK).
if (!remote)
return;
closeInternal();
remote->closeInternal();
> diff --git a/WebCore/dom/default/MessagePortProxy.h b/WebCore/dom/default/MessagePortProxy.h
> +
> + class MessagePort;
> +
> + // MessagePortProxy is a platform-dependent interface to the remote side of a message channel.
> + // This default implementation supports multiple threads running within a single process. Implementations for multi-process platforms should define these public APIs in their own platform-specific MessagePortProxy file.
> + // The goal of this implementation is to eliminate contention except when cloning or closing the port, so each side of the channel has its own separate mutex.
> + class MessagePortProxy : public ThreadSafeShared<MessagePortProxy> {
When there is a FooBase, Foo derives from FooBase but that pattern isn't
followed here. Why not? Do the names need to be changed?
> + public:
> + // Creates a channel entangling two ports.
> + static void createChannel(PassRefPtr<MessagePort>, PassRefPtr<MessagePort>);
Nice to add blank line here.
> + class MessagePortQueue : public ThreadSafeShared<MessagePortQueue>
> + {
{ is placed incorrectly.
> + private:
> + MessagePortQueue() {}
{ } add space.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
More information about the webkit-unassigned
mailing list