Changeset 150867 in webkit


Ignore:
Timestamp:
May 28, 2013 10:29:54 PM (11 years ago)
Author:
Nate Chapin
Message:

Crash in WebCore::SubresourceLoader::releaseResources when connection fails
​https://bugs.webkit.org/show_bug.cgi?id=87743

Don't do anything complicated in SubresourceLoader::releaseResources(),
just clear variables. With this patch, releaseResources() will still
assert in debug builds if it is called twice, but it will safely execute
in release.

Reviewed by Darin Adler.

  • loader/ResourceLoader.cpp:

(WebCore::ResourceLoader::cleanupForError): Pull shared cleanup code out of didFail()

and cancel() into a helper.

(WebCore::ResourceLoader::cancel): Merge a couple variables into an enum, check for

reentrancy from within didCancel().

  • loader/ResourceLoader.h: Replace m_calledWillCancel and m_cancelled with an enum.
  • loader/SubresourceLoader.cpp:

(WebCore::SubresourceLoader::didFinishLoading): Don't call ResourceLoader::didFinishLoading(),

put finish() in the middle of the process.

(WebCore::SubresourceLoader::didFail): Don't call ResourceLoader::didFail(), put finish()

in the middle of the process.

(WebCore::SubresourceLoader::didCancel):
(WebCore::SubresourceLoader::notifyDone): Do the non-trivial work previous done in releaseResources(),

most importantly calling loadDone().

(WebCore::SubresourceLoader::releaseResources): Only do simple variable clearing here.

  • loader/SubresourceLoader.h:

(SubresourceLoader):

  • loader/cache/CachedResource.cpp: Split stopLoading() into cancelLoad() (which notifies clients)

and clearLoader() (which just nulls m_loader).

  • loader/cache/CachedResource.h:
  • loader/chromium/ResourceLoaderChromium.cpp:
