Bug 72066 – IndexedDB: Close database connections, abort transactions, and terminate requests on stop()

• Component: New Bugs
• Status: RESOLVED FIXED

[Joshua Bell]

IndexedDB: Close database connection on script context destruction to unblock transaction queue

[Joshua Bell]

Created attachment 114585 [details]
Patch

[Joshua Bell]

Comment on attachment 114585 [details]
Patch

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

Early feedback appreciated. I'll see if I can put together a layouttest that does triggers the condition, but I'm not optimistic.

> Source/WebCore/storage/IDBDatabase.cpp:59
> +    , m_closePending(false)

Just a rename - the spec names the internal flag "closePending" with these semantics so we might as well do the same.

> Source/WebCore/storage/IDBDatabase.cpp:177
> +    m_backend->close(m_databaseCallbacks);

This is moved above the eventQueue work below, to allow for the early exit if the script context is gone.

> Source/WebCore/storage/IDBDatabase.cpp:197
> +

Guard against a callback coming through over IPC during document teardown

> Source/WebCore/storage/IDBDatabase.cpp:218
> +    ASSERT(scriptExecutionContext());

All callers should be checking.

> Source/WebCore/storage/IDBDatabase.cpp:240
> +}

This is the important bit - without this, the back end never gets told the connection is going away, and would never process pending open/setVersion/delete calls

> Source/WebCore/storage/IDBFactory.cpp:53
> +    : m_backend(factory)

Rename, for consistency.

> Source/WebCore/storage/IDBObjectStore.cpp:47
> +    : m_backend(idbObjectStore)

Rename, for consistency. This was the perpetually confusing one.

> Source/WebCore/storage/IDBRequest.cpp:186
>      m_errorCode = error->code();

Yet another case of defending against an IPC callback occurring during tear-down. Several more follow.

> LayoutTests/storage/indexeddb/transaction-abort-expected.txt:6
> +webkitIndexedDB.open('transaction-abort3')

Test naming conventions.

[David Grogan]

Comment on attachment 114585 [details]
Patch

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

Thanks for fixing this.

>> Source/WebCore/storage/IDBDatabase.cpp:240
>> +}
> 
> This is the important bit - without this, the back end never gets told the connection is going away, and would never process pending open/setVersion/delete calls

Is there any advantage or disadvantage to putting close in ~IDBDatabase?  I don't know that either is better or worse, just wondering if you've already figured it out.

> Source/WebCore/storage/IDBRequest.cpp:211
> +    if (!scriptExecutionContext())

I suspect that these checks don't matter much, as enqueueEvent does the same check, but I'm fine with leaving them in.

[Joshua Bell]

Comment on attachment 114585 [details]
Patch

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

>>> Source/WebCore/storage/IDBDatabase.cpp:240
>>> +}
>> 
>> This is the important bit - without this, the back end never gets told the connection is going away, and would never process pending open/setVersion/delete calls
> 
> Is there any advantage or disadvantage to putting close in ~IDBDatabase?  I don't know that either is better or worse, just wondering if you've already figured it out.

Hadn't thought about it, I'll compare. (I noticed when observing lifetimes that destructors in renderer processes often didn't run on shutdown, but that could have been how I was observing them.)

>> Source/WebCore/storage/IDBRequest.cpp:211
>> +    if (!scriptExecutionContext())
> 
> I suspect that these checks don't matter much, as enqueueEvent does the same check, but I'm fine with leaving them in.

On further thought, I'm going to back most of them out since they mutate state that is checked by assertions, which is good to keep. But I did notice...

> Source/WebCore/storage/IDBRequest.cpp:246
>  }

IDBRequest::onSuccess(PassRefPtr<IDBTransactionBackendInterface> prpBackend) which ALREADY had an early exit could be a source of problems. IPC is delivering a backend transaction pointer which is getting dropped on the floor by the early exit. I believe we'll want to call abort on it.

[Joshua Bell]

Created attachment 115407 [details]
Patch

[Joshua Bell]

Comment on attachment 115407 [details]
Patch

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

> Source/WebCore/dom/DocumentEventQueue.cpp:73
> +bool DocumentEventQueue::enqueueEvent(PassRefPtr<Event> event)

IDBRequest would keep its own list of queued events, but if it was doing so after the EventQueue was closed these would be out of sync and later assertions would fail. Return a value here so callers can determine if the event actually was queued.

> Source/WebCore/storage/IDBDatabase.cpp:59
> +    , m_closePending(false)

Rename to match spec semantics.

> Source/WebCore/storage/IDBDatabase.cpp:69
> +    close();

Since objects aren't often collected until well after the document is stopped, this is probably not necessary.

> Source/WebCore/storage/IDBDatabase.cpp:178
> +    m_backend->close(m_databaseCallbacks);

Re-order so that internal close() calls still close the backend even if stopped.

> Source/WebCore/storage/IDBFactory.cpp:53
> +    : m_backend(factory)

Rename backend pointers, for consistency.

> Source/WebCore/storage/IDBRequest.cpp:58
> +    , m_stopped(false)

