* SUMMARY Not receiving responses for async request IndexedDB.requestDatabaseNames. * STEPS TO REPRODUCE 1. Inspect a page without IndexedDB databases 2. Inspect the inspector 3. Dump InspectorBackend._callbackData => Map is non-empty, should be empty * NOTES - This results in a leak of runAfterPendingDispatches functions that never get run - This results in a leak of callbacks for IndexedDB.requestDatabaseNames that never get run - Not to mention possible broken functionality depending on those methods
Created attachment 258670 [details] [PATCH] Proposed Fix
Comment on attachment 258670 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=258670&action=review > LayoutTests/inspector/indexeddb/requestDatabaseNames-expected.txt:1 > +CONSOLE MESSAGE: line 10: Created Database 'Database1' I wish we could get these console messages properly interleaved with things that go through addResult(). It would make the test output less weird. > LayoutTests/inspector/indexeddb/requestDatabaseNames.html:20 > + InspectorTest.expectThat(names.length === 0, "No IndexedDB Databases Names"); "No databases should exist initially." > LayoutTests/inspector/indexeddb/requestDatabaseNames.html:28 > + InspectorTest.expectThat(names.length === 1, "1 IndexedDB Database Name"); "A single database should exist." > LayoutTests/inspector/indexeddb/requestDatabaseNames.html:37 > + InspectorTest.expectThat(names.length === 2, "Multiple IndexedDB Database Name"); "Two databases should exist." > LayoutTests/inspector/indexeddb/requestDatabaseNames.html:45 > + function next() { I will convert this test to the test suite approach once that stuff is accessible through inspector-test.js.
Comment on attachment 258670 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=258670&action=review > LayoutTests/inspector/indexeddb/requestDatabaseNames.html:51 > + step.action(); Instead of storing each action inside an object in the steps array, why not just store the functions themselves in the array? let action = steps.shift(); ... action(); > Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBFactoryBackend.cpp:104 > + if (recentNameIterator != sharedRecentDatabaseNameMap().end()) { So, correct me if I am wrong, this is supposed to pass an empty string list to the callback function in the event that the iterator is at the end of the map? Is the point of this to ensure that the callback gets called even when there are no entries in the database name map?
Comment on attachment 258670 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=258670&action=review >>> Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBFactoryBackend.cpp:104 >>> + if (recentNameIterator != sharedRecentDatabaseNameMap().end()) { >> >> So, correct me if I am wrong, this is supposed to pass an empty string list to the callback function in the event that the iterator is at the end of the map? Is the point of this to ensure that the callback gets called even when there are no entries in the database name map? > > So, correct me if I am wrong, this is supposed to pass an empty string list to the callback function in the event that the iterator is at the end of the map? Is the point of this to ensure that the callback gets called even when there are no entries in the database name map? Yes and yes.
Comment on attachment 258670 [details] [PATCH] Proposed Fix Attachment 258670 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/41231 New failing tests: inspector/indexeddb/requestDatabaseNames.html
Created attachment 258675 [details] Archive of layout-test-results from ews103 for mac-mavericks The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews103 Port: mac-mavericks Platform: Mac OS X 10.9.5
Comment on attachment 258670 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=258670&action=review > LayoutTests/inspector/indexeddb/requestDatabaseNames.html:19 > + IndexedDBAgent.requestDatabaseNames(WebInspector.frameResourceManager.mainFrame.securityOrigin, function(error, names) { Wouldn't it be a good idea to include check to see if error has a value and quit at that point? Like if, at some point in the future, someone makes a change that causes requestDatabaseNames to always output an error. > LayoutTests/inspector/indexeddb/requestDatabaseNames.html:27 > + IndexedDBAgent.requestDatabaseNames(WebInspector.frameResourceManager.mainFrame.securityOrigin, function(error, names) { Ditto from line 19. > LayoutTests/inspector/indexeddb/requestDatabaseNames.html:36 > + IndexedDBAgent.requestDatabaseNames(WebInspector.frameResourceManager.mainFrame.securityOrigin, function(error, names) { Ditto from line 19.
Comment on attachment 258670 [details] [PATCH] Proposed Fix Exception running the test: ERROR: Uncaught exception in inspector page while dispatching callback for command IndexedDB.requestDatabaseNames: TypeError: undefined is not an object (evaluating 'names.length') _dispatchCallback@file:///Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:181:31 dispatch@file:///Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/InspectorBackend.js:86:35 dispatchNextQueuedMessageFromBackend@file:///Volumes/Data/EWS/WebKit/WebKitBuild/Release/WebInspectorUI.framework/Resources/Protocol/MessageDispatcher.js:42:34
(In reply to comment #9) > Comment on attachment 258670 [details] > [PATCH] Proposed Fix > > Exception running the test: > > ERROR: Uncaught exception in inspector page while dispatching callback for > command IndexedDB.requestDatabaseNames: TypeError: undefined is not an > object (evaluating 'names.length') This is the error that happens when IndexedDB is not enabled for mac-WK1. I'm going to skip tests for that configuration. However, if run, the test would now fail like: PROTOCOL ERROR: No IndexedDB factory for given frame found ERROR: Uncaught exception in inspector page while dispatching callback for command IndexedDB.requestDatabaseNames: PROTOCOL ERROR
Comment on attachment 258670 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=258670&action=review >> LayoutTests/inspector/indexeddb/requestDatabaseNames-expected.txt:1 >> +CONSOLE MESSAGE: line 10: Created Database 'Database1' > > I wish we could get these console messages properly interleaved with things that go through addResult(). It would make the test output less weird. Done. >> LayoutTests/inspector/indexeddb/requestDatabaseNames.html:19 >> + IndexedDBAgent.requestDatabaseNames(WebInspector.frameResourceManager.mainFrame.securityOrigin, function(error, names) { > > Wouldn't it be a good idea to include check to see if error has a value and quit at that point? Like if, at some point in the future, someone makes a change that causes requestDatabaseNames to always output an error. Done.
Created attachment 258836 [details] [PATCH] Proposed Fix
Comment on attachment 258836 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=258836&action=review r=me > Source/WebInspectorUI/UserInterface/Base/Test.js:159 > +InspectorTest.expectNoError = function(error) I think it's called InspectorProtocol.checkForError in protocol tests. This is fine for now, I can unify them later.
Comment on attachment 258836 [details] [PATCH] Proposed Fix Clearing flags on attachment: 258836 Committed r188353: <http://trac.webkit.org/changeset/188353>
All reviewed patches have been landed. Closing bug.
> I think it's called InspectorProtocol.checkForError in protocol tests. This is fine for now, I can unify them later. Yes, I named it differently because they expect different types. "checkForError" expected a message response, so it digs into (messageObject.error). This new function just checks the error itself.
The test is very flaky: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Findexeddb%2FrequestDatabaseNames.html CONSOLE MESSAGE: line 10: Created Database 'Database1' CONSOLE MESSAGE: line 10: Created Database 'Database2' FAIL: No IndexedDB databases should exist initially Created Database 'Database1' FAIL: A single IndexedDB database should exist ["Database1","Page Cache Test"] Created Database 'Database2' FAIL: Two IndexedDB databases should exist ["Database1","Database2","Page Cache Test"] Could you please look into this soon?
(In reply to comment #17) > The test is very flaky: > > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=inspector%2Findexeddb%2FrequestDatabaseNames.html > > CONSOLE MESSAGE: line 10: Created Database 'Database1' > CONSOLE MESSAGE: line 10: Created Database 'Database2' > > FAIL: No IndexedDB databases should exist initially > Created Database 'Database1' > FAIL: A single IndexedDB database should exist > ["Database1","Page Cache Test"] > Created Database 'Database2' > FAIL: Two IndexedDB databases should exist > ["Database1","Database2","Page Cache Test"] > > > Could you please look into this soon? This test assumes that IndexedDB databases don't persist between test executions. This seems like a reasonable assumption to preserve for testing. Would you consider it a bug for the page cache tests to leave an IndexedDB database around? Or is this a bug in that the test runner isn't clearing databases between test executions?
(In reply to comment #18) > (In reply to comment #17) > > The test is very flaky: > > > > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > > html#showAllRuns=true&tests=inspector%2Findexeddb%2FrequestDatabaseNames.html > > > > CONSOLE MESSAGE: line 10: Created Database 'Database1' > > CONSOLE MESSAGE: line 10: Created Database 'Database2' > > > > FAIL: No IndexedDB databases should exist initially > > Created Database 'Database1' > > FAIL: A single IndexedDB database should exist > > ["Database1","Page Cache Test"] > > Created Database 'Database2' > > FAIL: Two IndexedDB databases should exist > > ["Database1","Database2","Page Cache Test"] > > > > > > Could you please look into this soon? > > This test assumes that IndexedDB databases don't persist between test > executions. This seems like a reasonable assumption to preserve for testing. > Would you consider it a bug for the page cache tests to leave an IndexedDB > database around? Or is this a bug in that the test runner isn't clearing > databases between test executions? After some more discussion, the assumption is still reasonable but doesn't quite reflect reality. WKTR shares a single IDB directory for all tests, and it would be extra (useful) work to use a separate IDB data store per test. To work around this, all existing IDB tests manually delete the relevant database at the beginning of the test. This test has the opposite problem, where it gets names for *all* databases. One solution is to save database names at the start of the test and diff the added database names against the baseline. This could still technically fail if two WKTR workers use the same DUMPRENDERTREE_TEMP directory. The actual test for IDBFactory.getDatabaseNames avoids listing all databases for the reason encountered in this test. Perhaps we should just fall back to testing set membership.
The interesting thing here is this test was made to test the case where a domain has No IndexedDB names. So even if we approached this using sets, that is the case I really want to have tested =)
<rdar://problem/22276450>
(In reply to comment #17) > The test is very flaky: Addressed in: <https://webkit.org/b/148008> Web Inspector: Reduce flakiness of inspector/indexeddb/requestDatabaseNames http://trac.webkit.org/changeset/188426