Bug 22380 - Fix WorkerContext refcounting
Summary: Fix WorkerContext refcounting
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-20 05:05 PST by Alexey Proskuryakov
Modified: 2008-11-20 12:37 PST (History)
0 users

See Also:


Attachments
proposed patch (2.94 KB, patch)
2008-11-20 05:08 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-20 05:05:16 PST
WorkerContext is not currently destroyed when its thread finishes, because its JS wrapper holds a reference to it. We need to clear destroy the JS heap before the thread finishes.
Comment 1 Alexey Proskuryakov 2008-11-20 05:08:18 PST
Created attachment 25311 [details]
proposed patch
Comment 2 Darin Adler 2008-11-20 09:24:59 PST
Comment on attachment 25311 [details]
proposed patch

> +    , m_script(new WorkerScriptController(this))

For future consideration: We want to try to avoid bare "new" calls almost everywhere. For single-owner classes, I think a create function that returns auto_ptr is a good design. We can change OwnPtr so it has a constructor that takes an auto_ptr, if it doesn't already have one. For reference counted classes a create function that returns PassRefPtr is obviously our approach.

> +    workerContext->clearScript();
> +    ASSERT(workerContext->refCount() == 1);

I think you should add a comment here explaining why there's a guarantee that no one else is holding a reference. Our usual aim with reference-counted objects is to make the "shutdown" operation be something other than destruction, and always allow someone to prolong the lifetime of the object a bit, just to keep a pointer valid. I know we don't always use reference counting that way, but I think it merits a comment when we're not.

By the way, can you use the hasOneRef() function here instead?

r=me
Comment 3 Alexey Proskuryakov 2008-11-20 12:37:08 PST
Committed revision 38626.

Added a comment that hopefully clarifies the behavior a little, and changed to hasOneRef, which is better.