Bug 44101 – [IndexedDB] Abort idle IDBTransactions when the JS context they were created in finishes execution.

• Component: New Bugs
• Status: RESOLVED FIXED

[Andrei Popescu]

[IndexedDB] Abort idle IDBTransactions when the JS context they were created in finishes execution.

[Andrei Popescu]

Created attachment 64575 [details]
Patch

[WebKit Review Bot]

Attachment 64575 [details] did not build on chromium:
Build output: http://queues.webkit.org/results/3784225

[Jeremy Orlow]

Comment on attachment 64575 [details]
Patch

LayoutTests/storage/indexeddb/script-tests/transaction-basics.js:1
 +  description("Test IndexedDB's basics.");
fix

LayoutTests/storage/indexeddb/script-tests/transaction-basics.js:7
 +      done();
Verify the event

WebCore/WebCore.gypi:3511
 +              'storage/IDBAbortEvent.cpp',
Lets update these in a later patch.

WebCore/bindings/v8/V8Proxy.cpp:655
 +          page->group().idbFactory()->abortPendingTransactions(IDBPendingTransactionMonitor::pendingTransactions());
Refactor to check pendingTransactions and skip the rest if there are none.  Actually hasIDBFactory might be an ASSERT in such a case?  This might make it a bit faster.

WebCore/storage/IDBAbortEvent.cpp:45
 +      : IDBEvent(eventNames().abortEvent, 0)
The source should be set.  Add a fixme for now and fix it in a subsequent patch.

WebCore/storage/IDBDatabase.cpp:76
 +      // appropriate locks have been acquired.
Maybe mention that IDBTransaction is 1:1 with a backend..since this is not true anywhere else.

WebCore/storage/IDBFactoryBackendImpl.cpp:48
 +  IDBFactoryBackendImpl::IDBFactoryBackendImpl() : m_transactionCoordinator(IDBTransactionCoordinator::create())
put on next line

WebCore/storage/IDBPendingTransactionMonitor.h:29
 +  #if ENABLE(INDEXED_DATABASE)
newline after?

WebCore/storage/IDBPendingTransactionMonitor.h:37
 +  // has been issued for it (e.g. an IDBObjectStore::put() or similar).
no asynchronous operation is currently queued for it (e.g. ...)

WebCore/storage/IDBPendingTransactionMonitor.cpp:1
 +  #include "config.h"
license

WebCore/storage/IDBPendingTransactionMonitor.cpp:8
 +  Vector<int> IDBPendingTransactionMonitor::m_ids;
Only POD static variables....need to make this lazily inited.

WebCore/storage/IDBTransactionCoordinator.h:40
 +  // This class manages transactions as follows. Requests for new transactions are
fixme: do it this way, maybe?

WebCore/storage/IDBTransactionCoordinator.h:49
 +  // available.
Mention the semantics of ordering.

WebCore/storage/IDBTransactionCoordinator.h:58
 +      IDBTransactionCoordinator();
newline after

WebCore/storage/IDBTransactionCoordinator.h:60
 +      typedef HashMap<int, RefPtr<IDBTransactionBackendInterface> > IdToQueuePositionMap;
type name sucks

WebCore/storage/IDBTransactionCoordinator.cpp:1
 +  #include "config.h"
license

