IndexedDB: Improve error messages
Created attachment 178686 [details] Patch
Josh and Alec, could you take a look?
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)."
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"
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.
Created attachment 178918 [details] Patch
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 'æ±ⰠåÂâ æ¼¢ ðÂâž'.]
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 'æ± å æ¼¢ ð'.]
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 漢.
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.
(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?)
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.
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.
Created attachment 179180 [details] Patch
Created attachment 179181 [details] Patch
(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?
(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.
Comment on attachment 179181 [details] Patch Clearing flags on attachment: 179181 Committed r137635: <http://trac.webkit.org/changeset/137635>
All reviewed patches have been landed. Closing bug.