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?
I added suppression for this. Please remove this after fixing this crash. http://trac.webkit.org/changeset/124996
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&)
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.
Code is in Source/WebCore/inspector/InspectorIndexedDBAgent.cpp BTW
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.
*** Bug 94261 has been marked as a duplicate of this bug. ***
Created attachment 166094 [details] Patch
*** Bug 97830 has been marked as a duplicate of this bug. ***
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() ?
Created attachment 166331 [details] Patch
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.
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?
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?
Created attachment 166335 [details] Patch
Created attachment 166336 [details] fix Â
Josh, could you give this a look before I ask Tony for a review?
Comment on attachment 166336 [details] fix  lgtm
Tony, could you review this?
Comment on attachment 166336 [details] fix  Clearing flags on attachment: 166336 Committed r130198: <http://trac.webkit.org/changeset/130198>
All reviewed patches have been landed. Closing bug.