- Revision
- 109271
- Author
- jsb...@chromium.org
- Date
- 2012-02-29 15:03:19 -0800 (Wed, 29 Feb 2012)
Log Message
IndexedDB: Resource leak with IDBObjectStore.openCursor
https://bugs.webkit.org/show_bug.cgi?id=79835
Source/WebCore:
IDBCursor object that were never continue()'d to the end would leak due to a
reference cycle with IDBRequest. In addition, the IDBRequest would never be
marked "finished" which would prevent GC from reclaiming it. IDBTransactions
now track and notify IDBCursors to break these references when the transaction
can no longer not process requests.
Reviewed by Tony Chang.
Tests: storage/indexeddb/cursor-continue.html
* storage/IDBCursor.cpp:
(WebCore::IDBCursor::IDBCursor): Register with IDBTransaction bookkeeping.
(WebCore::IDBCursor::continueFunction): Early error if transaction has finished.
(WebCore::IDBCursor::close): Break the reference cycle with IDBRequest, and notify it
that the cursor is finished.
(WebCore):
* storage/IDBCursor.h:
(WebCore):
(IDBCursor):
* storage/IDBRequest.cpp:
(WebCore::IDBRequest::IDBRequest):
(WebCore::IDBRequest::finishCursor): If there is no request in flight, mark itself as
finished to allow GC.
(WebCore):
(WebCore::IDBRequest::dispatchEvent): Once an in-flight request has been processed,
mark the request as finished if the cursor is finished, to allow GC.
* storage/IDBRequest.h:
(IDBRequest):
* storage/IDBTransaction.cpp: Track open cursors, close them when finished.
(WebCore::IDBTransaction::OpenCursorNotifier::OpenCursorNotifier):
(WebCore):
(WebCore::IDBTransaction::OpenCursorNotifier::~OpenCursorNotifier):
(WebCore::IDBTransaction::registerOpenCursor):
(WebCore::IDBTransaction::unregisterOpenCursor):
(WebCore::IDBTransaction::closeOpenCursors):
(WebCore::IDBTransaction::onAbort):
(WebCore::IDBTransaction::onComplete):
* storage/IDBTransaction.h:
(WebCore):
(OpenCursorNotifier):
(IDBTransaction):
LayoutTests:
Ensure that IDBCursor.continue() throws the right exception when transaction is finished.
Reviewed by Tony Chang.
* storage/indexeddb/cursor-continue-expected.txt:
* storage/indexeddb/cursor-continue.html:
Modified Paths
Diff
Modified: trunk/LayoutTests/ChangeLog (109270 => 109271)
--- trunk/LayoutTests/ChangeLog 2012-02-29 22:51:25 UTC (rev 109270)
+++ trunk/LayoutTests/ChangeLog 2012-02-29 23:03:19 UTC (rev 109271)
@@ -1,3 +1,15 @@
+2012-02-29 Joshua Bell <jsb...@chromium.org>
+
+ IndexedDB: Resource leak with IDBObjectStore.openCursor
+ https://bugs.webkit.org/show_bug.cgi?id=79835
+
+ Ensure that IDBCursor.continue() throws the right exception when transaction is finished.
+
+ Reviewed by Tony Chang.
+
+ * storage/indexeddb/cursor-continue-expected.txt:
+ * storage/indexeddb/cursor-continue.html:
+
2012-02-29 Adam Klein <ad...@chromium.org>
Unreviewed gardening. Remove suppressions for rubberbanding tests.
Modified: trunk/LayoutTests/storage/indexeddb/cursor-continue-expected.txt (109270 => 109271)
--- trunk/LayoutTests/storage/indexeddb/cursor-continue-expected.txt 2012-02-29 22:51:25 UTC (rev 109270)
+++ trunk/LayoutTests/storage/indexeddb/cursor-continue-expected.txt 2012-02-29 23:03:19 UTC (rev 109271)
@@ -108,9 +108,13 @@
PASS event.target.result.primaryKey is 17
event.target.result.continue('A bit2')
PASS event.target.result.primaryKey is 15
+cursor = event.target.result
Expecting exception from event.target.result.continue('A bit2')
PASS Exception was thrown.
PASS code is IDBDatabaseException.DATA_ERR
+Expecting exception from cursor.continue()
+PASS Exception was thrown.
+PASS code is IDBDatabaseException.TRANSACTION_INACTIVE_ERR
PASS successfullyParsed is true
TEST COMPLETE
Modified: trunk/LayoutTests/storage/indexeddb/cursor-continue.html (109270 => 109271)
--- trunk/LayoutTests/storage/indexeddb/cursor-continue.html 2012-02-29 22:51:25 UTC (rev 109270)
+++ trunk/LayoutTests/storage/indexeddb/cursor-continue.html 2012-02-29 23:03:19 UTC (rev 109271)
@@ -262,8 +262,9 @@
evalAndLog("event.target.result.continue('A bit2')");
} else if (window.stage == 1) {
shouldBe("event.target.result.primaryKey", "15");
+ evalAndLog("cursor = event.target.result");
evalAndExpectException("event.target.result.continue('A bit2')", "IDBDatabaseException.DATA_ERR");
- done();
+ window.trans._oncomplete_ = onTransactionComplete;
return;
} else {
testFailed("Illegal stage.");
@@ -272,6 +273,12 @@
};
}
+function onTransactionComplete()
+{
+ evalAndExpectException("cursor.continue()", "IDBDatabaseException.TRANSACTION_INACTIVE_ERR");
+ done();
+}
+
test();
</script>
Modified: trunk/Source/WebCore/ChangeLog (109270 => 109271)
--- trunk/Source/WebCore/ChangeLog 2012-02-29 22:51:25 UTC (rev 109270)
+++ trunk/Source/WebCore/ChangeLog 2012-02-29 23:03:19 UTC (rev 109271)
@@ -1,3 +1,50 @@
+2012-02-29 Joshua Bell <jsb...@chromium.org>
+
+ IndexedDB: Resource leak with IDBObjectStore.openCursor
+ https://bugs.webkit.org/show_bug.cgi?id=79835
+
+ IDBCursor object that were never continue()'d to the end would leak due to a
+ reference cycle with IDBRequest. In addition, the IDBRequest would never be
+ marked "finished" which would prevent GC from reclaiming it. IDBTransactions
+ now track and notify IDBCursors to break these references when the transaction
+ can no longer not process requests.
+
+ Reviewed by Tony Chang.
+
+ Tests: storage/indexeddb/cursor-continue.html
+
+ * storage/IDBCursor.cpp:
+ (WebCore::IDBCursor::IDBCursor): Register with IDBTransaction bookkeeping.
+ (WebCore::IDBCursor::continueFunction): Early error if transaction has finished.
+ (WebCore::IDBCursor::close): Break the reference cycle with IDBRequest, and notify it
+ that the cursor is finished.
+ (WebCore):
+ * storage/IDBCursor.h:
+ (WebCore):
+ (IDBCursor):
+ * storage/IDBRequest.cpp:
+ (WebCore::IDBRequest::IDBRequest):
+ (WebCore::IDBRequest::finishCursor): If there is no request in flight, mark itself as
+ finished to allow GC.
+ (WebCore):
+ (WebCore::IDBRequest::dispatchEvent): Once an in-flight request has been processed,
+ mark the request as finished if the cursor is finished, to allow GC.
+ * storage/IDBRequest.h:
+ (IDBRequest):
+ * storage/IDBTransaction.cpp: Track open cursors, close them when finished.
+ (WebCore::IDBTransaction::OpenCursorNotifier::OpenCursorNotifier):
+ (WebCore):
+ (WebCore::IDBTransaction::OpenCursorNotifier::~OpenCursorNotifier):
+ (WebCore::IDBTransaction::registerOpenCursor):
+ (WebCore::IDBTransaction::unregisterOpenCursor):
+ (WebCore::IDBTransaction::closeOpenCursors):
+ (WebCore::IDBTransaction::onAbort):
+ (WebCore::IDBTransaction::onComplete):
+ * storage/IDBTransaction.h:
+ (WebCore):
+ (OpenCursorNotifier):
+ (IDBTransaction):
+
2012-02-29 David Hyatt <hy...@apple.com>
https://bugs.webkit.org/show_bug.cgi?id=79940
Modified: trunk/Source/WebCore/storage/IDBCursor.cpp (109270 => 109271)
--- trunk/Source/WebCore/storage/IDBCursor.cpp 2012-02-29 22:51:25 UTC (rev 109270)
+++ trunk/Source/WebCore/storage/IDBCursor.cpp 2012-02-29 23:03:19 UTC (rev 109271)
@@ -50,6 +50,7 @@
, m_request(request)
, m_source(source)
, m_transaction(transaction)
+ , m_transactionNotifier(transaction, this)
{
ASSERT(m_backend);
ASSERT(m_request);
@@ -110,6 +111,10 @@
return;
}
+ if (!m_request) {
+ ec = IDBDatabaseException::TRANSACTION_INACTIVE_ERR;
+ return;
+ }
// FIXME: We're not using the context from when continue was called, which means the callback
// will be on the original context openCursor was called on. Is this right?
if (m_request->resetReadyState(m_transaction.get())) {
@@ -136,6 +141,13 @@
m_backend->postSuccessHandlerCallback();
}
+void IDBCursor::close()
+{
+ ASSERT(m_request);
+ m_request->finishCursor();
+ m_request.clear();
+}
+
} // namespace WebCore
#endif // ENABLE(INDEXED_DATABASE)
Modified: trunk/Source/WebCore/storage/IDBCursor.h (109270 => 109271)
--- trunk/Source/WebCore/storage/IDBCursor.h 2012-02-29 22:51:25 UTC (rev 109270)
+++ trunk/Source/WebCore/storage/IDBCursor.h 2012-02-29 23:03:19 UTC (rev 109271)
@@ -29,6 +29,7 @@
#if ENABLE(INDEXED_DATABASE)
#include "IDBKey.h"
+#include "IDBTransaction.h"
#include <wtf/PassRefPtr.h>
#include <wtf/RefCounted.h>
#include <wtf/RefPtr.h>
@@ -39,7 +40,6 @@
class IDBCallbacks;
class IDBCursorBackendInterface;
class IDBRequest;
-class IDBTransaction;
class ScriptExecutionContext;
class SerializedScriptValue;
@@ -71,6 +71,7 @@
PassRefPtr<IDBRequest> deleteFunction(ScriptExecutionContext*, ExceptionCode&);
void postSuccessHandlerCallback();
+ void close();
protected:
IDBCursor(PassRefPtr<IDBCursorBackendInterface>, IDBRequest*, IDBAny* source, IDBTransaction*);
@@ -80,6 +81,7 @@
RefPtr<IDBRequest> m_request;
RefPtr<IDBAny> m_source;
RefPtr<IDBTransaction> m_transaction;
+ IDBTransaction::OpenCursorNotifier m_transactionNotifier;
};
} // namespace WebCore
Modified: trunk/Source/WebCore/storage/IDBRequest.cpp (109270 => 109271)
--- trunk/Source/WebCore/storage/IDBRequest.cpp 2012-02-29 22:51:25 UTC (rev 109270)
+++ trunk/Source/WebCore/storage/IDBRequest.cpp 2012-02-29 23:03:19 UTC (rev 109271)
@@ -58,6 +58,7 @@
, m_transaction(transaction)
, m_readyState(LOADING)
, m_requestFinished(false)
+ , m_cursorFinished(false)
, m_contextStopped(false)
, m_cursorType(IDBCursorBackendInterface::InvalidCursorType)
, m_cursor(0)
@@ -182,6 +183,13 @@
m_cursor = cursor;
}
+void IDBRequest::finishCursor()
+{
+ m_cursorFinished = true;
+ if (m_readyState != LOADING)
+ m_requestFinished = true;
+}
+
void IDBRequest::onError(PassRefPtr<IDBDatabaseError> error)
{
ASSERT(!m_errorCode && m_errorMessage.isNull() && !m_result);
@@ -364,7 +372,7 @@
bool dontPreventDefault = IDBEventDispatcher::dispatch(event.get(), targets);
// If the result was of type IDBCursor, or a onBlocked event, then we'll fire again.
- if (event->type() != eventNames().blockedEvent && m_result && m_result->type() != IDBAny::IDBCursorType && m_result->type() != IDBAny::IDBCursorWithValueType)
+ if (event->type() != eventNames().blockedEvent && (!cursorToNotify || m_cursorFinished))
m_requestFinished = true;
if (cursorToNotify)
Modified: trunk/Source/WebCore/storage/IDBRequest.h (109270 => 109271)
--- trunk/Source/WebCore/storage/IDBRequest.h 2012-02-29 22:51:25 UTC (rev 109270)
+++ trunk/Source/WebCore/storage/IDBRequest.h 2012-02-29 23:03:19 UTC (rev 109271)
@@ -72,6 +72,7 @@
bool resetReadyState(IDBTransaction*);
void setCursorType(IDBCursorBackendInterface::CursorType);
void setCursor(PassRefPtr<IDBCursor>);
+ void finishCursor();
IDBAny* source();
void abort();
@@ -122,6 +123,7 @@
ReadyState m_readyState;
bool m_requestFinished; // Is it possible that we'll fire any more events? If not, we're finished.
+ bool m_cursorFinished;
bool m_contextStopped;
Vector<RefPtr<Event> > m_enqueuedEvents;
Modified: trunk/Source/WebCore/storage/IDBTransaction.cpp (109270 => 109271)
--- trunk/Source/WebCore/storage/IDBTransaction.cpp 2012-02-29 22:51:25 UTC (rev 109270)
+++ trunk/Source/WebCore/storage/IDBTransaction.cpp 2012-02-29 23:03:19 UTC (rev 109271)
@@ -126,6 +126,36 @@
m_backend->abort();
}
+IDBTransaction::OpenCursorNotifier::OpenCursorNotifier(PassRefPtr<IDBTransaction> transaction, IDBCursor* cursor)
+ : m_transaction(transaction),
+ m_cursor(cursor)
+{
+ m_transaction->registerOpenCursor(m_cursor);
+}
+
+IDBTransaction::OpenCursorNotifier::~OpenCursorNotifier()
+{
+ m_transaction->unregisterOpenCursor(m_cursor);
+}
+
+void IDBTransaction::registerOpenCursor(IDBCursor* cursor)
+{
+ m_openCursors.add(cursor);
+}
+
+void IDBTransaction::unregisterOpenCursor(IDBCursor* cursor)
+{
+ m_openCursors.remove(cursor);
+}
+
+void IDBTransaction::closeOpenCursors()
+{
+ HashSet<IDBCursor*> cursors;
+ cursors.swap(m_openCursors);
+ for (HashSet<IDBCursor*>::iterator i = cursors.begin(); i != cursors.end(); ++i)
+ (*i)->close();
+}
+
void IDBTransaction::registerRequest(IDBRequest* request)
{
m_childRequests.add(request);
@@ -148,6 +178,7 @@
if (m_mode == IDBTransaction::VERSION_CHANGE)
m_database->clearVersionChangeTransaction(this);
+ closeOpenCursors();
if (m_contextStopped || !scriptExecutionContext())
return;
@@ -160,6 +191,7 @@
ASSERT(!m_transactionFinished);
if (m_mode == IDBTransaction::VERSION_CHANGE)
m_database->clearVersionChangeTransaction(this);
+ closeOpenCursors();
if (m_contextStopped || !scriptExecutionContext())
return;
Modified: trunk/Source/WebCore/storage/IDBTransaction.h (109270 => 109271)
--- trunk/Source/WebCore/storage/IDBTransaction.h 2012-02-29 22:51:25 UTC (rev 109270)
+++ trunk/Source/WebCore/storage/IDBTransaction.h 2012-02-29 23:03:19 UTC (rev 109271)
@@ -36,10 +36,12 @@
#include "EventTarget.h"
#include "IDBTransactionBackendInterface.h"
#include "IDBTransactionCallbacks.h"
+#include <wtf/HashSet.h>
#include <wtf/RefCounted.h>
namespace WebCore {
+class IDBCursor;
class IDBDatabase;
class IDBObjectStore;
@@ -62,6 +64,15 @@
PassRefPtr<IDBObjectStore> objectStore(const String& name, ExceptionCode&);
void abort();
+ class OpenCursorNotifier {
+ public:
+ OpenCursorNotifier(PassRefPtr<IDBTransaction>, IDBCursor*);
+ ~OpenCursorNotifier();
+ private:
+ RefPtr<IDBTransaction> m_transaction;
+ IDBCursor* m_cursor;
+ };
+
void registerRequest(IDBRequest*);
void unregisterRequest(IDBRequest*);
void objectStoreCreated(const String&, PassRefPtr<IDBObjectStore>);
@@ -93,7 +104,11 @@
IDBTransaction(ScriptExecutionContext*, PassRefPtr<IDBTransactionBackendInterface>, IDBDatabase*);
void enqueueEvent(PassRefPtr<Event>);
+ void closeOpenCursors();
+ void registerOpenCursor(IDBCursor*);
+ void unregisterOpenCursor(IDBCursor*);
+
// EventTarget
virtual void refEventTarget() { ref(); }
virtual void derefEventTarget() { deref(); }
@@ -111,6 +126,8 @@
typedef HashMap<String, RefPtr<IDBObjectStore> > IDBObjectStoreMap;
IDBObjectStoreMap m_objectStoreMap;
+ HashSet<IDBCursor*> m_openCursors;
+
EventTargetData m_eventTargetData;
};
