⚠ Archived content — this site is no longer maintained.   Current WebKit documentation is at docs.webkit.org.

Changeset 219460 in webkit


Ignore:
Timestamp:
Jul 13, 2017, 12:16:54 PM (9 years ago)
Author:
mark.lam@apple.com
Message:

Implementors of memoryCost() need to be thread-safe.
​https://bugs.webkit.org/show_bug.cgi?id=172738
<rdar://problem/32474881>

Reviewed by Keith Miller.

No new tests. This patch fixes a race condition bug that can result in random
crashes (and other unpredictable behavior), and is very difficult to test for.

  • Modules/webaudio/AudioBuffer.cpp:

(WebCore::AudioBuffer::releaseMemory):
(WebCore::AudioBuffer::memoryCost):

  • Modules/webaudio/AudioBuffer.h:
  • dom/ChildNodeList.h:
  • dom/CollectionIndexCache.h:

(WebCore::CollectionIndexCache::memoryCost):

  • dom/LiveNodeList.h:
  • html/CachedHTMLCollection.h:
  • html/HTMLCanvasElement.cpp:

(WebCore::HTMLCanvasElement::memoryCost):
(WebCore::HTMLCanvasElement::externalMemoryCost):
(WebCore::HTMLCanvasElement::setImageBuffer):

  • html/HTMLCanvasElement.h:
  • html/HTMLCollection.cpp:

(WebCore::HTMLCollection::invalidateNamedElementCache):

  • html/HTMLCollection.h:

(WebCore::CollectionNamedElementCache::memoryCost):
(WebCore::HTMLCollection::memoryCost):
(WebCore::HTMLCollection::setNamedItemCache):

  • platform/graphics/ImageBuffer.cpp:

(WebCore::ImageBuffer::memoryCost):

  • platform/graphics/cg/ImageBufferCG.cpp:

(WebCore::ImageBuffer::memoryCost):
(WebCore::ImageBuffer::externalMemoryCost):

Location:
trunk/Source/WebCore
Files:
13 edited

Legend:

