Merge r181411 - Many users of Heap::reportExtraMemory* are wrong, cau…

…sing lots of memory growth

https://bugs.webkit.org/show_bug.cgi?id=142593

Reviewed by Andreas Kling.

Adopt deprecatedReportExtraMemory as a short-term fix for runaway
memory growth in these cases where we have not adopted
reportExtraMemoryVisited.

Long-term, we should use reportExtraMemoryAllocated+reportExtraMemoryVisited.
That's tracked by https://bugs.webkit.org/show_bug.cgi?id=142595.

Source/JavaScriptCore:

* API/JSBase.cpp:
(JSReportExtraMemoryCost):
* runtime/SparseArrayValueMap.cpp:
(JSC::SparseArrayValueMap::add):

Source/WebCore:

Using IOSDebug, I can see that the canvas stress test @ http://jsfiddle.net/fvyw4ba0/,
which used to keep > 1000 1MB NonVolatile GPU allocations live, now keeps about 10 live.

* Modules/mediasource/SourceBuffer.cpp:
(WebCore::SourceBuffer::reportExtraMemoryAllocated):
* bindings/js/JSDocumentCustom.cpp:
(WebCore::toJS):
* bindings/js/JSImageDataCustom.cpp:
(WebCore::toJS):
* bindings/js/JSNodeListCustom.cpp:
(WebCore::createWrapper):
* dom/CollectionIndexCache.cpp:
(WebCore::reportExtraMemoryAllocatedForCollectionIndexCache):
* html/HTMLCanvasElement.cpp:
(WebCore::HTMLCanvasElement::createImageBuffer):
* html/HTMLImageLoader.cpp:
(WebCore::HTMLImageLoader::imageChanged):
* html/HTMLMediaElement.cpp:
(WebCore::HTMLMediaElement::parseAttribute):
* xml/XMLHttpRequest.cpp:
(WebCore::XMLHttpRequest::dropProtection):

Source/JavaScriptCore/API/JSBase.cpp

@@ -139,9 +139,7 @@ void JSReportExtraMemoryCost(JSContextRef ctx, size_t size)
     ExecState* exec = toJS(ctx);
     JSLockHolder locker(exec);
 
-    // FIXME: switch to deprecatedReportExtraMemory.
-    // https://bugs.webkit.org/show_bug.cgi?id=142593
-    exec->vm().heap.reportExtraMemoryAllocated(size);
+    exec->vm().heap.deprecatedReportExtraMemory(size);
 }
 
 extern "C" JS_EXPORT void JSSynchronousGarbageCollectForDebugging(JSContextRef);

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,22 @@
+2015-03-11  Geoffrey Garen  <ggaren@apple.com>
+
+        Many users of Heap::reportExtraMemory* are wrong, causing lots of memory growth
+        https://bugs.webkit.org/show_bug.cgi?id=142593
+
+        Reviewed by Andreas Kling.
+
+        Adopt deprecatedReportExtraMemory as a short-term fix for runaway
+        memory growth in these cases where we have not adopted
+        reportExtraMemoryVisited.
+
+        Long-term, we should use reportExtraMemoryAllocated+reportExtraMemoryVisited.
+        That's tracked by https://bugs.webkit.org/show_bug.cgi?id=142595.
+
+        * API/JSBase.cpp:
+        (JSReportExtraMemoryCost):
+        * runtime/SparseArrayValueMap.cpp:
+        (JSC::SparseArrayValueMap::add):
+
 2015-03-11  Geoffrey Garen  <ggaren@apple.com>
 
         Refactored the JSC::Heap extra cost API for clarity and to make some known bugs more obvious

Source/JavaScriptCore/runtime/SparseArrayValueMap.cpp

