WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
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
Details
[PATCH] Proposed Fix
(7.35 KB, patch)
2015-08-12 12:01 PDT
,
Joseph Pecoraro
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
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
<
rdar://problem/22276450
>
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.
Top of Page
Format For Printing
XML
Clone This Bug