[Webkit-unassigned] [Bug 25043] Race condition in MessagePort::clone()

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=25043





------- Comment #10 from atwilson at google.com  2009-06-16 17:57 PDT -------
>>  MessageChannel::MessageChannel(ScriptExecutionContext* context)
>
>* This instance also cannot be given a null context so it should take a
>ScriptExecutionContext& as well.

This is pre-existing code. We can address this in a followup patch.

>> WebCore/dom/default/MessagePortProxy.h
>I don't think dom is the right place for platform specific items.  Is that
>where you're going with the "default" directory?

I think this is OK - I'm following the pattern under WebCore/page.

>>        ScriptExecutionContext* m_scriptExecutionContext;
>I would make this a ScriptExecutionContext& since it can never be 0 and you can
>get rid of a bunch of asserts.

It can't be zero at construction time, true, but my intention is that it *can*
be set to zero later (I have a pending patch that does more optimal GC which
detaches the port from the ScriptExecutionContext when it is cloned to enable
it to be GC'd).

>In MessagePort.cpp, it is odd that m_queueIsOpen is not set to false after
>MessagePort::close is called.  Perhaps it should have a different name:
>m_started?  or even better m_enabled? (which is terminology from the spec).

Agreed that "started" is a better name - renamed this API (enabled is not
particularly descriptive). I do not set m_started to false when close() is
called, because the spec states that existing messages continue to be delivered
even though the port has been closed.

>>>
* the formatting for things like "channel.port2.onmessage = function(evt)" is
inconsistent.  (Sometimes you put the { at the end of the line and sometimes at
the beginning of the next line.  I've seen both in these js files.  It seems
like at the beginning of the next line is more consistent with WebKit in
general.)
<<<
I wasn't sure what the style was originally, so I looked through other
examples. It seems that the WebKit style differs depending on how the function
is defined.

When defining a top-level function, the brace should be on a line by itself:

function foo()
{
   ...do stuff...
}

When defining an inline function, it should be on the same line
  setTimeout(function() {
      ...do stuff...
  }, 100);

This is just what I've gathered from other test files and I've tried to mimic
that style. So I think the code is correctly formatted, but I'm happy to change
it if I'm wrong.

I've addressed the other comments locally - I'll upload a patch shortly


-- 
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.