Implementors of memoryCost() need to be thread-safe.
authormark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Jul 2017 19:16:54 +0000 (19:16 +0000)
committermark.lam@apple.com <mark.lam@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 13 Jul 2017 19:16:54 +0000 (19:16 +0000)
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):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@219460 268f45cc-cd09-0410-ab3c-d52691b4dbfc

13 files changed:
Source/WebCore/ChangeLog
Source/WebCore/Modules/webaudio/AudioBuffer.cpp
Source/WebCore/Modules/webaudio/AudioBuffer.h
Source/WebCore/dom/ChildNodeList.h
Source/WebCore/dom/CollectionIndexCache.h
Source/WebCore/dom/LiveNodeList.h
Source/WebCore/html/CachedHTMLCollection.h
Source/WebCore/html/HTMLCanvasElement.cpp
Source/WebCore/html/HTMLCanvasElement.h
Source/WebCore/html/HTMLCollection.cpp
Source/WebCore/html/HTMLCollection.h
Source/WebCore/platform/graphics/ImageBuffer.cpp
Source/WebCore/platform/graphics/cg/ImageBufferCG.cpp

index 7133df2..3deb6d4 100644 (file)
@@ -1,3 +1,40 @@
+2017-07-13  Mark Lam  <mark.lam@apple.com>
+
+        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):
+
 2017-07-13  Jeremy Jones  <jeremyj@apple.com>
 
         Fix style. Use #pragma once in VideoFullscreen and PlaybackSession headers.
index d14e669..65aad37 100644 (file)
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
+ * Copyright (C) 2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -105,6 +106,7 @@ void AudioBuffer::invalidate()
 
 void AudioBuffer::releaseMemory()
 {
+    auto locker = holdLock(m_channelsLock);
     m_channels.clear();
 }
 
