Bug 48425 – Clean up IDBTransactionBackend/Coordinator

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Jeremy Orlow]

.

[Jeremy Orlow]

Created attachment 72027 [details]
patch

[Andrei Popescu]

>IDBTransactionBackendImpl::~IDBTransactionBackendImpl()
> {
>    abort();

Now that the coordinator doesn't keep a ref to the transaction backends, are we guaranteed to work as expected in the following scenario:

db.transaction().objectStore("foo").put(...);
window.gc();

If the transaction frontend object gets destroyed, it will unref it's backend object, which may abort the transaction before it's got the chance to do the work?

[Jeremy Orlow]

(In reply to comment #2)
> >IDBTransactionBackendImpl::~IDBTransactionBackendImpl()
> > {
> >    abort();
> 
> Now that the coordinator doesn't keep a ref to the transaction backends, are we guaranteed to work as expected in the following scenario:
> 
> db.transaction().objectStore("foo").put(...);
> window.gc();
> 
> If the transaction frontend object gets destroyed, it will unref it's backend object, which may abort the transaction before it's got the chance to do the work?

You're right.  Actually, I think this largely isn't a problem as currently written.

[Jeremy Orlow]

Created attachment 72041 [details]
a

[Andrei Popescu]

LGTM

> PassRefPtr<IDBTransactionCoordinator> IDBTransactionCoordinator::create()
> {
>    return adoptRef(new IDBTransactionCoordinator());
> }

Why is this in the cpp now?

> m_transactions.add(transaction, transaction);

Heh :)

[Jeremy Orlow]

(In reply to comment #5)
> LGTM
> 
> > PassRefPtr<IDBTransactionCoordinator> IDBTransactionCoordinator::create()
> > {
> >    return adoptRef(new IDBTransactionCoordinator());
> > }
> 
> Why is this in the cpp now?

We do it both ways.  More often we do it in the .h, but I really think it's cleaner to do it in the cpp files.  My intention is to do it this way throughout.
 
> > m_transactions.add(transaction, transaction);
> 
> Heh :)

[Steve Block]

Comment on attachment 72041 [details]
a

View in context: https://bugs.webkit.org/attachment.cgi?id=72041&action=review

> WebCore/storage/IDBTransactionCoordinator.h:63
> +    HashMap<IDBTransactionBackendImpl*, RefPtr<IDBTransactionBackendImpl> > m_transactions;

Using a map like this seems a little odd. Can't you just use a HashSet? If required, you could provide a custom HashFunctions to make sure the value of the raw pointer is used when looking up a value.

[Jeremy Orlow]

(In reply to comment #7)
> (From update of attachment 72041 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=72041&action=review
> 
> > WebCore/storage/IDBTransactionCoordinator.h:63
> > +    HashMap<IDBTransactionBackendImpl*, RefPtr<IDBTransactionBackendImpl> > m_transactions;
> 
> Using a map like this seems a little odd. Can't you just use a HashSet? If required, you could provide a custom HashFunctions to make sure the value of the raw pointer is used when looking up a value.

I'm pretty sure I'd have to create some custom version that handles the keys being ref pointers.  Is it really worth the effort to do this?  Does it really make it that much more clean?  Maybe just a fixme is enough so the tradeoff is documented?

[Steve Block]

Comment on attachment 72041 [details]
a

View in context: https://bugs.webkit.org/attachment.cgi?id=72041&action=review

>>> WebCore/storage/IDBTransactionCoordinator.h:63
>>> +    HashMap<IDBTransactionBackendImpl*, RefPtr<IDBTransactionBackendImpl> > m_transactions;
>> 
>> Using a map like this seems a little odd. Can't you just use a HashSet? If required, you could provide a custom HashFunctions to make sure the value of the raw pointer is used when looking up a value.
> 
> I'm pretty sure I'd have to create some custom version that handles the keys being ref pointers.  Is it really worth the effort to do this?  Does it really make it that much more clean?  Maybe just a fixme is enough so the tradeoff is documented?

Sure, I guess a FIXME is OK. I think it's worth a quick try though - I didn't read through all of HashSet, but I think it might just work with RefPtr.

[Jeremy Orlow]

Committed r71412: <http://trac.webkit.org/changeset/71412>