Bug 104624 – IndexedDB: Improve error messages

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[David Grogan]

IndexedDB: Improve error messages

[David Grogan]

Created attachment 178686 [details]
Patch

[David Grogan]

Josh and Alec, could you take a look?

[Joshua Bell]

Comment on attachment 178686 [details]
Patch

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

lgtm with nits/suggestions

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:304
> +        RefPtr<IDBDatabaseError> error = IDBDatabaseError::create(IDBDatabaseException::UnknownError, String::format("Internal error creating object store %s.", m_objectStore->name().utf8().data()));

If you haven't, can you try inducing one of these errors locally with an object store with a non-ASCII name to make sure the encoding/formatting of the name makes it through correctly?

Suggest quotes around the name in the output string. (I think we do that in other error messages, but whatever is consistent with existing messages.)

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:305
> +        transaction->abort(error);

Could be error.release()

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:363
>          transaction->abort(error);

Not new in this patch, but this could be error.release()

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:625
> +            RefPtr<IDBDatabaseError> error = IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Connection is closing.");

This should be moved outside the loop.

> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:85
> +        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error opening backing store for getDatabaseNames."));

Should probably use "webkitGetDatabaseNames"

> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:154
> +            callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error opening backing store for open."));

This might be more readable as "...for indexedDB.open()." or "...for IDBFactory.open()." (and maybe apply that pattern above/below too)

> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:108
> +    abort(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error, no error specified."));

Is this called anywhere? If not, maybe remove this method entirely? If it is, I think this reads a bit like a "Daily WTF Error'd" entry, so suggest "Internal error (unknown cause)."

[Alec Flett]

Comment on attachment 178686 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:348
> +        RefPtr<IDBDatabaseError> error = IDBDatabaseError::create(IDBDatabaseException::UnknownError, String::format("Internal error deleting object store %s.", m_objectStore->name().utf8().data()));

Is there any precident for putting quotes around user data like this?  i.e. "Internal error deleting object store 'store'" instead of "Internal error deleting object store store"

[David Grogan]

Comment on attachment 178686 [details]
Patch

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

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:304
>> +        RefPtr<IDBDatabaseError> error = IDBDatabaseError::create(IDBDatabaseException::UnknownError, String::format("Internal error creating object store %s.", m_objectStore->name().utf8().data()));
> 
> If you haven't, can you try inducing one of these errors locally with an object store with a non-ASCII name to make sure the encoding/formatting of the name makes it through correctly?
> 
> Suggest quotes around the name in the output string. (I think we do that in other error messages, but whatever is consistent with existing messages.)

Yeah, a trial run with non-ascii characters is a good idea.

Quotes added.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:305
>> +        transaction->abort(error);
> 
> Could be error.release()

Done. That just reduces ref-count churn, right?  Or does it do something else?

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:348
>> +        RefPtr<IDBDatabaseError> error = IDBDatabaseError::create(IDBDatabaseException::UnknownError, String::format("Internal error deleting object store %s.", m_objectStore->name().utf8().data()));
> 
> Is there any precident for putting quotes around user data like this?  i.e. "Internal error deleting object store 'store'" instead of "Internal error deleting object store store"

Quotes added.

>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:625
>> +            RefPtr<IDBDatabaseError> error = IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Connection is closing.");
> 
> This should be moved outside the loop.

Done.

>> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:85
>> +        callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error opening backing store for getDatabaseNames."));
> 
> Should probably use "webkitGetDatabaseNames"

Done.

>> Source/WebCore/Modules/indexeddb/IDBFactoryBackendImpl.cpp:154
>> +            callbacks->onError(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error opening backing store for open."));
> 
> This might be more readable as "...for indexedDB.open()." or "...for IDBFactory.open()." (and maybe apply that pattern above/below too)

Done.

>> Source/WebCore/Modules/indexeddb/IDBTransactionBackendImpl.cpp:108
>> +    abort(IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error, no error specified."));
> 
> Is this called anywhere? If not, maybe remove this method entirely? If it is, I think this reads a bit like a "Daily WTF Error'd" entry, so suggest "Internal error (unknown cause)."

I tried to remove it but it is indeed called. When script calls transaction->abort(), it ends up here, but AFAICT that's the only incoming path.

Message changed.

[David Grogan]

Created attachment 178918 [details]
Patch

[David Grogan]

Comment on attachment 178686 [details]
Patch

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

>>> Source/WebCore/Modules/indexeddb/IDBDatabaseBackendImpl.cpp:304
>>> +        RefPtr<IDBDatabaseError> error = IDBDatabaseError::create(IDBDatabaseException::UnknownError, String::format("Internal error creating object store %s.", m_objectStore->name().utf8().data()));
>> 
>> If you haven't, can you try inducing one of these errors locally with an object store with a non-ASCII name to make sure the encoding/formatting of the name makes it through correctly?
>> 
>> Suggest quotes around the name in the output string. (I think we do that in other error messages, but whatever is consistent with existing messages.)
> 
> Yeah, a trial run with non-ascii characters is a good idea.
> 
> Quotes added.

UTF8 whopping fail.

Line from js file:
evalAndLog("db.createObjectStore('汉  字  漢  𝄞', {keyPath: 'a'})");

Result in chrome:
db.createObjectStore('汉  å­—  æ¼¢  ð„ž', {keyPath: 'a'})
FAIL Abort function called unexpectedly! Message: [Internal error creating object store 'æ±Ⱐ Ã¥Â­â  Ã¦Â¼Â¢  ðÂâž'.]

[David Grogan]

After specifying charset="utf-8" in the <script> tag, we get

evalAndLog("db.createObjectStore('汉  字  漢  𝄞', {keyPath: 'a'})");


db.createObjectStore('汉  字  漢  𝄞', {keyPath: 'a'})
FAIL Abort function called unexpectedly! Message: [Internal error creating object store 'æ±  å­  æ¼¢  ð'.]

[David Grogan]

A literal chinese character inserted into the error message in the source file is also garbled:

RefPtr<IDBDatabaseError> error = IDBDatabaseError::create(IDBDatabaseException::UnknownError, "Internal error creating object 漢 sto")

causes

FAIL Abort function called unexpectedly! Message: [Internal error creating object æ¼¢ sto]

æ¼¢ are the latin1/extended ascii symbols for the three individual bytes of the utf8 representation of 漢.

[David Grogan]

Tony, could you review this?

It appears that something further down the chain from IDB is misinterpreting these bytes so I don't want this patch to block on it.

[Joshua Bell]

(In reply to comment #10)
> It appears that something further down the chain from IDB is misinterpreting these bytes so I don't want this patch to block on it.

Agreed - we should file a bug and track it down at some point. (I wouldn't bother with a FIXME since it's in so many places. I wonder if there are non-IDB examples that work or are equally broken?)

[David Grogan]

Comment on attachment 178918 [details]
Patch

String::format is the problem. It assumes its result is only 8-bit characters:
StringImpl::create(reinterpret_cast<const LChar*>(buffer.data()), len)

I will use a series of String::append instead of format if nothing else. But that can wait, this patch can go in as is.

[Tony Chang]

Comment on attachment 178918 [details]
Patch

Are there tests for any of these error strings?  Would be nice to have some with unicode names even if they are failing right now.

[David Grogan]

Created attachment 179180 [details]
Patch

[David Grogan]

Created attachment 179181 [details]
Patch

[David Grogan]

(In reply to comment #13)
> (From update of attachment 178918 [details])
> Are there tests for any of these error strings?  Would be nice to have some with unicode names even if they are failing right now.

We don't have tests for many of them, but I added a chinese character to one of the few we do have. (The tool behind "Review Patch" also does a utf8->ascii conversion, but you can see the original characters in the raw Patch.)

Alec, I plumbed errorMessage from verifyIndexKeys up to script. There was a FIXME for it, it seemed easy, and it was the only way I could test this utf8 stuff. Was there any reason you were holding off on plumbing it through?

[Alec Flett]

(In reply to comment #16)
> Alec, I plumbed errorMessage from verifyIndexKeys up to script. There was a FIXME for it, it seemed easy, and it was the only way I could test this utf8 stuff. Was there any reason you were holding off on plumbing it through?

I'm pretty sure when that FIXME first went in, the plumbing wasn't completely there.... like abort only took an error code or something. So what you have here looks great.

[WebKit Review Bot]

Comment on attachment 179181 [details]
Patch

Clearing flags on attachment: 179181

Committed r137635: <http://trac.webkit.org/changeset/137635>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.