Location:
trunk/Source/WebCore
Files:
7 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r150865 r150867  
     12013-05-28  Nate Chapin  <japhet@chromium.org>
     2
     3        Crash in WebCore::SubresourceLoader::releaseResources when connection fails
     4        https://bugs.webkit.org/show_bug.cgi?id=87743
     5
     6        Don't do anything complicated in SubresourceLoader::releaseResources(),
     7        just clear variables. With this patch, releaseResources() will still
     8        assert in debug builds if it is called twice, but it will safely execute
     9        in release.
     10
     11        Reviewed by Darin Adler.
     12
     13        * loader/ResourceLoader.cpp:
     14        (WebCore::ResourceLoader::cleanupForError): Pull shared cleanup code out of didFail()
     15            and cancel() into a helper.
     16        (WebCore::ResourceLoader::cancel): Merge a couple variables into an enum, check for
     17            reentrancy from within didCancel().
     18        * loader/ResourceLoader.h: Replace m_calledWillCancel and m_cancelled with an enum.
     19        * loader/SubresourceLoader.cpp:
     20        (WebCore::SubresourceLoader::didFinishLoading): Don't call ResourceLoader::didFinishLoading(),
     21            put finish() in the middle of the process.
     22        (WebCore::SubresourceLoader::didFail): Don't call ResourceLoader::didFail(), put finish()
     23            in the middle of the process.
     24        (WebCore::SubresourceLoader::didCancel):
     25        (WebCore::SubresourceLoader::notifyDone): Do the non-trivial work previous done in releaseResources(),
     26            most importantly calling loadDone().
     27        (WebCore::SubresourceLoader::releaseResources): Only do simple variable clearing here.
     28        * loader/SubresourceLoader.h:
     29        (SubresourceLoader):
     30        * loader/cache/CachedResource.cpp: Split stopLoading() into cancelLoad() (which notifies clients)
     31            and clearLoader() (which just nulls m_loader).
     32        * loader/cache/CachedResource.h:
     33        * loader/chromium/ResourceLoaderChromium.cpp:
     34
    1352013-05-28  Seokju Kwon  <seokju.kwon@gmail.com>
    236

  • trunk/Source/WebCore/loader/ResourceLoader.cpp

    r150104 r150867  
    6363    , m_identifier(0)
    6464    , m_reachedTerminalState(false)
    65     , m_calledWillCancel(false)
    66     , m_cancelled(false)
    6765    , m_notifiedLoadComplete(false)
     66    , m_cancellationStatus(NotCancelled)
    6867    , m_defersLoading(frame->page()->defersLoading())
    6968    , m_options(options)
    … …  
    333332    // If the load has been cancelled by a delegate in response to didFinishLoad(), do not release
    334333    // the resources a second time, they have been released by cancel.
    335     if (m_cancelled)
     334    if (wasCancelled())
    336335        return;
    337336    releaseResources();
    … …  
    342341    // If load has been cancelled after finishing (which could happen with a
    343342    // JavaScript that changes the window location), do nothing.
    344     if (m_cancelled)
     343    if (wasCancelled())
    345344        return;
    346345    ASSERT(!m_reachedTerminalState);
    … …  
    355354void ResourceLoader::didFail(const ResourceError& error)
    356355{
    357     if (m_cancelled)
     356    if (wasCancelled())
    358357        return;
    359358    ASSERT(!m_reachedTerminalState);
    … …  
    363362    RefPtr<ResourceLoader> protector(this);
    364363
     364    cleanupForError(error);
     365    releaseResources();
     366}
     367
     368void ResourceLoader::cleanupForError(const ResourceError& error)
     369{
    365370    if (FormData* data = m_request.httpBody())
    366371        data->removeGeneratedFilesIfNeeded();
    367372
    368     if (!m_notifiedLoadComplete) {
    369         m_notifiedLoadComplete = true;
    370         if (m_options.sendLoadCallbacks == SendCallbacks)
    371             frameLoader()->notifier()->didFailToLoad(this, error);
    372     }
    373 
    374     releaseResources();
     373    if (m_notifiedLoadComplete)
     374        return;
     375    m_notifiedLoadComplete = true;
     376    if (m_options.sendLoadCallbacks == SendCallbacks && m_identifier)
     377        frameLoader()->notifier()->didFailToLoad(this, error);
    375378}
    376379
    … …  
    402405    // If we re-enter cancel() from inside willCancel(), we want to pick up from where we left
    403406    // off without re-running willCancel()
    404     if (!m_calledWillCancel) {
    405         m_calledWillCancel = true;
     407    if (m_cancellationStatus == NotCancelled) {
     408        m_cancellationStatus = CalledWillCancel;
    406409       
    407410        willCancel(nonNullError);
    … …  
    410413    // If we re-enter cancel() from inside didFailToLoad(), we want to pick up from where we
    411414    // left off without redoing any of this work.
    412     if (!m_cancelled) {
    413         m_cancelled = true;
    414        
    415         if (FormData* data = m_request.httpBody())
    416             data->removeGeneratedFilesIfNeeded();
     415    if (m_cancellationStatus == CalledWillCancel) {
     416        m_cancellationStatus = Cancelled;
    417417
    418418        if (m_handle)
    … …  
    424424            m_handle = 0;
    425425        }
    426 
    427         if (m_options.sendLoadCallbacks == SendCallbacks && m_identifier && !m_notifiedLoadComplete)
    428             frameLoader()->notifier()->didFailToLoad(this, nonNullError);
     426        cleanupForError(nonNullError);
    429427    }
    430428
    … …  
    435433
    436434    didCancel(nonNullError);
    437            
     435
     436    if (m_cancellationStatus == FinishedCancel)
     437        return;
     438    m_cancellationStatus = FinishedCancel;
     439
    438440    releaseResources();
    439441}

  • trunk/Source/WebCore/loader/ResourceLoader.h

    r149303 r150867  
    155155
    156156    void didFinishLoadingOnePart(double finishTime);
    157 
    158     bool cancelled() const { return m_cancelled; }
     157    void cleanupForError(const ResourceError&);
     158
     159    bool wasCancelled() const { return m_cancellationStatus >= Cancelled; }
    159160
    160161    void didReceiveDataOrBuffer(const char*, int, PassRefPtr<SharedBuffer>, long long encodedDataLength, DataPayloadType);
    … …  
    178179
    179180    bool m_reachedTerminalState;
    180     bool m_calledWillCancel;
    181     bool m_cancelled;
    182181    bool m_notifiedLoadComplete;
     182
     183    enum CancellationStatus {
     184        NotCancelled,
     185        CalledWillCancel,
     186        Cancelled,
     187        FinishedCancel
     188    };
     189    CancellationStatus m_cancellationStatus;
    183190
    184191    bool m_defersLoading;

  • trunk/Source/WebCore/loader/SubresourceLoader.cpp

    r148932 r150867  
    281281    m_resource->setLoadFinishTime(finishTime);
    282282    m_resource->data(resourceData(), true);
     283
     284    if (wasCancelled())
     285        return;
    283286    m_resource->finish();
    284     ResourceLoader::didFinishLoading(finishTime);
     287    ASSERT(!reachedTerminalState());
     288    didFinishLoadingOnePart(finishTime);
     289    notifyDone();
     290    if (reachedTerminalState())
     291        return;
     292    releaseResources();
    285293}
    286294
    … …  
    298306        memoryCache()->revalidationFailed(m_resource);
    299307    m_resource->setResourceError(error);
    300     m_resource->error(CachedResource::LoadError);
    301308    if (!m_resource->isPreloaded())
    302309        memoryCache()->remove(m_resource);
    303     ResourceLoader::didFail(error);
     310    m_resource->error(CachedResource::LoadError);
     311    cleanupForError(error);
     312    notifyDone();
     313    if (reachedTerminalState())
     314        return;
     315    releaseResources();
    304316}
    305317
    … …  
    319331}
    320332
     333void SubresourceLoader::didCancel(const ResourceError&)
     334{
     335    m_resource->cancelLoad();
     336    notifyDone();
     337}
     338
     339void SubresourceLoader::notifyDone()
     340{
     341    if (reachedTerminalState())
     342        return;
     343
     344    m_requestCountTracker.clear();
     345    m_documentLoader->cachedResourceLoader()->loadDone(m_resource);
     346    if (reachedTerminalState())
     347        return;
     348    m_documentLoader->removeSubresourceLoader(this);
     349}
     350
    321351void SubresourceLoader::releaseResources()
    322352{
    323353    ASSERT(!reachedTerminalState());
    324     if (m_state != Uninitialized) {
    325         m_requestCountTracker.clear();
    326         m_documentLoader->cachedResourceLoader()->loadDone(m_resource);
    327         if (reachedTerminalState())
    328             return;
    329         m_resource->stopLoading();
    330         m_documentLoader->removeSubresourceLoader(this);
    331     }
     354    if (m_state != Uninitialized)
     355        m_resource->clearLoader();
     356    m_resource = 0;
    332357    ResourceLoader::releaseResources();
    333358}

  • trunk/Source/WebCore/loader/SubresourceLoader.h

    r148921 r150867  
    6565    virtual void didFail(const ResourceError&) OVERRIDE;
    6666    virtual void willCancel(const ResourceError&) OVERRIDE;
    67     virtual void didCancel(const ResourceError&) OVERRIDE { }
     67    virtual void didCancel(const ResourceError&) OVERRIDE;
    6868
    6969#if USE(NETWORK_CFDATA_ARRAY_CALLBACK)
    … …  
    7777
    7878    void didReceiveDataOrBuffer(const char*, int, PassRefPtr<SharedBuffer>, long long encodedDataLength, DataPayloadType);
     79
     80    void notifyDone();
    7981
    8082    enum SubresourceLoaderState {

  • trunk/Source/WebCore/loader/cache/CachedResource.cpp

    r150863 r150867  
    381381    checkNotify();
    382382}
     383   
     384void CachedResource::cancelLoad()
     385{
     386    if (!isLoading())
     387        return;
     388
     389    setStatus(LoadError);
     390    setLoading(false);
     391    checkNotify();
     392}
    383393
    384394void CachedResource::finish()
    … …  
    451461}
    452462
    453 void CachedResource::stopLoading()
    454 {
    455     ASSERT(m_loader);           
     463void CachedResource::clearLoader()
     464{
     465    ASSERT(m_loader);
    456466    m_loader = 0;
    457 
    458     CachedResourceHandle<CachedResource> protect(this);
    459 
    460     // All loads finish with data(allDataReceived = true) or error(), except for
    461     // canceled loads, which silently set our request to 0. Be sure to notify our
    462     // client in that case, so we don't seem to continue loading forever.
    463     if (isLoading()) {
    464         setLoading(false);
    465         setStatus(LoadError);
    466         checkNotify();
    467     }
    468467}
    469468

  • trunk/Source/WebCore/loader/cache/CachedResource.h

    r150613 r150867  
    186186    bool inLiveDecodedResourcesList() { return m_inLiveDecodedResourcesList; }
    187187   
    188     void stopLoading();
     188    void clearLoader();
    189189
    190190    ResourceBuffer* resourceBuffer() const { ASSERT(!m_purgeableData); return m_data.get(); }
    … …  
    205205    void setAccept(const String& accept) { m_accept = accept; }
    206206
     207    void cancelLoad();
    207208    bool wasCanceled() const { return m_error.isCancellation(); }
    208209    bool errorOccurred() const { return m_status == LoadError || m_status == DecodeError; }
Note: See TracChangeset for help on using the changeset viewer.