@@ -80,9 +80,9 @@ SparseArrayValueMap::AddResult SparseArrayValueMap::add(JSObject* array, unsigne
     AddResult result = m_map.add(i, entry);
     size_t capacity = m_map.capacity();
     if (capacity != m_reportedCapacity) {
-        // FIXME: Switch to deprecatedReportExtraMemory, or adopt reportExtraMemoryVisited.
-        // https://bugs.webkit.org/show_bug.cgi?id=142593
-        Heap::heap(array)->reportExtraMemoryAllocated((capacity - m_reportedCapacity) * (sizeof(unsigned) + sizeof(WriteBarrier<Unknown>)));
+        // FIXME: Adopt reportExtraMemoryVisited, and switch to reportExtraMemoryAllocated.
+        // https://bugs.webkit.org/show_bug.cgi?id=142595
+        Heap::heap(array)->deprecatedReportExtraMemory((capacity - m_reportedCapacity) * (sizeof(unsigned) + sizeof(WriteBarrier<Unknown>)));
         m_reportedCapacity = capacity;
     }
     return result;

Source/WebCore/ChangeLog

@@ -1,3 +1,61 @@
+2015-03-12  Geoffrey Garen  <ggaren@apple.com>
+
+        REGRESSION: Crash under Heap::reportExtraMemoryAllocatedSlowCase for media element
+        https://bugs.webkit.org/show_bug.cgi?id=142636
+
+        Reviewed by Mark Hahnenberg.
+
+        This was a pre-existing bug that I made a lot worse in
+        <https://trac.webkit.org/changeset/181411>.
+
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::parseAttribute): Compare size before
+        subtracting rather than subtracting and then comparing to zero. The
+        latter technique is not valid for unsigned integers, which will happily
+        underflow into giant numbers.
+
+        * Modules/mediasource/SourceBuffer.cpp:
+        (WebCore::SourceBuffer::reportExtraMemoryAllocated): This code was
+         technically correct, but I took the opportunity to clean it up a bit.
+         There's no need to do two checks here, and it smells bad to check for
+         a negative unsigned integer.
+
+2015-03-11  Geoffrey Garen  <ggaren@apple.com>
+
+        Many users of Heap::reportExtraMemory* are wrong, causing lots of memory growth
+        https://bugs.webkit.org/show_bug.cgi?id=142593
+
+        Reviewed by Andreas Kling.
+
+        Adopt deprecatedReportExtraMemory as a short-term fix for runaway
+        memory growth in these cases where we have not adopted
+        reportExtraMemoryVisited.
+
+        Long-term, we should use reportExtraMemoryAllocated+reportExtraMemoryVisited.
+        That's tracked by https://bugs.webkit.org/show_bug.cgi?id=142595.
+
+        Using IOSDebug, I can see that the canvas stress test @ http://jsfiddle.net/fvyw4ba0/,
+        which used to keep > 1000 1MB NonVolatile GPU allocations live, now keeps about 10 live.
+
+        * Modules/mediasource/SourceBuffer.cpp:
+        (WebCore::SourceBuffer::reportExtraMemoryAllocated):
+        * bindings/js/JSDocumentCustom.cpp:
+        (WebCore::toJS):
+        * bindings/js/JSImageDataCustom.cpp:
+        (WebCore::toJS):
+        * bindings/js/JSNodeListCustom.cpp:
+        (WebCore::createWrapper):
+        * dom/CollectionIndexCache.cpp:
+        (WebCore::reportExtraMemoryAllocatedForCollectionIndexCache):
+        * html/HTMLCanvasElement.cpp:
+        (WebCore::HTMLCanvasElement::createImageBuffer):
+        * html/HTMLImageLoader.cpp:
+        (WebCore::HTMLImageLoader::imageChanged):
+        * html/HTMLMediaElement.cpp:
+        (WebCore::HTMLMediaElement::parseAttribute):
+        * xml/XMLHttpRequest.cpp:
+        (WebCore::XMLHttpRequest::dropProtection):
+
 2015-03-11  Geoffrey Garen  <ggaren@apple.com>
 
         Refactored the JSC::Heap extra cost API for clarity and to make some known bugs more obvious

Source/WebCore/Modules/mediasource/SourceBuffer.cpp

@@ -1981,18 +1981,16 @@ size_t SourceBuffer::extraMemoryCost() const
 void SourceBuffer::reportExtraMemoryAllocated()
 {
     size_t extraMemoryCost = this->extraMemoryCost();
-    if (extraMemoryCost < m_reportedExtraMemoryCost)
+    if (extraMemoryCost <= m_reportedExtraMemoryCost)
         return;
 
     size_t extraMemoryCostDelta = extraMemoryCost - m_reportedExtraMemoryCost;
     m_reportedExtraMemoryCost = extraMemoryCost;
 
     JSC::JSLockHolder lock(scriptExecutionContext()->vm());
-    if (extraMemoryCostDelta > 0) {
-        // FIXME: Switch to deprecatedReportExtraMemory, or adopt reportExtraMemoryVisited.
-        // https://bugs.webkit.org/show_bug.cgi?id=142593
-        scriptExecutionContext()->vm().heap.reportExtraMemoryAllocated(extraMemoryCostDelta);
-    }
+    // FIXME: Adopt reportExtraMemoryVisited, and switch to reportExtraMemoryAllocated.
+    // https://bugs.webkit.org/show_bug.cgi?id=142595
+    scriptExecutionContext()->vm().heap.deprecatedReportExtraMemory(extraMemoryCostDelta);
 }
 
 Vector<String> SourceBuffer::bufferedSamplesForTrackID(const AtomicString& trackID)