WebCore/storage/IDBTransactionCoordinator.cpp:13
 +  class IDBTransactionBackendImpl : public IDBTransactionBackendInterface {
split into its own file

WebCore/storage/IDBTransactionCoordinator.cpp:107
 +      m_transactionQueue.remove(transaction.get());
assert it's in the transaction queue

WebCore/storage/IDBTransactionCoordinator.cpp:110
 +      // FIXME: this will change once we have transactions actually running.
what will change though?

WebCore/storage/IDBTransactionCoordinator.cpp:16
 +      virtual ~IDBTransactionBackendImpl() { }
maybe newline after this?...and the ~ not inline when it's in its own .h file

WebCore/storage/IDBTransactionCoordinator.cpp:18
 +      virtual unsigned short mode() const;
an example of somethign that should prob just be inline

WebCore/storage/IDBTransactionCoordinator.cpp:21
 +      virtual SQLiteDatabase* sqliteDatabase();
lets not add this until we need it

WebCore/storage/IDBTransactionCoordinator.cpp:23
 +      virtual void setCallbacks(IDBTransactionCallbacks*);
newline after

WebCore/storage/IDBTransactionCoordinator.cpp:56
 +      return 0;
do something

WebCore/storage/IDBTransactionCoordinator.cpp:51
 +      return 0;
fixme

WebCore/storage/IDBTransactionCoordinator.cpp:60
 +  {
fixme...plus ASSERT_NOT_REACHED on all

WebCore/storage/IDBTransactionCoordinator.cpp:66
 +      if (m_callbacks)
remove if

WebCore/storage/IDBTransactionCoordinator.cpp:72
 +      return 0;
delete

WebCore/storage/IDBTransactionCoordinator.cpp:77
 +      return m_id;
inline...plus mode and such

WebCore/storage/IDBTransactionCoordinator.cpp:82
 +      m_callbacks = callbacks;
inline

WebCore/storage/IDBTransactionCoordinator.cpp:86
 +  IDBTransactionCoordinator::IDBTransactionCoordinator() : m_nextID(0)
initialization on the next line

WebCore/storage/IDBTransactionCallbacks.h:35
 +  #if ENABLE(INDEXED_DATABASE)
put this above the other includes

WebCore/storage/IDBTransactionCallbacks.h:45
 +      // FIXME: add the rest
In comments, try to use complete sentences.

WebCore/storage/IDBTransaction.h:100
 +  
no newline

WebCore/storage/IDBTransaction.h:70
 +      virtual int id() const;
newline after this

WebCore/storage/IDBTransaction.cpp:106
 +      dispatchEvent(abortEvent); // FIXME: create the correct event
delete fixme

WebCore/storage/IDBTransaction.cpp:105
 +      RefPtr<IDBAbortEvent> abortEvent = IDBAbortEvent::create();
just do this inline?

WebCore/storage/IDBTransaction.cpp:104
 +      RefPtr<IDBTransaction> selfRef = m_selfRef.release();
Reference comment in IDBRequest.cpp about how the self ref works.

WebCore/storage/IDBTransaction.h:70
 +      virtual int id() const;
fixme: implement other events.

WebCore/storage/IDBTransaction.cpp:65
 +      // Release the backend.
Mention this is to break a circular reference?

WebKit/chromium/public/WebIDBFactory.h:69
 +        WEBKIT_ASSERT_NOT_REACHED();
wrong indent

WebKit/chromium/public/WebIDBFactory.h:67
 +      virtual void abortPendingTransactions(const WebVector<int>& pendingIDs)
Can be inlined...only need multiple lines if multiple statements.

WebKit/chromium/public/WebIDBTransaction.h:60
 +      virtual void setCallbacks(WebIDBTransactionCallbacks*)
one line

WebKit/chromium/public/WebIDBTransaction.h:53
 +          WEBKIT_ASSERT_NOT_REACHED();
one line

WebKit/chromium/public/WebIDBTransactionCallbacks.h:31
 +  namespace WebKit {
newline after

WebKit/chromium/src/IDBDatabaseProxy.cpp:101
 +      if (!transaction)
Can this happen?

WebKit/chromium/src/IDBFactoryBackendProxy.cpp:70
 +      if (!pendingIDs.size())
Maybe assert this?

WebKit/chromium/src/IDBFactoryBackendProxy.h:49
 +      virtual void abortPendingTransactions(const Vector<int>& pendingIDs);
newline after

WebKit/chromium/src/IDBTransactionBackendProxy.cpp:36
 +  #if ENABLE(INDEXED_DATABASE)
move up

WebKit/chromium/src/IDBTransactionBackendProxy.h:33
 +  #if ENABLE(INDEXED_DATABASE)
move up

WebKit/chromium/src/IDBTransactionBackendProxy.cpp:57
 +      if (!objectStore)
possible?  it probaby is, but double check

WebKit/chromium/src/IDBTransactionBackendProxy.cpp:74
 +      ASSERT_NOT_REACHED();
FIXME

WebKit/chromium/src/IDBTransactionBackendProxy.cpp:79
 +      ASSERT_NOT_REACHED();
delete

WebKit/chromium/src/IDBTransactionCallbacksProxy.h:51
 +      virtual int id() const;
FIXME: implement others

WebKit/chromium/src/IDBTransactionCallbacksProxy.h:37
 +  #if ENABLE(INDEXED_DATABASE)
move up

WebKit/chromium/src/IDBTransactionCallbacksProxy.h:40
 +  class WebIDBTransactionCallbacks;
one line

WebKit/chromium/src/IDBTransactionCallbacksProxy.h:35
 +  #include <wtf/RefPtr.h>
why do you nee this?

WebKit/chromium/src/IDBTransactionCallbacksProxy.h:34
 +  #include <wtf/PassRefPtr.h>
or this?

WebKit/chromium/src/WebIDBDatabaseImpl.cpp:92
 +      RefPtr<DOMStringList> nameList = PassRefPtr<DOMStringList>(names);
For this reason, I'd lean towards having transaction take in a PassRefPtr...

WebKit/chromium/src/WebIDBDatabaseImpl.cpp:95
 +          return 0;
make sure the level above does the right thing

WebKit/chromium/src/WebIDBFactoryImpl.cpp:67
 +  {
I think WebVector has "cute" ways of dealing with this.

WebKit/chromium/src/WebIDBFactoryImpl.h:46
 +      virtual void abortPendingTransactions(const WebVector<int>& pendingIDs);
newline after

WebKit/chromium/src/WebIDBTransactionCallbacksImpl.cpp:31
 +  #if ENABLE(INDEXED_DATABASE)
move up

WebKit/chromium/src/WebIDBTransactionImpl.h:53
 +  
extra new line

WebKit/chromium/src/WebIDBTransactionImpl.cpp:34
 +  #if ENABLE(INDEXED_DATABASE)
move up

[Andrei Popescu]

Created attachment 64709 [details]
Patch

[Andrei Popescu]

(In reply to comment #3)
> (From update of attachment 64575 [details])

Thanks for the comments Jeremy. All fixed, new patch attached.

[Jeremy Orlow]

Comment on attachment 64709 [details]
Patch

WebKit/chromium/src/WebIDBTransactionImpl.h:54
 +  
extra newline

WebCore/storage/IDBPendingTransactionMonitor.cpp:37
 +      return (m_ids && m_ids->size());
no need for ()'s

WebCore/storage/IDBPendingTransactionMonitor.cpp:58
 +      m_ids->clear();
Hm...might be worth to have this do a if (m_ids) check?

WebCore/storage/IDBPendingTransactionMonitor.cpp:47
 +  void IDBPendingTransactionMonitor::removePendingTransaction(int id)
I'm tempted to say this should be a set rather than a vector...but at the same time I don't see the list ever getting very big.

WebCore/storage/IDBPendingTransactionMonitor.h:51
 +      static const Vector<int>& pendingTransactions();
newline after

WebCore/storage/IDBTransactionCoordinator.cpp:1
 +  #include "config.h"
license!


r=me

[Andrei Popescu]

Created attachment 64840 [details]
Patch

[Andrei Popescu]

Thanks, all comments addressed, will land as soon as I get confirmation from the try bots that all is well.

[Andrei Popescu]

Committed r65670: <http://trac.webkit.org/changeset/65670>