@@ -133,6 +135,12 @@ void AudioBuffer::zero()
 
 size_t AudioBuffer::memoryCost() const
 {
+    // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful
+    // about what data we access here and how. We need to hold a lock to prevent m_channels
+    // from being changed while we iterate it, but calling channel->byteLength() is safe
+    // because it doesn't involve chasing any pointers that can be nullified while the
+    // AudioBuffer is alive.
+    auto locker = holdLock(m_channelsLock);
     size_t cost = 0;
     for (auto& channel : m_channels)
         cost += channel->byteLength();
index 9c9dd1d..f36b606 100644 (file)
@@ -1,5 +1,6 @@
 /*
  * Copyright (C) 2010 Google Inc. All rights reserved.
+ * Copyright (C) 2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -30,6 +31,7 @@
 
 #include "ExceptionOr.h"
 #include <runtime/Float32Array.h>
+#include <wtf/Lock.h>
 #include <wtf/Vector.h>
 
 namespace WebCore {
@@ -73,6 +75,7 @@ private:
 
     double m_gain { 1.0 }; // scalar gain
     float m_sampleRate;
+    mutable Lock m_channelsLock;
     size_t m_length;
     Vector<RefPtr<Float32Array>> m_channels;
 };
index 818227a..0ee4a48 100644 (file)
@@ -2,7 +2,7 @@
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
- * Copyright (C) 2004, 2007, 2013 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2017 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -80,7 +80,13 @@ private:
 
     unsigned length() const override;
     Node* item(unsigned index) const override;
-    size_t memoryCost() const override { return m_indexCache.memoryCost(); }
+    size_t memoryCost() const override
+    {
+        // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful
+        // about what data we access here and how. Accessing m_indexCache is safe because
+        // because it doesn't involve any pointer chasing.
+        return m_indexCache.memoryCost();
+    }
 
     bool isChildNodeList() const override { return true; }
 
index 048c9ae..6e622e9 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2013-2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2013-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -43,7 +43,13 @@ public:
 
     bool hasValidCache(const Collection& collection) const { return m_current != collection.collectionEnd() || m_nodeCountValid || m_listValid; }
     void invalidate(const Collection&);
-    size_t memoryCost() { return m_cachedList.capacity() * sizeof(NodeType*); }
+    size_t memoryCost()
+    {
+        // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful
+        // about what data we access here and how. Accessing m_cachedList.capacity() is safe
+        // because it doesn't involve any pointer chasing.
+        return m_cachedList.capacity() * sizeof(NodeType*);
+    }
 
 private:
     unsigned computeNodeCountUpdatingListCache(const Collection&);
index 613350a..37b9cfd 100644 (file)
@@ -2,7 +2,7 @@
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
  *           (C) 2001 Dirk Mueller (mueller@kde.org)
- * Copyright (C) 2004, 2006-2007, 2013-2014 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2017 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -90,7 +90,13 @@ public:
     void willValidateIndexCache() const { document().registerNodeListForInvalidation(const_cast<CachedLiveNodeList<NodeListType>&>(*this)); }
 
     void invalidateCacheForDocument(Document&) const final;
-    size_t memoryCost() const final { return m_indexCache.memoryCost(); }
+    size_t memoryCost() const final
+    {
+        // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful
+        // about what data we access here and how. Accessing m_indexCache is safe because
+        // because it doesn't involve any pointer chasing.
+        return m_indexCache.memoryCost();
+    }
 
 protected:
     CachedLiveNodeList(ContainerNode& rootNode, NodeListInvalidationType);
index 900e5a8..3590bee 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2015-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -41,7 +41,13 @@ public:
     unsigned length() const final { return m_indexCache.nodeCount(collection()); }
     Element* item(unsigned offset) const override { return m_indexCache.nodeAt(collection(), offset); }
     Element* namedItem(const AtomicString& name) const override;
-    size_t memoryCost() const final { return m_indexCache.memoryCost() + HTMLCollection::memoryCost(); }
+    size_t memoryCost() const final
+    {
+        // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful about what data we access here and how.
+        // Accessing m_indexCache.memoryCost() is safe because because it doesn't involve any pointer chasing.
+        // HTMLCollection::memoryCost() ensures its own thread safety.
+        return m_indexCache.memoryCost() + HTMLCollection::memoryCost();
+    }
 
     // For CollectionIndexCache; do not use elsewhere.
     using CollectionTraversalIterator = typename CollectionTraversal<traversalType>::Iterator;
index 32fea8c..1390f3e 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2004, 2006, 2007, 2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2017 Apple Inc. All rights reserved.
  * Copyright (C) 2007 Alp Toker <alp@atoker.com>
  * Copyright (C) 2010 Torch Mobile (Beijing) Co. Ltd. All rights reserved.
  *
@@ -677,6 +677,10 @@ bool HTMLCanvasElement::shouldAccelerate(const IntSize& size) const
 
 size_t HTMLCanvasElement::memoryCost() const
 {
+    // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful
+    // about what data we access here and how. We need to hold a lock to prevent m_imageBuffer
+    // from being changed while we access it.
+    auto locker = holdLock(m_imageBufferAssignmentLock);
     if (!m_imageBuffer)
         return 0;
     return m_imageBuffer->memoryCost();
@@ -684,6 +688,10 @@ size_t HTMLCanvasElement::memoryCost() const
 
 size_t HTMLCanvasElement::externalMemoryCost() const
 {
+    // externalMemoryCost() may be invoked concurrently from a GC thread, and we need to be careful
+    // about what data we access here and how. We need to hold a lock to prevent m_imageBuffer
+    // from being changed while we access it.
+    auto locker = holdLock(m_imageBufferAssignmentLock);
     if (!m_imageBuffer)
         return 0;
     return m_imageBuffer->externalMemoryCost();
@@ -783,7 +791,10 @@ void HTMLCanvasElement::setImageBuffer(std::unique_ptr<ImageBuffer> buffer) cons
     size_t previousMemoryCost = memoryCost();
     removeFromActivePixelMemory(previousMemoryCost);
 
-    m_imageBuffer = WTFMove(buffer);
+    {
+        auto locker = holdLock(m_imageBufferAssignmentLock);
+        m_imageBuffer = WTFMove(buffer);
+    }
 
     size_t currentMemoryCost = memoryCost();
     activePixelMemory += currentMemoryCost;
index 1fdf8a3..796db06 100644 (file)
@@ -1,5 +1,5 @@
 /*
- * Copyright (C) 2004, 2006, 2009, 2010, 2017 Apple Inc. All rights reserved.
+ * Copyright (C) 2004-2017 Apple Inc. All rights reserved.
  * Copyright (C) 2007 Alp Toker <alp@atoker.com>
  * Copyright (C) 2010 Torch Mobile (Beijing) Co. Ltd. All rights reserved.
  *
@@ -190,6 +190,8 @@ private:
     bool m_usesDisplayListDrawing { false };
     bool m_tracksDisplayListReplay { false };
 
+    mutable Lock m_imageBufferAssignmentLock;
+    
     // m_createdImageBuffer means we tried to malloc the buffer.  We didn't necessarily get it.
     mutable bool m_hasCreatedImageBuffer { false };
     mutable bool m_didClearImageBuffer { false };
index d8bf3b0..5f596a3 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
- * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2011, 2012 Apple Inc. All rights reserved.
+ * Copyright (C) 2003-2017 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -148,7 +148,10 @@ void HTMLCollection::invalidateNamedElementCache(Document& document) const
 {
     ASSERT(hasNamedElementCache());
     document.collectionWillClearIdNameMap(*this);
-    m_namedElementCache = nullptr;
+    {
+        auto locker = holdLock(m_namedElementCacheAssignmentLock);
+        m_namedElementCache = nullptr;
+    }
 }
 
 Element* HTMLCollection::namedItemSlow(const AtomicString& name) const
index bd620ea..75ac434 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * Copyright (C) 1999 Lars Knoll (knoll@kde.org)
  *           (C) 1999 Antti Koivisto (koivisto@kde.org)
- * Copyright (C) 2003, 2004, 2005, 2006, 2007, 2008, 2011, 2012, 2013, 2014 Apple Inc. All rights reserved.
+ * Copyright (C) 2003-2017 Apple Inc. All rights reserved.
  *
  * This library is free software; you can redistribute it and/or
  * modify it under the terms of the GNU Library General Public
@@ -104,7 +104,8 @@ protected:
     Ref<ContainerNode> m_ownerNode;
 
     mutable std::unique_ptr<CollectionNamedElementCache> m_namedElementCache;
-
+    mutable Lock m_namedElementCacheAssignmentLock;
+    
     const unsigned m_collectionType : 5;
     const unsigned m_invalidationType : 4;
     const unsigned m_rootType : 1;
@@ -140,6 +141,8 @@ inline void CollectionNamedElementCache::appendToNameCache(const AtomicString& n
 
 inline size_t CollectionNamedElementCache::memoryCost() const
 {
+    // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful about what data we access here and how.
+    // It is safe to access m_idMap.size(), m_nameMap.size(), and m_propertyNames.size() because they don't chase pointers.
     return (m_idMap.size() + m_nameMap.size()) * sizeof(Element*) + m_propertyNames.size() * sizeof(AtomicString);
 }
 
@@ -168,6 +171,9 @@ inline void CollectionNamedElementCache::append(StringToElementsMap& map, const
 
 inline size_t HTMLCollection::memoryCost() const
 {
+    // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful about what data we access here and how.
+    // Hence, we need to guard m_namedElementCache from being replaced while accessing it.
+    auto locker = holdLock(m_namedElementCacheAssignmentLock);
     return m_namedElementCache ? m_namedElementCache->memoryCost() : 0;
 }
 
@@ -214,7 +220,10 @@ inline void HTMLCollection::setNamedItemCache(std::unique_ptr<CollectionNamedEle
     ASSERT(cache);
     ASSERT(!m_namedElementCache);
     cache->didPopulate();
-    m_namedElementCache = WTFMove(cache);
+    {
+        auto locker = holdLock(m_namedElementCacheAssignmentLock);
+        m_namedElementCache = WTFMove(cache);
+    }
     document().collectionCachedIdNameMap(*this);
 }
 
index 6230099..72e88e0 100644 (file)
@@ -1,7 +1,7 @@
 /*
  * Copyright (C) 2009 Dirk Schulze <krit@webkit.org>
  * Copyright (C) Research In Motion Limited 2011. All rights reserved.
- * Copyright (C) 2016 Apple Inc. All rights reserved.
+ * Copyright (C) 2016-2017 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
@@ -233,6 +233,8 @@ bool ImageBuffer::isCompatibleWithContext(const GraphicsContext& context) const
 #if !USE(IOSURFACE_CANVAS_BACKING_STORE)
 size_t ImageBuffer::memoryCost() const
 {
+    // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful about what data we access here and how.
+    // It's safe to access internalSize() because it doesn't do any pointer chasing.
     return 4 * internalSize().width() * internalSize().height();
 }
 
index 9d0427d..deae2bc 100644 (file)
@@ -1,6 +1,6 @@
 /*
  * Copyright (C) 2006 Nikolas Zimmermann <zimmermann@kde.org>
- * Copyright (C) 2008, 2015 Apple Inc. All rights reserved.
+ * Copyright (C) 2008-2017 Apple Inc. All rights reserved.
  * Copyright (C) 2010 Torch Mobile (Beijing) Co. Ltd. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
@@ -212,6 +212,10 @@ FloatSize ImageBuffer::sizeForDestinationSize(FloatSize destinationSize) const
 #if USE(IOSURFACE_CANVAS_BACKING_STORE)
 size_t ImageBuffer::memoryCost() const
 {
+    // memoryCost() may be invoked concurrently from a GC thread, and we need to be careful about what data we access here and how.
+    // It's safe to access internalSize() because it doesn't do any pointer chasing.
+    // It's safe to access m_data.surface because the surface can only be assigned during construction of this ImageBuffer.
+    // It's safe to access m_data.surface->totalBytes() because totalBytes() doesn't chase pointers.
     if (m_data.surface)
         return m_data.surface->totalBytes();
     return 4 * internalSize().width() * internalSize().height();
@@ -219,6 +223,9 @@ size_t ImageBuffer::memoryCost() const
 
 size_t ImageBuffer::externalMemoryCost() const
 {
+    // externalMemoryCost() may be invoked concurrently from a GC thread, and we need to be careful about what data we access here and how.
+    // It's safe to access m_data.surface because the surface can only be assigned during construction of this ImageBuffer.
+    // It's safe to access m_data.surface->totalBytes() because totalBytes() doesn't chase pointers.
     if (m_data.surface)
         return m_data.surface->totalBytes();
     return 0;