Source/WebCore/bindings/js/JSDocumentCustom.cpp

@@ -109,9 +109,9 @@ JSValue toJS(ExecState* exec, JSDOMGlobalObject* globalObject, Document* documen
         for (Node* n = document; n; n = NodeTraversal::next(*n))
             nodeCount++;
 
-        // FIXME: Switch to deprecatedReportExtraMemory, or adopt reportExtraMemoryVisited.
-        // https://bugs.webkit.org/show_bug.cgi?id=142593
-        exec->heap()->reportExtraMemoryAllocated(nodeCount * sizeof(Node));
+        // FIXME: Adopt reportExtraMemoryVisited, and switch to reportExtraMemoryAllocated.
+        // https://bugs.webkit.org/show_bug.cgi?id=142595
+        exec->heap()->deprecatedReportExtraMemory(nodeCount * sizeof(Node));
     }
 
     return wrapper;

Source/WebCore/bindings/js/JSImageDataCustom.cpp

@@ -47,9 +47,9 @@ JSValue toJS(ExecState* exec, JSDOMGlobalObject* globalObject, ImageData* imageD
     wrapper = CREATE_DOM_WRAPPER(globalObject, ImageData, imageData);
     Identifier dataName(exec, "data");
     wrapper->putDirect(exec->vm(), dataName, toJS(exec, globalObject, imageData->data()), DontDelete | ReadOnly);
-    // FIXME: Switch to deprecatedReportExtraMemory, or adopt reportExtraMemoryVisited.
-    // https://bugs.webkit.org/show_bug.cgi?id=142593
-    exec->heap()->reportExtraMemoryAllocated(imageData->data()->length());
+    // FIXME: Adopt reportExtraMemoryVisited, and switch to reportExtraMemoryAllocated.
+    // https://bugs.webkit.org/show_bug.cgi?id=142595
+    exec->heap()->deprecatedReportExtraMemory(imageData->data()->length());
 
     return wrapper;
 }

Source/WebCore/bindings/js/JSNodeListCustom.cpp

@@ -62,9 +62,9 @@ bool JSNodeList::getOwnPropertySlotDelegate(ExecState* exec, PropertyName proper
 
 JSC::JSValue createWrapper(JSDOMGlobalObject& globalObject, NodeList& nodeList)
 {
-    // FIXME: Switch to deprecatedReportExtraMemory, or adopt reportExtraMemoryVisited.
-    // https://bugs.webkit.org/show_bug.cgi?id=142593
-    globalObject.vm().heap.reportExtraMemoryAllocated(nodeList.memoryCost());
+    // FIXME: Adopt reportExtraMemoryVisited, and switch to reportExtraMemoryAllocated.
+    // https://bugs.webkit.org/show_bug.cgi?id=142595
+    globalObject.vm().heap.deprecatedReportExtraMemory(nodeList.memoryCost());
     return createNewWrapper<JSNodeList>(&globalObject, &nodeList);
 }
 

Source/WebCore/dom/CollectionIndexCache.cpp

@@ -34,9 +34,9 @@ void reportExtraMemoryAllocatedForCollectionIndexCache(size_t cost)
 {
     JSC::VM& vm = JSDOMWindowBase::commonVM();
     JSC::JSLockHolder lock(vm);
-    // FIXME: Switch to deprecatedReportExtraMemory, or adopt reportExtraMemoryVisited.
-    // https://bugs.webkit.org/show_bug.cgi?id=142593
-    vm.heap.reportExtraMemoryAllocated(cost);
+    // FIXME: Adopt reportExtraMemoryVisited, and switch to reportExtraMemoryAllocated.
+    // https://bugs.webkit.org/show_bug.cgi?id=142595
+    vm.heap.deprecatedReportExtraMemory(cost);
 }
 
 }

Source/WebCore/html/HTMLCanvasElement.cpp

