Bug 93225 – http/tests/inspector/indexeddb/database-structure.html start to crash after r124675

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Takashi Toyoshima]

David,
I guess this crash start after your change. Because following blame list include some changes, but only your change is related to IndexedDB.
http://trac.webkit.org/log/?verbose=on&rev=124677&stop_rev=124675
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#tests=http%2Ftests%2Finspector%2Findexeddb%2Fdatabase-structure.html

Do you have any idea on this?

[Takashi Toyoshima]

I added suppression for this.
Please remove this after fixing this crash.
http://trac.webkit.org/changeset/124996

[Joshua Bell]

FYI, the crash is due to this assert getting hit:

STDERR: ASSERTION FAILED: m_databaseBackendMap.contains(uniqueIdentifier)
STDERR: third_party/WebKit/Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp(68) : virtual void WebCore::IDBFactoryBackendImpl::removeIDBDatabaseBackend(const WTF::String&)

[David Grogan]

vsevik@, could you describe the indexeddb operations this test is doing?  I'd like to create a repro layout test that uses indexeddb through its regular javascript api. I think the inspector test is calling inspector functions that call idb webcore objects directly.

The failed assertion is somehow due to switching the order of firing a "complete" event and calling processPendingCalls.  We used to do processPendingCalls and then fire, now we fire then processPendingCalls.

[Joshua Bell]

Code is in Source/WebCore/inspector/InspectorIndexedDBAgent.cpp BTW

[Alec Flett]

in general the inspector is calling way too deep into the  IDB code - it should be able to do everything purely through the public API. (i.e. everything referred to with IDL) and in fact I don't see why the whole thing couldn't be written in JS.

[Alec Flett]

*** Bug 94261 has been marked as a duplicate of this bug. ***

[David Grogan]

Created attachment 166094 [details]
Patch

[David Grogan]

*** Bug 97830 has been marked as a duplicate of this bug. ***

[Joshua Bell]

Comment on attachment 166094 [details]
Patch

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

> Source/WebCore/ChangeLog:12
> +        With processPendingCalls in its old spot, it allowed the inspector to

Nice find... this doesn't affect scripts since they can't cause reentrant calls due to the event loop.

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:576
>      // FIXME: Add a test for the m_pendingOpenCalls·and m_pendingOpenWithVersionCalls cases below.

FYI, that · in the comment is my fault (copy/paste turd c/o visible whitespace). Can you nuke it while you're here?

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:590
> +    processPendingCalls();

Just trying to reason this through...

1. Before the patch:

(A) If there are 0 or 1 connections remaining, then process pending, which could allow a blocked open-with-version (if count = 0), setVersion (if count = 1), or delete if (count = 0) to run; neither an open-second-half nor a plain open would have been unblocked by a close(). 
(B) If the connection count is 0 (which implies no pending setVersions or open-second-halfs) and there are no pending opens or deletes, release the backing store.

2. After the patch:

(B) If the connection count is 0 (which implies no pending setVersions or open-second-halfs) and there are no pending opens or deletes, release the backing store.
(A) If there are 0 or 1 connections remaining, then process pending, which could allow a blocked open-with-version (if count = 0), setVersion (if count = 1), or delete if (count = 0) to run; neither an open-second-half nor a plain open would have been unblocked by a close(). 

An unblocked open-with-version: In (1), (1.A) runs, count++, so (1.B) is skipped. In (2), there's a pending open so (2.B) is skipped and (2.A) runs. Both: A runs, B skipped. Yay!

An unblocked setVersion - implies count == 1 In (1), (1.A) runs, count != 0 so (1.B) is skipped. In (2), count != 0 so (2.B) is skipped, (2.A) runs. Both: A runs, B skipped. Yay!

An unblocked delete: In (1), (1.A) runs, deleted, then (B) runs, releasing store. In (2), there's a pending delete so (2.B) is skipped, (2.A) runs. Behavior change?!?!?!?!?

It looks like this might result in the backend not being released after a delete is unblocked due to a closing handle. This would not be detectable from script - it'd require a unit test.

Stepping back, it seems like the very last last thing close() should do is release the backing store; only if there are really, truly no more pending actions should that occur. However, deleteDatabase() doesn't maintain an open connection, so it doesn't signal that it's done by going through close(). Possibly the logic in close() that releases the backing store should be factored out and called by both close() and deleteDatabase() ?

[David Grogan]

Created attachment 166331 [details]
Patch

[David Grogan]

Great catch. Thanks for the illustrative reasoning, it helped me follow.

It seems lame that we have to refactor because the inspector code is wrapped around webcore so tightly.  What do you think of this punt?  I suppose I should add a comment explaining what's up with that flag.

[Joshua Bell]

Comment on attachment 166331 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.h:85
> +    void closeBackendIfPossible();

Leftover from an experiment?

[Joshua Bell]

I had to write it out to understand what was going on. It looked fine to me until I was typing up the last case. :P

(In reply to comment #11)
> It seems lame that we have to refactor because the inspector code is wrapped around webcore so tightly.

Yep. Would be nice if it could use the script API, like the indexer does.

>  What do you think of this punt?  I suppose I should add a comment explaining what's up with that flag.

Definitely add a comment. The flag name "m_didXXX" makes methink of variables in a Mock that class record whether something *ever* happened. How about m_closingConnection?

[David Grogan]

Created attachment 166335 [details]
Patch

[David Grogan]

Created attachment 166336 [details]
fix Â

[David Grogan]

Josh, could you give this a look before I ask Tony for a review?

[Joshua Bell]

Comment on attachment 166336 [details]
fix Â

lgtm

[David Grogan]

Tony, could you review this?

[WebKit Review Bot]

Comment on attachment 166336 [details]
fix Â

Clearing flags on attachment: 166336

Committed r130198: <http://trac.webkit.org/changeset/130198>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.