RESOLVED FIXED 147844
Web Inspector: Not receiving responses for async request IndexedDB.requestDatabaseNames
https://bugs.webkit.org/show_bug.cgi?id=147844
Summary Web Inspector: Not receiving responses for async request IndexedDB.requestDat...
Joseph Pecoraro
Reported 2015-08-10 14:50:59 PDT
* 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
Attachments
[PATCH] Proposed Fix (5.53 KB, patch)
2015-08-10 16:20 PDT, Joseph Pecoraro
timothy: review-
buildbot: commit-queue-
Archive of layout-test-results from ews103 for mac-mavericks (536.46 KB, application/zip)
2015-08-10 16:55 PDT, Build Bot
no flags
[PATCH] Proposed Fix (7.35 KB, patch)
2015-08-12 12:01 PDT, Joseph Pecoraro
no flags
Joseph Pecoraro
Comment 1 2015-08-10 16:20:32 PDT
Created attachment 258670 [details] [PATCH] Proposed Fix
Blaze Burg
Comment 2 2015-08-10 16:28:08 PDT
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.
Devin Rousso
Comment 3 2015-08-10 16:41:16 PDT
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?
Devin Rousso
Comment 4 2015-08-10 16:41:17 PDT
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?
Joseph Pecoraro
Comment 5 2015-08-10 16:44:06 PDT
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.
Build Bot
Comment 6 2015-08-10 16:55:21 PDT
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
Build Bot
Comment 7 2015-08-10 16:55:23 PDT
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
Devin Rousso
Comment 8 2015-08-10 17:02:03 PDT
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.
Timothy Hatcher
Comment 9 2015-08-11 22:43:20 PDT
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
Joseph Pecoraro
Comment 10 2015-08-12 11:58:20 PDT
(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
Joseph Pecoraro
Comment 11 2015-08-12 12:00:24 PDT
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.
Joseph Pecoraro
Comment 12 2015-08-12 12:01:08 PDT
Created attachment 258836 [details] [PATCH] Proposed Fix
Blaze Burg
Comment 13 2015-08-12 12:27:33 PDT
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.
WebKit Commit Bot
Comment 14 2015-08-12 13:20:59 PDT
Comment on attachment 258836 [details] [PATCH] Proposed Fix Clearing flags on attachment: 258836 Committed r188353: <http://trac.webkit.org/changeset/188353>
WebKit Commit Bot
Comment 15 2015-08-12 13:21:03 PDT
All reviewed patches have been landed. Closing bug.
Joseph Pecoraro
Comment 16 2015-08-12 13:25:39 PDT
> 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.
Alexey Proskuryakov
Comment 17 2015-08-13 09:55:21 PDT
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?
Blaze Burg
Comment 18 2015-08-13 11:17:23 PDT
(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?
Blaze Burg
Comment 19 2015-08-13 14:12:09 PDT
(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.
Joseph Pecoraro
Comment 20 2015-08-13 14:42:01 PDT
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 =)
Radar WebKit Bug Importer
Comment 21 2015-08-13 14:42:17 PDT
Joseph Pecoraro
Comment 22 2015-08-13 18:31:19 PDT
(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
Note You need to log in before you can comment on or make changes to this bug.