@@ -580,9 +580,9 @@ void HTMLCanvasElement::createImageBuffer() const
 
     JSC::JSLockHolder lock(scriptExecutionContext()->vm());
     size_t numBytes = 4 * m_imageBuffer->internalSize().width() * m_imageBuffer->internalSize().height();
-    // FIXME: Switch to deprecatedReportExtraMemory, or adopt reportExtraMemoryVisited.
-    // https://bugs.webkit.org/show_bug.cgi?id=142593
-    scriptExecutionContext()->vm().heap.reportExtraMemoryAllocated(numBytes);
+    // FIXME: Adopt reportExtraMemoryVisited, and switch to reportExtraMemoryAllocated.
+    // https://bugs.webkit.org/show_bug.cgi?id=142595
+    scriptExecutionContext()->vm().heap.deprecatedReportExtraMemory(numBytes);
 
 #if USE(IOSURFACE_CANVAS_BACKING_STORE) || ENABLE(ACCELERATED_2D_CANVAS)
     if (m_context && m_context->is2d())

Source/WebCore/html/HTMLImageLoader.cpp

@@ -87,9 +87,9 @@ void HTMLImageLoader::imageChanged(CachedImage* cachedImage, const IntRect*)
         if (!element().inDocument()) {
             JSC::VM& vm = JSDOMWindowBase::commonVM();
             JSC::JSLockHolder lock(vm);
-            // FIXME: Switch to deprecatedReportExtraMemory, or adopt reportExtraMemoryVisited.
-            // https://bugs.webkit.org/show_bug.cgi?id=142593
-            vm.heap.reportExtraMemoryAllocated(cachedImage->encodedSize());
+            // FIXME: Adopt reportExtraMemoryVisited, and switch to reportExtraMemoryAllocated.
+            // https://bugs.webkit.org/show_bug.cgi?id=142595
+            vm.heap.deprecatedReportExtraMemory(cachedImage->encodedSize());
         }
     }
 

Source/WebCore/html/HTMLMediaElement.cpp

@@ -685,17 +685,16 @@ void HTMLMediaElement::removedFrom(ContainerNode& insertionPoint)
             exitFullscreen();
 
         if (m_player) {
-            JSC::VM& vm = JSDOMWindowBase::commonVM();
-            JSC::JSLockHolder lock(vm);
-
             size_t extraMemoryCost = m_player->extraMemoryCost();
-            size_t extraMemoryCostDelta = extraMemoryCost - m_reportedExtraMemoryCost;
-            m_reportedExtraMemoryCost = extraMemoryCost;
-
-            if (extraMemoryCostDelta > 0) {
-                // FIXME: Switch to deprecatedReportExtraMemory, or adopt reportExtraMemoryVisited.
-                // https://bugs.webkit.org/show_bug.cgi?id=142593
-                vm.heap.reportExtraMemoryAllocated(extraMemoryCostDelta);
+            if (extraMemoryCost > m_reportedExtraMemoryCost) {
+                JSC::VM& vm = JSDOMWindowBase::commonVM();
+                JSC::JSLockHolder lock(vm);
+
+                size_t extraMemoryCostDelta = extraMemoryCost - m_reportedExtraMemoryCost;
+                m_reportedExtraMemoryCost = extraMemoryCost;
+                // FIXME: Adopt reportExtraMemoryVisited, and switch to reportExtraMemoryAllocated.
+                // https://bugs.webkit.org/show_bug.cgi?id=142595
+                vm.heap.deprecatedReportExtraMemory(extraMemoryCostDelta);
             }
         }
     }

Source/WebCore/xml/XMLHttpRequest.cpp

@@ -913,9 +913,9 @@ void XMLHttpRequest::dropProtection()
     // report the extra cost at that point.
     JSC::VM& vm = scriptExecutionContext()->vm();
     JSC::JSLockHolder lock(vm);
-    // FIXME: Switch to deprecatedReportExtraMemory, or adopt reportExtraMemoryVisited.
-    // https://bugs.webkit.org/show_bug.cgi?id=142593
-    vm.heap.reportExtraMemoryAllocated(m_responseBuilder.length() * 2);
+    // FIXME: Adopt reportExtraMemoryVisited, and switch to reportExtraMemoryAllocated.
+    // https://bugs.webkit.org/show_bug.cgi?id=142595
+    vm.heap.deprecatedReportExtraMemory(m_responseBuilder.length() * 2);
 
     unsetPendingActivity(this);
 }