Changeset 241121 in webkit


Ignore:
Timestamp:
Feb 7, 2019 8:07:03 AM (5 years ago)
Author:
Antti Koivisto
Message:

Infinite recursion via CachedResource::~CachedResource
​https://bugs.webkit.org/show_bug.cgi?id=194378
<rdar://problem/42023295>

Reviewed by Daniel Bates.

I don't know the exact steps to trigger this but the mechanism seems clear.

1) An existing resource is removed from or replaced in CachedResourceLoader::m_documentResources map.
2) This decrements the handle count of resource and causes it be deleted.
3) CachedResource::~CachedResource calls m_owningCachedResourceLoader->removeCachedResource(*this). This only happens with

resources that are "owned" by CachedResourceLoader which is a rare special case (used by image document and if memory cache is disabled).

4) CachedResourceLoader::removeCachedResource looks up the resource from the map which causes a temporary CachedResourceHandle to be created.

This increments the handle count of the resource from 0 back to 1.

5) When the temporary dies, CachedResource::~CachedResource is called again and we cycle back to 3).

The fix here is simply to remove CachedResourceLoader::removeCachedResource call from ~CachedResource.
It is a leftover from when the map contained raw pointers instead of owning CachedResourceHandles.

Since m_documentResources map has a handle to the resource, the only way we are in the destructor is that the resource
has been removed from the map already (or is in process of being removed like in this crash). Any call that does anything
other than bail out is going to crash.

CachedResource::n_owningCachedResourceLoader member and CachedResourceLoader::removeCachedResource function only exist to
support this erranous call so they are removed as well.

  • loader/ImageLoader.cpp:

(WebCore::ImageLoader::updateFromElement):

  • loader/cache/CachedResource.cpp:

(WebCore::CachedResource::~CachedResource):

This is the substantive change. The rest just removes now-dead code.

  • loader/cache/CachedResource.h:

(WebCore::CachedResource::setOwningCachedResourceLoader): Deleted.

  • loader/cache/CachedResourceLoader.cpp:

(WebCore::CachedResourceLoader::~CachedResourceLoader):
(WebCore::CachedResourceLoader::requestUserCSSStyleSheet):
(WebCore::CachedResourceLoader::requestResource):
(WebCore::CachedResourceLoader::loadResource):
(WebCore::CachedResourceLoader::garbageCollectDocumentResources):
(WebCore::CachedResourceLoader::removeCachedResource): Deleted.

  • loader/cache/CachedResourceLoader.h:
