Changeset 54292 in webkit


Ignore:
Timestamp:
Feb 3, 2010 12:14:38 PM (14 years ago)
Author:
eric@webkit.org
Message:

2010-02-03 Drew Wilson <​atwilson@chromium.org>

Reviewed by Alexey Proskuryakov.

SharedWorkerScriptLoader should not be an ActiveDOMObject
​https://bugs.webkit.org/show_bug.cgi?id=34513

Test: Existing tests suffice (fixes test downstream in Chrome).

  • workers/DefaultSharedWorkerRepository.cpp: (WebCore::SharedWorkerScriptLoader::SharedWorkerScriptLoader): Changed to no longer derive from ActiveDOMObject (handles its own refcounting). (WebCore::SharedWorkerScriptLoader::load): Now increments own refcount when a load is pending. (WebCore::SharedWorkerScriptLoader::notifyFinished): Changed to decrement refcount when load is complete.
  • workers/WorkerScriptLoaderClient.h: Documentation change about reliability of notifyFinished() when used from worker context.

2010-02-03 Drew Wilson <​atwilson@chromium.org>

Reviewed by Alexey Proskuryakov.

SharedWorkerScriptLoader should not be an ActiveDOMObject
​https://bugs.webkit.org/show_bug.cgi?id=34513

  • src/SharedWorkerRepository.cpp: (WebCore::SharedWorkerScriptLoader::SharedWorkerScriptLoader): Changed SharedWorkerScriptLoader to manage its own lifecycle without using ActiveDOMObject. (WebCore::SharedWorkerScriptLoader::parentContext): (WebCore::pendingLoaders): Now we manually track pending loads so we can shut them down when the parent context shuts down. (WebCore::SharedWorkerScriptLoader::contextDetached): Shuts down/frees any pending worker loads. (WebCore::SharedWorkerScriptLoader::~SharedWorkerScriptLoader): Marks the SharedWorker object as not having pending activity if there was a load active (handles case where load was pending when parent document exits). (WebCore::SharedWorkerScriptLoader::load): (WebCore::SharedWorkerRepository::documentDetached): Now calls SharedWorkerScriptLoader::contextDetached() to shutdown any pending worker loads.
Location:
trunk
Files:
5 edited

Legend:

