Bug 21769

Summary: MessagePort should be GC protected if there are messages to be delivered
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: WebCore JavaScriptAssignee: Alexey Proskuryakov <ap>
Status: RESOLVED FIXED    
Severity: Normal    
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Mac   
OS: OS X 10.5   
Attachments:
Description Flags
proposed fix darin: review+

Description Alexey Proskuryakov 2008-10-21 02:39:08 PDT
Even if a message port and its entangled port both become inaccessible from JS, outstanding messages should be delivered, as there may be listeners registered.

This is kinda obvious, as it is too easy to write JS code that fails randomly if GC kicks in just in time for messages to be lost. But HTML5 was just changed to say this.

Patch forthcoming.
Comment 1 Alexey Proskuryakov 2008-10-21 02:51:51 PDT
Created attachment 24538 [details]
proposed fix
Comment 2 Darin Adler 2008-10-21 10:39:05 PDT
Comment on attachment 24538 [details]
proposed fix

+        // Warning: using the below methods is risky, as the queue state may change by the time they return. Extra caution is advised.

I like what this comment is warning about, but I'm not fond of "risky" and "caution" when describing how to use functions. And "queue state" is also pretty vague. It would be better if this was concrete about what the consideration is. I think that "be careful" doesn't help as much as specifics. Maybe a comment more like "the result of isEmpty() is only useful if there's some other guarantee that no one is going to add messages to the queue on another thread" would be one good example. I think that's better than talking about risk and caution. The comment in MessagePort::hasPendingActivity is nice and clear on this point.

r=me
Comment 3 Alexey Proskuryakov 2008-10-21 23:08:42 PDT
(In reply to comment #2)
> I like what this comment is warning about, but I'm not fond of "risky" and
> "caution" when describing how to use functions.

OK. I changed the warning for isEmpty(), but didn't find a good way to specifically warn about tryGetMessage()/killed(). Given that those are normally used for polling, maybe it's not important to warn about these.