Unmodified
Added
Removed

  • trunk/Source/WebCore/ChangeLog

    r219459 r219460  
     12017-07-13  Mark Lam  <mark.lam@apple.com>
     2
     3        Implementors of memoryCost() need to be thread-safe.
     4        https://bugs.webkit.org/show_bug.cgi?id=172738
     5        <rdar://problem/32474881>
     6
     7        Reviewed by Keith Miller.
     8
     9        No new tests. This patch fixes a race condition bug that can result in random
     10        crashes (and other unpredictable behavior), and is very difficult to test for.
     11
     12        * Modules/webaudio/AudioBuffer.cpp:
     13        (WebCore::AudioBuffer::releaseMemory):
     14        (WebCore::AudioBuffer::memoryCost):
     15        * Modules/webaudio/AudioBuffer.h:
     16        * dom/ChildNodeList.h:
     17        * dom/CollectionIndexCache.h:
     18        (WebCore::CollectionIndexCache::memoryCost):
     19        * dom/LiveNodeList.h:
     20        * html/CachedHTMLCollection.h:
     21        * html/HTMLCanvasElement.cpp:
     22        (WebCore::HTMLCanvasElement::memoryCost):
     23        (WebCore::HTMLCanvasElement::externalMemoryCost):
     24        (WebCore::HTMLCanvasElement::setImageBuffer):
     25        * html/HTMLCanvasElement.h:
     26        * html/HTMLCollection.cpp:
     27        (WebCore::HTMLCollection::invalidateNamedElementCache):
     28        * html/HTMLCollection.h:
     29        (WebCore::CollectionNamedElementCache::memoryCost):
     30        (WebCore::HTMLCollection::memoryCost):
     31        (WebCore::HTMLCollection::setNamedItemCache):
     32        * platform/graphics/ImageBuffer.cpp:
     33        (WebCore::ImageBuffer::memoryCost):
     34        * platform/graphics/cg/ImageBufferCG.cpp:
     35        (WebCore::ImageBuffer::memoryCost):
     36        (WebCore::ImageBuffer::externalMemoryCost):
     37
    1382017-07-13  Jeremy Jones  <jeremyj@apple.com>
    239

  • trunk/Source/WebCore/Modules/webaudio/AudioBuffer.cpp

    r214618 r219460  
    11/*
    22 * Copyright (C) 2010 Google Inc. All rights reserved.
     3 * Copyright (C) 2017 Apple Inc. All rights reserved.
    34 *
    45 * Redistribution and use in source and binary forms, with or without
    … …  
    106107void AudioBuffer::releaseMemory()
    107108{
     109    auto locker = holdLock(m_channelsLock);
    108110    m_channels.clear();
    109111}
    … …  
    134136size_t AudioBuffer::memoryCost() const
    135137{
     138    // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful
     139    // about what data we access here and how. We need to hold a lock to prevent m_channels
     140    // from being changed while we iterate it, but calling channel->byteLength() is safe
     141    // because it doesn't involve chasing any pointers that can be nullified while the
     142    // AudioBuffer is alive.
     143    auto locker = holdLock(m_channelsLock);
    136144    size_t cost = 0;
    137145    for (auto& channel : m_channels)

  • trunk/Source/WebCore/Modules/webaudio/AudioBuffer.h

    r214618 r219460  
    11/*
    22 * Copyright (C) 2010 Google Inc. All rights reserved.
     3 * Copyright (C) 2017 Apple Inc. All rights reserved.
    34 *
    45 * Redistribution and use in source and binary forms, with or without
    … …  
    3132#include "ExceptionOr.h"
    3233#include <runtime/Float32Array.h>
     34#include <wtf/Lock.h>
    3335#include <wtf/Vector.h>
    3436
    … …  
    7476    double m_gain { 1.0 }; // scalar gain
    7577    float m_sampleRate;
     78    mutable Lock m_channelsLock;
    7679    size_t m_length;
    7780    Vector<RefPtr<Float32Array>> m_channels;

  • trunk/Source/WebCore/dom/ChildNodeList.h

    r208179 r219460  
    33 *           (C) 1999 Antti Koivisto (koivisto@kde.org)
    44 *           (C) 2001 Dirk Mueller (mueller@kde.org)
    5  * Copyright (C) 2004, 2007, 2013 Apple Inc. All rights reserved.
     5 * Copyright (C) 2004-2017 Apple Inc. All rights reserved.
    66 *
    77 * This library is free software; you can redistribute it and/or
    … …  
    8181    unsigned length() const override;
    8282    Node* item(unsigned index) const override;
    83     size_t memoryCost() const override { return m_indexCache.memoryCost(); }
     83    size_t memoryCost() const override
     84    {
     85        // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful
     86        // about what data we access here and how. Accessing m_indexCache is safe because
     87        // because it doesn't involve any pointer chasing.
     88        return m_indexCache.memoryCost();
     89    }
    8490
    8591    bool isChildNodeList() const override { return true; }

  • trunk/Source/WebCore/dom/CollectionIndexCache.h

    r204717 r219460  
    11/*
    2  * Copyright (C) 2013-2016 Apple Inc. All rights reserved.
     2 * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    4444    bool hasValidCache(const Collection& collection) const { return m_current != collection.collectionEnd() || m_nodeCountValid || m_listValid; }
    4545    void invalidate(const Collection&);
    46     size_t memoryCost() { return m_cachedList.capacity() * sizeof(NodeType*); }
     46    size_t memoryCost()
     47    {
     48        // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful
     49        // about what data we access here and how. Accessing m_cachedList.capacity() is safe
     50        // because it doesn't involve any pointer chasing.
     51        return m_cachedList.capacity() * sizeof(NodeType*);
     52    }
    4753
    4854private:

  • trunk/Source/WebCore/dom/LiveNodeList.h

    r218593 r219460  
    33 *           (C) 1999 Antti Koivisto (koivisto@kde.org)
    44 *           (C) 2001 Dirk Mueller (mueller@kde.org)
    5  * Copyright (C) 2004, 2006-2007, 2013-2014 Apple Inc. All rights reserved.
     5 * Copyright (C) 2004-2017 Apple Inc. All rights reserved.
    66 *
    77 * This library is free software; you can redistribute it and/or
    … …  
    9191
    9292    void invalidateCacheForDocument(Document&) const final;
    93     size_t memoryCost() const final { return m_indexCache.memoryCost(); }
     93    size_t memoryCost() const final
     94    {
     95        // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful
     96        // about what data we access here and how. Accessing m_indexCache is safe because
     97        // because it doesn't involve any pointer chasing.
     98        return m_indexCache.memoryCost();
     99    }
    94100
    95101protected:

  • trunk/Source/WebCore/html/CachedHTMLCollection.h

    r216851 r219460  
    11/*
    2  * Copyright (C) 2015 Apple Inc. All rights reserved.
     2 * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
    33 *
    44 * Redistribution and use in source and binary forms, with or without
    … …  
    4242    Element* item(unsigned offset) const override { return m_indexCache.nodeAt(collection(), offset); }
    4343    Element* namedItem(const AtomicString& name) const override;
    44     size_t memoryCost() const final { return m_indexCache.memoryCost() + HTMLCollection::memoryCost(); }
     44    size_t memoryCost() const final
     45    {
     46        // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful about what data we access here and how.
     47        // Accessing m_indexCache.memoryCost() is safe because because it doesn't involve any pointer chasing.
     48        // HTMLCollection::memoryCost() ensures its own thread safety.
     49        return m_indexCache.memoryCost() + HTMLCollection::memoryCost();
     50    }
    4551
    4652    // For CollectionIndexCache; do not use elsewhere.

  • trunk/Source/WebCore/html/HTMLCanvasElement.cpp

    r219268 r219460  
    11/*
    2  * Copyright (C) 2004, 2006, 2007, 2017 Apple Inc. All rights reserved.
     2 * Copyright (C) 2004-2017 Apple Inc. All rights reserved.
    33 * Copyright (C) 2007 Alp Toker <alp@atoker.com>
    44 * Copyright (C) 2010 Torch Mobile (Beijing) Co. Ltd. All rights reserved.
    … …  
    678678size_t HTMLCanvasElement::memoryCost() const
    679679{
     680    // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful
     681    // about what data we access here and how. We need to hold a lock to prevent m_imageBuffer
     682    // from being changed while we access it.
     683    auto locker = holdLock(m_imageBufferAssignmentLock);
    680684    if (!m_imageBuffer)
    681685        return 0;
    … …  
    685689size_t HTMLCanvasElement::externalMemoryCost() const
    686690{
     691    // externalMemoryCost() may be invoked concurrently from a GC thread, and we need to be careful
     692    // about what data we access here and how. We need to hold a lock to prevent m_imageBuffer
     693    // from being changed while we access it.
     694    auto locker = holdLock(m_imageBufferAssignmentLock);
    687695    if (!m_imageBuffer)
    688696        return 0;
    … …  
    784792    removeFromActivePixelMemory(previousMemoryCost);
    785793
    786     m_imageBuffer = WTFMove(buffer);
     794    {
     795        auto locker = holdLock(m_imageBufferAssignmentLock);
     796        m_imageBuffer = WTFMove(buffer);
     797    }
    787798
    788799    size_t currentMemoryCost = memoryCost();

  • trunk/Source/WebCore/html/HTMLCanvasElement.h

    r219268 r219460  
    11/*
    2  * Copyright (C) 2004, 2006, 2009, 2010, 2017 Apple Inc. All rights reserved.
     2 * Copyright (C) 2004-2017 Apple Inc. All rights reserved.
    33 * Copyright (C) 2007 Alp Toker <alp@atoker.com>
    44 * Copyright (C) 2010 Torch Mobile (Beijing) Co. Ltd. All rights reserved.
    … …  
    191191    bool m_tracksDisplayListReplay { false };
    192192
     193    mutable Lock m_imageBufferAssignmentLock;
     194   
    193195    // m_createdImageBuffer means we tried to malloc the buffer.  We didn't necessarily get it.
    194196    mutable bool m_hasCreatedImageBuffer { false };

  • trunk/Source/WebCore/html/HTMLCollection.cpp

    r217773 r219460  
    22 * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
    33 *           (C) 1999 Antti Koivisto (koivisto@kde.org)
    4  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2011, 2012 Apple Inc. All rights reserved.
     4 * Copyright (C) 2003-2017 Apple Inc. All rights reserved.
    55 *
    66 * This library is free software; you can redistribute it and/or
    … …  
    149149    ASSERT(hasNamedElementCache());
    150150    document.collectionWillClearIdNameMap(*this);
    151     m_namedElementCache = nullptr;
     151    {
     152        auto locker = holdLock(m_namedElementCacheAssignmentLock);
     153        m_namedElementCache = nullptr;
     154    }
    152155}
    153156

  • trunk/Source/WebCore/html/HTMLCollection.h

    r218748 r219460  
    22 * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
    33 *           (C) 1999 Antti Koivisto (koivisto@kde.org)
    4  * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2011, 2012, 2013, 2014 Apple Inc. All rights reserved.
     4 * Copyright (C) 2003-2017 Apple Inc. All rights reserved.
    55 *
    66 * This library is free software; you can redistribute it and/or
    … …  
    105105
    106106    mutable std::unique_ptr<CollectionNamedElementCache> m_namedElementCache;
    107 
     107    mutable Lock m_namedElementCacheAssignmentLock;
     108   
    108109    const unsigned m_collectionType : 5;
    109110    const unsigned m_invalidationType : 4;
    … …  
    141142inline size_t CollectionNamedElementCache::memoryCost() const
    142143{
     144    // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful about what data we access here and how.
     145    // It is safe to access m_idMap.size(), m_nameMap.size(), and m_propertyNames.size() because they don't chase pointers.
    143146    return (m_idMap.size() + m_nameMap.size()) * sizeof(Element*) + m_propertyNames.size() * sizeof(AtomicString);
    144147}
    … …  
    169172inline size_t HTMLCollection::memoryCost() const
    170173{
     174    // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful about what data we access here and how.
     175    // Hence, we need to guard m_namedElementCache from being replaced while accessing it.
     176    auto locker = holdLock(m_namedElementCacheAssignmentLock);
    171177    return m_namedElementCache ? m_namedElementCache->memoryCost() : 0;
    172178}
    … …  
    215221    ASSERT(!m_namedElementCache);
    216222    cache->didPopulate();
    217     m_namedElementCache = WTFMove(cache);
     223    {
     224        auto locker = holdLock(m_namedElementCacheAssignmentLock);
     225        m_namedElementCache = WTFMove(cache);
     226    }
    218227    document().collectionCachedIdNameMap(*this);
    219228}

  • trunk/Source/WebCore/platform/graphics/ImageBuffer.cpp

    r213598 r219460  
    22 * Copyright (C) 2009 Dirk Schulze <krit@webkit.org>
    33 * Copyright (C) Research In Motion Limited 2011. All rights reserved.
    4  * Copyright (C) 2016 Apple Inc. All rights reserved.
     4 * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
    55 *
    66 * Redistribution and use in source and binary forms, with or without
    … …  
    234234size_t ImageBuffer::memoryCost() const
    235235{
     236    // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful about what data we access here and how.
     237    // It's safe to access internalSize() because it doesn't do any pointer chasing.
    236238    return 4 * internalSize().width() * internalSize().height();
    237239}

  • trunk/Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp

    r215069 r219460  
    11/*
    22 * Copyright (C) 2006 Nikolas Zimmermann <zimmermann@kde.org>
    3  * Copyright (C) 2008, 2015 Apple Inc. All rights reserved.
     3 * Copyright (C) 2008-2017 Apple Inc. All rights reserved.
    44 * Copyright (C) 2010 Torch Mobile (Beijing) Co. Ltd. All rights reserved.
    55 *
    … …  
    213213size_t ImageBuffer::memoryCost() const
    214214{
     215    // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful about what data we access here and how.
     216    // It's safe to access internalSize() because it doesn't do any pointer chasing.
     217    // It's safe to access m_data.surface because the surface can only be assigned during construction of this ImageBuffer.
     218    // It's safe to access m_data.surface->totalBytes() because totalBytes() doesn't chase pointers.
    215219    if (m_data.surface)
    216220        return m_data.surface->totalBytes();
    … …  
    220224size_t ImageBuffer::externalMemoryCost() const
    221225{
     226    // externalMemoryCost() may be invoked concurrently from a GC thread, and we need to be careful about what data we access here and how.
     227    // It's safe to access m_data.surface because the surface can only be assigned during construction of this ImageBuffer.
     228    // It's safe to access m_data.surface->totalBytes() because totalBytes() doesn't chase pointers.
    222229    if (m_data.surface)
    223230        return m_data.surface->totalBytes();
Note: See TracChangeset for help on using the changeset viewer.