Track stopped (= document is stopped) vs. finished (the IDBRequest has been fulfilled)

> Source/WebCore/storage/IDBRequest.cpp:244
> +        backend->abort();

If the backend has created a transaction and told us about it but we're stopped (so can't process it), we turn around and immediately abort it.

> Source/WebCore/storage/IDBTransaction.cpp:54
> +    , m_stopped(false)

IDBTransaction likewise needs to distinguish between stopped (Document-centric) vs. finished (IDB-centric)

> Source/WebCore/storage/IDBTransaction.cpp:190
> +void IDBTransaction::stop()

This looks scary but the change is: contextDestroyed is called only when the whole document/context/shebang is being garbage collected, which is deferred for quite some time in practice. We MUST abort the backend immediately on stop(). Tracing through, ActiveDOMObjects are not destructed - the backend abort() loops back into the frontend (this) onAbort() which aborts the IDBRequests but they don't get destructed.

> Source/WebCore/storage/IDBTransactionBackendImpl.cpp:133
> +        m_callbacks->onAbort();

It's possible that the frontend (represented here by m_callbacks) was never actually hooked up because it died before the transaction creation completed. The abort() call here comes through via the browser's dispatch mechanism cleaning up.

[David Grogan]

Comment on attachment 115407 [details]
Patch

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

LGTM

> Source/WebCore/storage/IDBRequest.cpp:153
>          ASSERT(m_readyState == DONE);

Was this getting hit when the m_stopped || !scriptExecutionContext() check was below?

> Source/WebCore/storage/IDBTransaction.cpp:-161
> -    ASSERT(!m_finished);

hilarious

[Joshua Bell]

Comment on attachment 115407 [details]
Patch

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

>> Source/WebCore/storage/IDBRequest.cpp:153
>>          ASSERT(m_readyState == DONE);
> 
> Was this getting hit when the m_stopped || !scriptExecutionContext() check was below?

Yes, but only because of the new changes - when a stop() occurs while m_readyState is LOADING the new code puts the request into the EarlyDeath state. When the IDBTransaction that's the container for this IDBRequest gets its stop() call it will abort() this request, which would trigger the assert.

I considered adding additional states to handle "death before loading", "death before done", etc but EarlyDeath seemed close enough.

[David Grogan]

(In reply to comment #8)
> (From update of attachment 115407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=115407&action=review
> 
> >> Source/WebCore/storage/IDBRequest.cpp:153
> >>          ASSERT(m_readyState == DONE);
> > 
> > Was this getting hit when the m_stopped || !scriptExecutionContext() check was below?
> 
> Yes, but only because of the new changes - when a stop() occurs while m_readyState is LOADING the new code puts the request into the EarlyDeath state. When the IDBTransaction that's the container for this IDBRequest gets its stop() call it will abort() this request, which would trigger the assert.

Got it.

> I considered adding additional states to handle "death before loading", "death before done", etc but EarlyDeath seemed close enough.

Agreed.

[Joshua Bell]

tony@ can you take a look?

[Tony Chang]

Comment on attachment 115407 [details]
Patch

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

OVERRIDE is new in webkit.  Now is as good a time as any to start using it.  r- since the cq will reject the ChangeLog anyway.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

You need to remove the OOPS! or the cq won't land this.  Please add a test or explain why this isn't testable.

> Source/WebCore/dom/DocumentEventQueue.h:55
> +    virtual bool enqueueEvent(PassRefPtr<Event>);

Nit: Can you add OVERRIDE here?

> Source/WebCore/storage/IDBDatabase.cpp:237
> +

Nit: Did you mean to add this blank line?

>> Source/WebCore/storage/IDBRequest.cpp:58
>> +    , m_stopped(false)
> 
> Track stopped (= document is stopped) vs. finished (the IDBRequest has been fulfilled)

Nit: Can we name the variables to make this more clear?  E.g., m_documentStopped and m_requestFulfilled.

> Source/WebCore/storage/IDBRequest.h:91
>      virtual bool hasPendingActivity() const;
> +    virtual void stop();

OVERRIDE

> Source/WebCore/storage/IDBTransaction.h:85
>      virtual bool hasPendingActivity() const;
>      virtual bool canSuspend() const;
> -    virtual void contextDestroyed();
> +    virtual void stop();

OVERRIDE

> Source/WebCore/workers/WorkerEventQueue.h:49
> +    virtual bool enqueueEvent(PassRefPtr<Event>);

Nit: OVERRIDE

[Joshua Bell]

Created attachment 115910 [details]
Patch

[Joshua Bell]

I'm still pondering testing possibilities here. With mock backends and execution contexts we should be able to push the sensitive objects (IDBDatabase, IDBTransaction, IDBRequest) through various stages and assert correct behavior in all cases. I've filed https://bugs.webkit.org/show_bug.cgi?id=72777 to track this. We have some substantial refactoring planned in this area too which will change the division of labor between front and back-ends, and testability should be designed in.

[WebKit Review Bot]

Comment on attachment 115910 [details]
Patch

Clearing flags on attachment: 115910

Committed r100959: <http://trac.webkit.org/changeset/100959>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.