Bug 147844 – Web Inspector: Not receiving responses for async request IndexedDB.requestDatabaseNames

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

* 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

[Joseph Pecoraro]

Created attachment 258670 [details]
[PATCH] Proposed Fix

[Blaze Burg]

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 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 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 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 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]

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 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 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]

(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 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]

Created attachment 258836 [details]
[PATCH] Proposed Fix

[Blaze Burg]

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 on attachment 258836 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 258836

Committed r188353: <http://trac.webkit.org/changeset/188353>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

> 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]

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]

(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]

(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]

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]

<rdar://problem/22276450>

[Joseph Pecoraro]

(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