Bug 71575 – WebSQLDatabase could use some better error reporting.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Michael Nordman]

I'm getting reports of errors opening databases and starting transactions, but there's not much to go on in these reports beyond the WebSQLError code which isn't very informative. See http://code.google.com/p/chromium/issues/detail?id=98939

I plan on adding some console logging and also building a way for errors in webcore to get reported in chrome's 'histogram' logging mechanism to aggregate information about specific errors at specific callsites (see about:histogram in chrome).

[Michael Nordman]

Created attachment 113683 [details]
addMessage from bgThread

[Gyuyoung Kim]

Comment on attachment 113683 [details]
addMessage from bgThread

Attachment 113683 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10337040

[Michael Nordman]

Created attachment 113697 [details]
addMessage from bgThread

make efl happy with this too (hopefully)

[WebKit Review Bot]

Attachment 113697 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/dom/ScriptExecutionContext.h:172:  The parameter name "context" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Nordman]

Created attachment 113698 [details]
addMessage from bgThread

style change

[Michael Nordman]

horray for bot happiness :)

[Nate Chapin]

Comment on attachment 113698 [details]
addMessage from bgThread

View in context: https://bugs.webkit.org/attachment.cgi?id=113698&action=review

Ok.

What happens when addMessage() is called currently?

> Source/WebCore/ChangeLog:9
> +        No new tests.
> +

Is this testable yet?

[Michael Nordman]

> What happens when addMessage() is called currently?

Its not called on anything other than the 'context' thread, it's not thread safe.

> Is this testable yet?

Nothing calls this method on a bg thread yet, when there are new calls to it that affect console output related layout tests (whose expectations include the console output) will get updated to check for that output.

[WebKit Review Bot]

Comment on attachment 113698 [details]
addMessage from bgThread

Clearing flags on attachment: 113698

Committed r99328: <http://trac.webkit.org/changeset/99328>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Nordman]

reopening for subsequent patches

[Michael Nordman]

Created attachment 114192 [details]
dberrors

[WebKit Review Bot]

Comment on attachment 114192 [details]
dberrors

Attachment 114192 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10374281

New failing tests:
storage/test-authorizer.html
storage/change-version-handle-reuse.html
storage/open-database-creation-callback.html

[Michael Nordman]

Created attachment 114199 [details]
dberrors

[WebKit Review Bot]

Comment on attachment 114199 [details]
dberrors

Attachment 114199 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10377117

New failing tests:
storage/test-authorizer.html
storage/change-version-handle-reuse.html
storage/open-database-creation-callback.html

[Michael Nordman]

Created attachment 114367 [details]
dberrors

[Michael Nordman]

Created attachment 114558 [details]
dberrors

[WebKit Review Bot]

Attachment 114558 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/ChangeLog', u'Source/WebCor..." exit_code: 1

Source/WebCore/storage/AbstractDatabase.h:98:  The parameter name "ec" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Michael Nordman]

Created attachment 114560 [details]
dberrors

[Michael Nordman]

Created attachment 114581 [details]
dberrors

update layout tests

[Michael Nordman]

Created attachment 114589 [details]
dberrors

layout tests for .lastErrorMessage

[Michael Nordman]

ptal, i still want to return a formatted openDatabase[Sync] error message to the caller in some way, in this patch those messages only get put on the js console, but this patch is large enough already.

[Michael Nordman]

adding some reviewers to the cc list

[David Levin]

Comment on attachment 114589 [details]
dberrors

View in context: https://bugs.webkit.org/attachment.cgi?id=114589&action=review

> Source/WebCore/ChangeLog:11
> +        Tests are in the works, just not in this snapshot yet.

I'm confused, so is this ready for review?

> Source/WebCore/ChangeLog:15
> +        (WebCore::SQLiteDatabase::open):

The changelog ideally guides the reviewer through why changes were done in particular function so they don't have to reconstruct it.

For example, I don't know why m_openError was added in here and being used. I'm sure this is obvious to you, and I could piece it together by spending enough time on it but I'd rather not :).

[David Levin]

Comment on attachment 114589 [details]
dberrors

Per previous comments.

[Michael Nordman]

> > +        Tests are in the works, just not in this snapshot yet.
> I'm confused, so is this ready for review?

I just forgot to update the change log, please see the updated tests that are also in the patch.

> > +        (WebCore::SQLiteDatabase::open):
> The changelog ideally guides the reviewer through why changes were done in particular function so they don't have to reconstruct it.
> 
> For example, I don't know why m_openError was added in here and being used. I'm sure this is obvious to you, and I could piece it together by spending enough time on it but I'd rather not :).

Sure, i'll add some comments in the ChangeLog and prepare a new patch.

To answer your question here, the reason is to allow the lastError() and lastErrorMsg() methods to reflect errors experienced in the open() method as well as the other methods... right details of errors in open() are not available to consumers.

[Michael Nordman]

Created attachment 114781 [details]
dberrors

fixed changelog comments, added console logging for vacuum errors

[Michael Nordman]

Created attachment 114782 [details]
dberrors

really this time... log to the console errors when vacuuming

[Michael Nordman]

Created attachment 114784 [details]
dberrors

reverted the unintentional xcode project file changes

[David Levin]

Comment on attachment 114784 [details]
dberrors

View in context: https://bugs.webkit.org/attachment.cgi?id=114782&action=review

Tnis could go in as is. Here are some nice to haves but not critical:
 Would be nice to clean up the duplicate strings.

> Source/WebCore/storage/ChangeVersionWrapper.cpp:55
> +                                      transaction->database()->sqliteDatabase().lastErrorMsg());

Somehow the repeat of "transaction->database()->sqliteDatabase()" bothers me. (It would be nice to do only once.)

> Source/WebCore/storage/ChangeVersionWrapper.cpp:74
> +                                      transaction->database()->sqliteDatabase().lastErrorMsg());

Ditto.

[Early Warning System Bot]

Comment on attachment 114784 [details]
dberrors

Attachment 114784 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/10450622

[Michael Nordman]

Comment on attachment 114784 [details]
dberrors

View in context: https://bugs.webkit.org/attachment.cgi?id=114784&action=review

> Source/WebCore/storage/AbstractDatabase.cpp:536
> +        if (result != SQLResultOK)

ooops... got the wrong symbol here s/b SQLResultOk (lower k)

>> Source/WebCore/storage/ChangeVersionWrapper.cpp:55
>> +                                      transaction->database()->sqliteDatabase().lastErrorMsg());
> 
> Somehow the repeat of "transaction->database()->sqliteDatabase()" bothers me. (It would be nice to do only once.)

done

>> Source/WebCore/storage/ChangeVersionWrapper.cpp:74
>> +                                      transaction->database()->sqliteDatabase().lastErrorMsg());
> 
> Ditto.

done

[Gyuyoung Kim]

Comment on attachment 114784 [details]
dberrors

Attachment 114784 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/10396584

[Michael Nordman]

Created attachment 114805 [details]
dberrors

[Michael Nordman]

Created attachment 114978 [details]
dberrors (patch for landing)

patch for landing

[WebKit Review Bot]

Comment on attachment 114978 [details]
dberrors (patch for landing)

Clearing flags on attachment: 114978

Committed r100172: <http://trac.webkit.org/changeset/100172>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Nordman]

reopening for more

[David Levin]

(In reply to comment #38)
> reopening for more

Please use a new bug for new patches. (Just like you'd have a new code review issue in chromium land.)

Otherwise it gets really confusing. (Note how there are 38 comments and 14 patches in this bug already.)

[Michael Nordman]

continuing work in https://bugs.webkit.org/show_bug.cgi?id=73258