Unmodified
Added
Removed

  • trunk/WebCore/ChangeLog

    r54291 r54292  
     12010-02-03  Drew Wilson  <atwilson@chromium.org>
     2
     3        Reviewed by Alexey Proskuryakov.
     4
     5        SharedWorkerScriptLoader should not be an ActiveDOMObject
     6        https://bugs.webkit.org/show_bug.cgi?id=34513
     7
     8        Test: Existing tests suffice (fixes test downstream in Chrome).
     9
     10        * workers/DefaultSharedWorkerRepository.cpp:
     11        (WebCore::SharedWorkerScriptLoader::SharedWorkerScriptLoader):
     12        Changed to no longer derive from ActiveDOMObject (handles its own refcounting).
     13        (WebCore::SharedWorkerScriptLoader::load):
     14        Now increments own refcount when a load is pending.
     15        (WebCore::SharedWorkerScriptLoader::notifyFinished):
     16        Changed to decrement refcount when load is complete.
     17        * workers/WorkerScriptLoaderClient.h:
     18        Documentation change about reliability of notifyFinished() when used from worker context.
     19
    1202010-02-03  Pavel Feldman  <pfeldman@chromium.org>
    221

  • trunk/WebCore/workers/DefaultSharedWorkerRepository.cpp

    r50427 r54292  
    249249
    250250// Loads the script on behalf of a worker.
    251 class SharedWorkerScriptLoader : public RefCounted<SharedWorkerScriptLoader>, public ActiveDOMObject, private WorkerScriptLoaderClient {
     251class SharedWorkerScriptLoader : public RefCounted<SharedWorkerScriptLoader>, private WorkerScriptLoaderClient {
    252252public:
    253253    SharedWorkerScriptLoader(PassRefPtr<SharedWorker>, PassOwnPtr<MessagePortChannel>, PassRefPtr<SharedWorkerProxy>);
    … …  
    265265
    266266SharedWorkerScriptLoader::SharedWorkerScriptLoader(PassRefPtr<SharedWorker> worker, PassOwnPtr<MessagePortChannel> port, PassRefPtr<SharedWorkerProxy> proxy)
    267     : ActiveDOMObject(worker->scriptExecutionContext(), this)
    268     , m_worker(worker)
     267    : m_worker(worker)
    269268    , m_port(port)
    270269    , m_proxy(proxy)
    … …  
    275274{
    276275    // Mark this object as active for the duration of the load.
    277     ASSERT(!hasPendingActivity());
    278276    m_scriptLoader = new WorkerScriptLoader();
    279     m_scriptLoader->loadAsynchronously(scriptExecutionContext(), url, DenyCrossOriginRequests, this);
    280 
    281     // Stay alive until the load finishes.
    282     setPendingActivity(this);
     277    m_scriptLoader->loadAsynchronously(m_worker->scriptExecutionContext(), url, DenyCrossOriginRequests, this);
     278
     279    // Stay alive (and keep the SharedWorker and JS wrapper alive) until the load finishes.
     280    this->ref();
    283281    m_worker->setPendingActivity(m_worker.get());
    284282}
    … …  
    286284void SharedWorkerScriptLoader::notifyFinished()
    287285{
     286    // FIXME: This method is not guaranteed to be invoked if we are loading from WorkerContext (see comment for WorkerScriptLoaderClient::notifyFinished()).
     287    // We need to address this before supporting nested workers.
     288
    288289    // Hand off the just-loaded code to the repository to start up the worker thread.
    289290    if (m_scriptLoader->failed())
    290291        m_worker->dispatchEvent(Event::create(eventNames().errorEvent, false, true));
    291292    else
    292         DefaultSharedWorkerRepository::instance().workerScriptLoaded(*m_proxy, scriptExecutionContext()->userAgent(m_scriptLoader->url()), m_scriptLoader->script(), m_port.release());
     293        DefaultSharedWorkerRepository::instance().workerScriptLoaded(*m_proxy, m_worker->scriptExecutionContext()->userAgent(m_scriptLoader->url()), m_scriptLoader->script(), m_port.release());
    293294
    294295    m_worker->unsetPendingActivity(m_worker.get());
    295     unsetPendingActivity(this); // This frees this object - must be the last action in this function.
     296    this->deref(); // This frees this object - must be the last action in this function.
    296297}
    297298

  • trunk/WebCore/workers/WorkerScriptLoaderClient.h

    r44726 r54292  
    3535    class WorkerScriptLoaderClient {
    3636    public:
     37        // FIXME: notifyFinished() is not currently guaranteed to be invoked if used from worker context and the worker shuts down in the middle of an operation.
     38        // This will cause leaks when we support nested workers.
    3739        virtual void notifyFinished() { }
    38        
     40
    3941    protected:
    4042        virtual ~WorkerScriptLoaderClient() { }

  • trunk/WebKit/chromium/ChangeLog

    r54283 r54292  
     12010-02-03  Drew Wilson  <atwilson@chromium.org>
     2
     3        Reviewed by Alexey Proskuryakov.
     4
     5        SharedWorkerScriptLoader should not be an ActiveDOMObject
     6        https://bugs.webkit.org/show_bug.cgi?id=34513
     7
     8        * src/SharedWorkerRepository.cpp:
     9        (WebCore::SharedWorkerScriptLoader::SharedWorkerScriptLoader):
     10        Changed SharedWorkerScriptLoader to manage its own lifecycle without using ActiveDOMObject.
     11        (WebCore::SharedWorkerScriptLoader::parentContext):
     12        (WebCore::pendingLoaders):
     13        Now we manually track pending loads so we can shut them down when the parent context shuts down.
     14        (WebCore::SharedWorkerScriptLoader::contextDetached):
     15        Shuts down/frees any pending worker loads.
     16        (WebCore::SharedWorkerScriptLoader::~SharedWorkerScriptLoader):
     17        Marks the SharedWorker object as not having pending activity if there was a load active (handles case where load was pending when parent document exits).
     18        (WebCore::SharedWorkerScriptLoader::load):
     19        (WebCore::SharedWorkerRepository::documentDetached):
     20        Now calls SharedWorkerScriptLoader::contextDetached() to shutdown any pending worker loads.
     21
    1222010-02-03  Alexander Pavlov  <apavlov@chromium.org>
    223

  • trunk/WebKit/chromium/src/SharedWorkerRepository.cpp

    r50838 r54292  
    6262
    6363// Callback class that keeps the SharedWorker and WebSharedWorker objects alive while loads are potentially happening, and also translates load errors into error events on the worker.
    64 class SharedWorkerScriptLoader : private WorkerScriptLoaderClient, private WebSharedWorker::ConnectListener, private ActiveDOMObject {
     64class SharedWorkerScriptLoader : private WorkerScriptLoaderClient, private WebSharedWorker::ConnectListener {
    6565public:
    6666    SharedWorkerScriptLoader(PassRefPtr<SharedWorker> worker, const KURL& url, const String& name, PassOwnPtr<MessagePortChannel> port, PassOwnPtr<WebSharedWorker> webWorker)
    67         : ActiveDOMObject(worker->scriptExecutionContext(), this)
    68         , m_worker(worker)
     67        : m_worker(worker)
    6968        , m_url(url)
    7069        , m_name(name)
    7170        , m_webWorker(webWorker)
    7271        , m_port(port)
     72        , m_loading(false)
    7373    {
    7474    }
    7575
     76    ~SharedWorkerScriptLoader();
    7677    void load();
    77     virtual void contextDestroyed();
     78    static void stopAllLoadersForContext(ScriptExecutionContext*);
     79
    7880private:
    7981    // WorkerScriptLoaderClient callback
    … …  
    8183
    8284    virtual void connected();
     85
     86    const ScriptExecutionContext* loadingContext() { return m_worker->scriptExecutionContext(); }
    8387
    8488    void sendConnect();
    … …  
    9094    OwnPtr<MessagePortChannel> m_port;
    9195    WorkerScriptLoader m_scriptLoader;
     96    bool m_loading;
    9297};
    9398
     99static Vector<SharedWorkerScriptLoader*>& pendingLoaders()
     100{
     101    AtomicallyInitializedStatic(Vector<SharedWorkerScriptLoader*>&, loaders = *new Vector<SharedWorkerScriptLoader*>);
     102    return loaders;
     103}
     104
     105void SharedWorkerScriptLoader::stopAllLoadersForContext(ScriptExecutionContext* context)
     106{
     107    // Walk our list of pending loaders and shutdown any that belong to this context.
     108    Vector<SharedWorkerScriptLoader*>& loaders = pendingLoaders();
     109    for (unsigned i = 0; i < loaders.size(); ) {
     110        SharedWorkerScriptLoader* loader = loaders[i];
     111        if (context == loader->loadingContext()) {
     112            loaders.remove(i);
     113            delete loader;
     114        } else
     115            i++;
     116    }
     117}
     118
     119SharedWorkerScriptLoader::~SharedWorkerScriptLoader()
     120{
     121    if (m_loading)
     122        m_worker->unsetPendingActivity(m_worker.get());
     123}
     124
    94125void SharedWorkerScriptLoader::load()
    95126{
     127    ASSERT(!m_loading);
    96128    // If the shared worker is not yet running, load the script resource for it, otherwise just send it a connect event.
    97129    if (m_webWorker->isStarted())
    98130        sendConnect();
    99     else
     131    else {
    100132        m_scriptLoader.loadAsynchronously(m_worker->scriptExecutionContext(), m_url, DenyCrossOriginRequests, this);
     133        // Keep the worker + JS wrapper alive until the resource load is complete in case we need to dispatch an error event.
     134        m_worker->setPendingActivity(m_worker.get());
     135        m_loading = true;
     136    }
    101137}
    102138
    … …  
    129165}
    130166
    131 void SharedWorkerScriptLoader::contextDestroyed()
    132 {
    133     ActiveDOMObject::contextDestroyed();
    134     delete this;
    135 }
    136 
    137167void SharedWorkerScriptLoader::connected()
    138168{
    … …  
    186216    if (repo)
    187217        repo->documentDetached(getId(document));
     218
     219    // Stop the creation of any pending SharedWorkers for this context.
     220    // FIXME: Need a way to invoke this for WorkerContexts as well when we support for nested workers.
     221    SharedWorkerScriptLoader::stopAllLoadersForContext(document);
    188222}
    189223
Note: See TracChangeset for help on using the changeset viewer.