Bug 21769 - MessagePort should be GC protected if there are messages to be delivered
Summary: MessagePort should be GC protected if there are messages to be delivered
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Alexey Proskuryakov
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-10-21 02:39 PDT by Alexey Proskuryakov
Modified: 2008-10-21 23:08 PDT (History)
0 users

See Also:


Attachments
proposed fix (10.96 KB, patch)
2008-10-21 02:51 PDT, Alexey Proskuryakov
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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.