Location:
trunk/Source/WebCore
Files:
6 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r241120 r241121  
     12019-02-07  Antti Koivisto  <antti@apple.com>
     2
     3        Infinite recursion via CachedResource::~CachedResource
     4        https://bugs.webkit.org/show_bug.cgi?id=194378
     5        <rdar://problem/42023295>
     6
     7        Reviewed by Daniel Bates.
     8
     9        I don't know the exact steps to trigger this but the mechanism seems clear.
     10
     11        1) An existing resource is removed from or replaced in CachedResourceLoader::m_documentResources map.
     12        2) This decrements the handle count of resource and causes it be deleted.
     13        3) CachedResource::~CachedResource calls m_owningCachedResourceLoader->removeCachedResource(*this). This only happens with
     14           resources that are "owned" by CachedResourceLoader which is a rare special case (used by image document and if memory cache is disabled).
     15        4) CachedResourceLoader::removeCachedResource looks up the resource from the map which causes a temporary CachedResourceHandle to be created.
     16           This increments the handle count of the resource from 0 back to 1.
     17        5) When the temporary dies, CachedResource::~CachedResource is called again and we cycle back to 3).
     18
     19        The fix here is simply to remove CachedResourceLoader::removeCachedResource call from ~CachedResource.
     20        It is a leftover from when the map contained raw pointers instead of owning CachedResourceHandles.
     21
     22        Since m_documentResources map has a handle to the resource, the only way we are in the destructor is that the resource
     23        has been removed from the map already (or is in process of being removed like in this crash). Any call that does anything
     24        other than bail out is going to crash.
     25
     26        CachedResource::n_owningCachedResourceLoader member and CachedResourceLoader::removeCachedResource function only exist to
     27        support this erranous call so they are removed as well.
     28
     29        * loader/ImageLoader.cpp:
     30        (WebCore::ImageLoader::updateFromElement):
     31        * loader/cache/CachedResource.cpp:
     32        (WebCore::CachedResource::~CachedResource):
     33
     34        This is the substantive change. The rest just removes now-dead code.
     35
     36        * loader/cache/CachedResource.h:
     37        (WebCore::CachedResource::setOwningCachedResourceLoader): Deleted.
     38        * loader/cache/CachedResourceLoader.cpp:
     39        (WebCore::CachedResourceLoader::~CachedResourceLoader):
     40        (WebCore::CachedResourceLoader::requestUserCSSStyleSheet):
     41        (WebCore::CachedResourceLoader::requestResource):
     42        (WebCore::CachedResourceLoader::loadResource):
     43        (WebCore::CachedResourceLoader::garbageCollectDocumentResources):
     44        (WebCore::CachedResourceLoader::removeCachedResource): Deleted.
     45        * loader/cache/CachedResourceLoader.h:
     46
    1472019-02-07  Miguel Gomez  <magomez@igalia.com>
    248

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

    r240014 r241121  
    195195            newImage->setStatus(CachedResource::Pending);
    196196            newImage->setLoading(true);
    197             newImage->setOwningCachedResourceLoader(&document.cachedResourceLoader());
    198197            document.cachedResourceLoader().m_documentResources.set(newImage->url(), newImage.get());
    199198            document.cachedResourceLoader().setAutoLoadImages(autoLoadOtherImages);

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

    r240641 r241121  
    175175    cachedResourceLeakCounter.decrement();
    176176#endif
    177 
    178     if (m_owningCachedResourceLoader)
    179         m_owningCachedResourceLoader->removeCachedResource(*this);
    180177}
    181178

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

    r241113 r241121  
    235235    virtual void destroyDecodedData() { }
    236236
    237     void setOwningCachedResourceLoader(CachedResourceLoader* cachedResourceLoader) { m_owningCachedResourceLoader = cachedResourceLoader; }
    238 
    239237    bool isPreloaded() const { return m_preloadCount; }
    240238    void increasePreloadCount() { ++m_preloadCount; }
    … …  
    334332    Vector<std::pair<String, String>> m_varyingHeaderValues;
    335333
    336     CachedResourceLoader* m_owningCachedResourceLoader { nullptr }; // only non-null for resources that are not in the cache
    337 
    338334    // If this field is non-null we are using the resource as a proxy for checking whether an existing resource is still up to date
    339335    // using HTTP If-Modified-Since/If-None-Match headers. If the response is 304 all clients of this resource are moved

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

    r240014 r241121  
    162162
    163163    clearPreloads(ClearPreloadsMode::ClearAllPreloads);
    164     for (auto& resource : m_documentResources.values())
    165         resource->setOwningCachedResourceLoader(nullptr);
    166164
    167165    // Make sure no requests still point to this CachedResourceLoader
    … …  
    259257    if (userSheet->allowsCaching())
    260258        memoryCache.add(*userSheet);
    261     // FIXME: loadResource calls setOwningCachedResourceLoader() if the resource couldn't be added to cache. Does this function need to call it, too?
    262259
    263260    userSheet->load(*this);
    … …  
    862859
    863860    auto& memoryCache = MemoryCache::singleton();
    864     if (request.allowsCaching() && memoryCache.disabled()) {
    865         if (auto handle = m_documentResources.take(url.string()))
    866             handle->setOwningCachedResourceLoader(nullptr);
    867     }
     861    if (request.allowsCaching() && memoryCache.disabled())
     862        m_documentResources.remove(url.string());
    868863
    869864    // See if we can use an existing resource from the cache.
    … …  
    10141009    CachedResourceHandle<CachedResource> resource = createResource(type, WTFMove(request), sessionID(), cookieJar);
    10151010
    1016     if (resource->allowsCaching() && !memoryCache.add(*resource))
    1017         resource->setOwningCachedResourceLoader(this);
     1011    if (resource->allowsCaching())
     1012        memoryCache.add(*resource);
    10181013
    10191014    if (RuntimeEnabledFeatures::sharedFeatures().resourceTimingEnabled())
    … …  
    13031298}
    13041299
    1305 void CachedResourceLoader::removeCachedResource(CachedResource& resource)
    1306 {
    1307     if (m_documentResources.contains(resource.url()) && m_documentResources.get(resource.url()).get() != &resource)
    1308         return;
    1309     m_documentResources.remove(resource.url());
    1310 }
    1311 
    13121300void CachedResourceLoader::loadDone(LoadCompletionType type, bool shouldPerformPostLoadActions)
    13131301{
    … …  
    13431331        LOG(ResourceLoading, "  cached resource %p - hasOneHandle %d", resource.value.get(), resource.value->hasOneHandle());
    13441332
    1345         if (resource.value->hasOneHandle()) {
     1333        if (resource.value->hasOneHandle())
    13461334            resourcesToDelete.append(resource.key);
    1347             resource.value->setOwningCachedResourceLoader(nullptr);
    1348         }
    13491335    }
    13501336

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

    r240014 r241121  
    130130    void clearDocumentLoader() { m_documentLoader = nullptr; }
    131131    PAL::SessionID sessionID() const;
    132 
    133     void removeCachedResource(CachedResource&);
    134132
    135133    void loadDone(LoadCompletionType, bool shouldPerformPostLoadActions = true);
Note: See TracChangeset for help on using the changeset viewer.