[webkit-reviews] review granted: [Bug 22397] Worker threads are not destroyed if running a tight loop : [Attachment 25347] proposed fix

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Alexey Proskuryakov <ap at webkit.org>'s
request for review:
Bug 22397: Worker threads are not destroyed if running a tight loop
https://bugs.webkit.org/show_bug.cgi?id=22397

Attachment 25347: proposed fix
https://bugs.webkit.org/attachment.cgi?id=25347&action=review

------- Additional Comments from Darin Adler <darin at apple.com>
> +    m_globalData->interpreter->setTimeoutTime(1);

What does this "1" mean? Is it a magic constant of some sort? 1 second perhaps?


> +    // The thread object may be already destroyed from notification now,
don't try to access "this".

I don't think this comment makes it clear enough that it was the assignment
m_workerContext = 0 that could trigger the notification. I had to study the
code for a while to understand why the previous line could access "this"
without a problem, until I realized that it was the previous line that was the
trigger.

> +    // FIXME: rudely killing the thread won't work when we allow nested
workers, because they will try to post notifications of their destruction.
> +    m_messageQueue.kill();

Please capitalize "rudely".

r=me