Bug 22397 - Worker threads are not destroyed if running a tight loop
Summary: Worker threads are not destroyed if running a tight loop
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-11-21 02:48 PST by Alexey Proskuryakov
Modified: 2008-11-21 23:43 PST (History)
1 user (show)

See Also:


Attachments
proposed fix (7.62 KB, patch)
2008-11-21 02:58 PST, Alexey Proskuryakov
no flags Details | Formatted Diff | Diff
proposed fix (7.72 KB, patch)
2008-11-21 04:30 PST, 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-11-21 02:48:50 PST
Worker threads need to be destroyed when their Worker object is destroyed. Currently, this doesn't happen if the thread in running a JS loop, because the event loop in not entered in this case.
Comment 1 Alexey Proskuryakov 2008-11-21 02:58:13 PST
Created attachment 25345 [details]
proposed fix
Comment 2 Alexey Proskuryakov 2008-11-21 04:30:06 PST
Created attachment 25347 [details]
proposed fix

Removed access to possibly already destroyed "this" object.
Comment 3 Darin Adler 2008-11-21 13:26:38 PST
Comment on attachment 25347 [details]
proposed fix

> +    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
Comment 4 Alexey Proskuryakov 2008-11-21 13:36:22 PST
(In reply to comment #3)
> (From update of attachment 25347 [details] [review])
> > +    m_globalData->interpreter->setTimeoutTime(1);
> 
> What does this "1" mean? Is it a magic constant of some sort? 1 second perhaps?

This is 1 millisecond, which is the minimal value that can be passed to this function (0 is magic, and means no timeout).
Comment 5 Alexey Proskuryakov 2008-11-21 23:43:25 PST
Committed revision 38689. Re-worded the comments a bit for clarity.