[Webkit-unassigned] [Bug 44700] IDBObjectStore::get should run in a transaction.

Wed Sep 22 10:01:47 PDT 2010

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

--- Comment #21 from Andrei Popescu <andreip at google.com>  2010-09-22 10:01:47 PST ---
hanks for the review. All fixed except:

(In reply to comment #19)
> 
> > WebCore/storage/IDBPendingTransactionMonitor.cpp:40
> > +        m_transactions = new Vector<IDBTransactionBackendInterface*>();
> 
> NOOOOOO manual memory management!
> 
> Why make the vector like this?  at least store it in an ownPtr...but why not just make Vector a member...and do swap's if you need to?
> 
> > WebCore/storage/IDBPendingTransactionMonitor.cpp:56
> > +        delete m_transactions;
> 
> NOOOOO
> 
> > WebCore/storage/IDBPendingTransactionMonitor.cpp:68
> > +    delete m_transactions;
> 
> NOOOOOO
> 
> > WebCore/storage/IDBPendingTransactionMonitor.h:58
> > +    static Vector<IDBTransactionBackendInterface*>* m_transactions;
> 
> monitor is in browser process, right?  if so, it should have BackendImpl pointers....or maybe even refs?
> 

No, it's in the renderer! Remember we went through all this when we added the pending transaction monitor. Eventually this will live in TLS but for now it is static. And since it's static, we need to maintain the vector's lifetime manually. I am not sure how an OwnPtr can help.

> > WebCore/storage/IDBRequest.cpp:174
> > +    if (m_transaction.get()) {
> 
> no .get()
> 
> > WebCore/storage/IDBRequest.cpp:180
> > +        m_transaction.clear();
> 
> why clear it?  i know we clear objects in a few places, but i'm wondering if it's really worth bothering
> 

We don't need the transaction anymore after this but the request object will only be released later when GC happens. I was therefore opting for clearing the pointer so that we don't needlessly keep the transaction object alive.

> > WebCore/storage/IDBRequest.h:97
> > +    RefPtr<IDBTransactionBackendInterface> m_transaction;
> 
> store an id here...and anywhere else other than backend impls
> 
> > WebCore/storage/IDBTransaction.cpp:70
> > +        throwError(NOT_SUPPORTED_ERR);
> 
> as i mentioned in the other thread, i think this is the wrong error...we should use some idb error.  unknown if there isn't one specced yet (and file a spec bug)
> 

Ok, moved to use IDBDatabaseException::NO_SUPPORTED_ERR, which is a clone of the standard DOM one. I wonder why we have to copy standard DOM exception codes in our spec?

> > WebCore/storage/IDBTransactionBackendImpl.cpp:68
> > +    if (m_state == IDLE)
> 
> some of your methods here can always be called (like abort) and bail if it's not in a state where that makes sense...and some like start assume you only call it when it makes sense.  Maybe you should be consistent?
> 

The methods that bail are the ones called from JS side, like abort or scheduleTask (a result of IDBObjectStore::get/put/etc). We don't control when those are called so we bail when there is nothing to do or we need to report an error.

The methods that don't bail are part of the internal state machine logic of the transaction and the transaction coordinator. There I have a bunch of asserts to make sure the state is always as expected.

> > WebCore/storage/IDBTransactionBackendImpl.cpp:146
> > +    queue.swap(m_taskQueue);
> 
> Why do we need to swap like this?  If stuff is added, it'll be at the end of the list...and you just keep grabbing the first item.  So I think you're ok.
> 
> Oh...but then I guess you'll need to make the above logic not reschedule another timer......so I guess this is ok.
>

Yes.

> > WebCore/storage/IDBTransactionBackendImpl.cpp:151
> > +        m_pendingEvents++;
> 
> should this happen before we call performTask?  Otherwise it's possible that the task should call didCompleteTaskEvents
>

That should not happen synchronously. The events complete asynchronously.

> But, in general, this seems fairly fragile.  Can we do it any better?  Maybe keep a list of transactions with pending tasks and then call didCompleteTaskEvents at the end of the while loop on each one?
> 

Added an assert in the IDBRequest dtor to check that the events fired for the associated transaction when the IDBRequest is destroyed.

> 

> > WebKit/chromium/public/WebIDBTransaction.h:62
> > +    virtual operator WebCore::IDBTransactionBackendInterface*() const = 0;
> 
> I'm not sure we need this here....the callers should be dealing only with the impl object, right?
> 

We're passing a WebIDBTransaction const ref to the WebIDBObjectStore::get() and such. The get() method needs to pass an IDBTransactionBackendInterface ptr to IDBObjectStoreBackendImpl::get(). It therefore needs to obtain an IDBTransactionBackendInterface from a WebIDBTransaction. This operator achieves that.

> > WebKit/chromium/src/IDBDatabaseProxy.h:53
> > +    virtual PassRefPtr<IDBObjectStoreBackendInterface> objectStore(const String& name, unsigned short mode, IDBTransactionBackendInterface* transaction = 0);
> 
> I don't think you need the default param here
> 

We're overriding a method from the interface. I think we do?

> 
> > WebKit/chromium/src/IDBTransactionBackendProxy.cpp:72
> > +bool IDBTransactionBackendProxy::scheduleTask(PassOwnPtr<ScriptExecutionContext::Task>)
> 
> Lets get rid of it from the interface then.
>

If we do, then we must allow the ObjectStore backend impls to use transaction backend impls. So far, we got away with using only interfaces everywhere. Maybe it's ok to leave as is?

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.