Don't use invalidated ResourceLoadStatistics iterators
authorbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 6 May 2016 15:53:10 +0000 (15:53 +0000)
committerbfulgham@apple.com <bfulgham@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 6 May 2016 15:53:10 +0000 (15:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=157412
<rdar://problem/26133153>

Reviewed by Chris Dumez.

ResourceLoadObserver::logFrameNavigation was using references bound to the 'value'
member of iterators from the ResourceLoadStatistics HashMap. When new entries were
added, these iterators were invalidated causing the references to refer to invalid
memory.

Renamed 'resourceStatisticsForPrimaryDomain' to 'ensureResourceStatisticsForPrimaryDomain'
to clarify that it may mutate the underlying HashMap, thereby invalidating any
existing iterators.

* loader/ResourceLoadObserver.cpp:
(WebCore::ResourceLoadObserver::logFrameNavigation): Protect against HashMap
elements being copied/moved when new intries are added.
* loader/ResourceLoadStatisticsStore.cpp:
(WebCore::ResourceLoadStatisticsStore::setResourceStatisticsForPrimaryDomain): Added.
* loader/ResourceLoadStatisticsStore.h:

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

Source/WebCore/ChangeLog
Source/WebCore/loader/ResourceLoadObserver.cpp
Source/WebCore/loader/ResourceLoadStatisticsStore.cpp
Source/WebCore/loader/ResourceLoadStatisticsStore.h

index 99e4f27..37a715a 100644 (file)
@@ -1,3 +1,27 @@
+2016-05-06  Brent Fulgham  <bfulgham@apple.com>
+
+        Don't use invalidated ResourceLoadStatistics iterators
+        https://bugs.webkit.org/show_bug.cgi?id=157412
+        <rdar://problem/26133153>
+
+        Reviewed by Chris Dumez.
+
+        ResourceLoadObserver::logFrameNavigation was using references bound to the 'value'
+        member of iterators from the ResourceLoadStatistics HashMap. When new entries were
+        added, these iterators were invalidated causing the references to refer to invalid
+        memory.
+
+        Renamed 'resourceStatisticsForPrimaryDomain' to 'ensureResourceStatisticsForPrimaryDomain'
+        to clarify that it may mutate the underlying HashMap, thereby invalidating any
+        existing iterators.
+
+        * loader/ResourceLoadObserver.cpp:
+        (WebCore::ResourceLoadObserver::logFrameNavigation): Protect against HashMap
+        elements being copied/moved when new intries are added.
+        * loader/ResourceLoadStatisticsStore.cpp:
+        (WebCore::ResourceLoadStatisticsStore::setResourceStatisticsForPrimaryDomain): Added.
+        * loader/ResourceLoadStatisticsStore.h:
+
 2016-05-06  Manuel Rego Casasnovas  <rego@igalia.com>
 
         [css-grid] Unprefix CSS Grid Layout properties
index 3fe0e2e..506f329 100644 (file)
@@ -96,7 +96,7 @@ void ResourceLoadObserver::logFrameNavigation(const Frame& frame, const Frame& t
         return;
     
     auto targetOrigin = SecurityOrigin::create(targetURL);
-    auto& targetStatistics = m_store->resourceStatisticsForPrimaryDomain(targetPrimaryDomain);
+    auto targetStatistics = m_store->ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain);
     
     if (isMainFrame)
         targetStatistics.topFrameHasBeenNavigatedToBefore = true;
@@ -108,7 +108,7 @@ void ResourceLoadObserver::logFrameNavigation(const Frame& frame, const Frame& t
     }
     
     if (isRedirect) {
-        auto& redirectingOriginResourceStatistics = m_store->resourceStatisticsForPrimaryDomain(sourcePrimaryDomain);
+        auto& redirectingOriginResourceStatistics = m_store->ensureResourceStatisticsForPrimaryDomain(sourcePrimaryDomain);
         
         if (m_store->isPrevalentResource(targetPrimaryDomain))
             redirectingOriginResourceStatistics.redirectedToOtherPrevalentResourceOrigins.add(targetPrimaryDomain);
@@ -130,7 +130,7 @@ void ResourceLoadObserver::logFrameNavigation(const Frame& frame, const Frame& t
             else
                 ++targetStatistics.subframeSubResourceCount;
         } else {
-            auto& sourceOriginResourceStatistics = m_store->resourceStatisticsForPrimaryDomain(sourcePrimaryDomain);
+            auto& sourceOriginResourceStatistics = m_store->ensureResourceStatisticsForPrimaryDomain(sourcePrimaryDomain);
 
             if (isMainFrame) {
                 ++sourceOriginResourceStatistics.topFrameHasBeenNavigatedFrom;
@@ -142,6 +142,7 @@ void ResourceLoadObserver::logFrameNavigation(const Frame& frame, const Frame& t
         }
     }
 
+    m_store->setResourceStatisticsForPrimaryDomain(targetPrimaryDomain, WTFMove(targetStatistics));
     m_store->fireDataModificationHandler();
 }
     
@@ -175,13 +176,13 @@ void ResourceLoadObserver::logSubresourceLoading(const Frame* frame, const Resou
     if (targetPrimaryDomain == mainFramePrimaryDomain || targetPrimaryDomain == sourcePrimaryDomain)
         return;
 
-    auto& targetStatistics = m_store->resourceStatisticsForPrimaryDomain(targetPrimaryDomain);
+    auto& targetStatistics = m_store->ensureResourceStatisticsForPrimaryDomain(targetPrimaryDomain);
 
     auto mainFrameOrigin = SecurityOrigin::create(mainFrameURL);
     targetStatistics.subresourceUnderTopFrameOrigins.add(mainFramePrimaryDomain);
 
     if (isRedirect) {
-        auto& redirectingOriginStatistics = m_store->resourceStatisticsForPrimaryDomain(sourcePrimaryDomain);
+        auto& redirectingOriginStatistics = m_store->ensureResourceStatisticsForPrimaryDomain(sourcePrimaryDomain);
         
         if (m_store->isPrevalentResource(targetPrimaryDomain))
             redirectingOriginStatistics.redirectedToOtherPrevalentResourceOrigins.add(targetPrimaryDomain);
@@ -219,7 +220,7 @@ void ResourceLoadObserver::logUserInteraction(const Document& document)
     if (needPrivacy)
         return;
 
-    auto& statistics = m_store->resourceStatisticsForPrimaryDomain(primaryDomain(document.url()));
+    auto& statistics = m_store->ensureResourceStatisticsForPrimaryDomain(primaryDomain(document.url()));
     statistics.hadUserInteraction = true;
     m_store->fireDataModificationHandler();
 }
index d576972..24f63ae 100644 (file)
@@ -57,7 +57,7 @@ bool ResourceLoadStatisticsStore::isPrevalentResource(const String& primaryDomai
     return mapEntry->value.isPrevalentResource;
 }
     
-ResourceLoadStatistics& ResourceLoadStatisticsStore::resourceStatisticsForPrimaryDomain(const String& primaryDomain)
+ResourceLoadStatistics& ResourceLoadStatisticsStore::ensureResourceStatisticsForPrimaryDomain(const String& primaryDomain)
 {
     auto addResult = m_resourceStatisticsMap.ensure(primaryDomain, [&primaryDomain] {
         return ResourceLoadStatistics(primaryDomain);
@@ -66,6 +66,11 @@ ResourceLoadStatistics& ResourceLoadStatisticsStore::resourceStatisticsForPrimar
     return addResult.iterator->value;
 }
 
+void ResourceLoadStatisticsStore::setResourceStatisticsForPrimaryDomain(const String& primaryDomain, ResourceLoadStatistics&& statistics)
+{
+    m_resourceStatisticsMap.set(primaryDomain, WTFMove(statistics));
+}
+
 typedef HashMap<String, ResourceLoadStatistics>::KeyValuePairType StatisticsValue;
 
 std::unique_ptr<KeyedEncoder> ResourceLoadStatisticsStore::createEncoderFromData()
index c5fe56f..ac96966 100644 (file)
@@ -49,7 +49,8 @@ public:
     size_t size() const { return m_resourceStatisticsMap.size(); }
     void clear() { m_resourceStatisticsMap.clear(); }
 
-    ResourceLoadStatistics& resourceStatisticsForPrimaryDomain(const String&);
+    ResourceLoadStatistics& ensureResourceStatisticsForPrimaryDomain(const String&);
+    void setResourceStatisticsForPrimaryDomain(const String&, ResourceLoadStatistics&&);
 
     bool isPrevalentResource(const String&) const;