Bug 77374 – IndexedDB: Raise exception during add/put call if autoIncrement key insertion will fail

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joshua Bell]

For generated keys (i.e. autoIncrement) the generation step should occur within the async task within the transaction (per discussion on mailing list). 

Currently the key injection call is deferred until the asynchronous task is executed during the transaction. This leaves open a hole where the value may not support key injection (e.g. the value is a string, on which properties can not be set). In this case, Chrome currently fires an error event at the IDBRequest.

This may require that in put() we check to see if a value can be inserted (by inserting a dummy value?) and then in putInternal() we run the key generation step and actually insert the key.

[Joshua Bell]

Don't try and fix this until the refactoring in https://bugs.webkit.org/show_bug.cgi?id=76952 lands

[Joshua Bell]

Created attachment 125363 [details]
Patch

[David Grogan]

Comment on attachment 125363 [details]
Patch

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

LGTM

I think you can delete at least the FIXME at line 211.  Can you change the line 214-217 if (!valueAfterInjection) clause to an ASSERT?

I'm annoyed on behalf of the theoretical developer that we can't give error messages along with the exception so they don't have to consult the spec or other reference to figure out what, e.g., DATA_ERR actually means.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:156
> +            if (autoIncrement && !keyPathKey) {

Bummer that we have to do another sync call out to the utility process while the renderer process is synchronously waiting for the browser process.  Hopefully idb.next saves us from this madness soon.

> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:-171
> -        const bool hasKeyPath = !objectStore->m_keyPath.isNull();

sanity check: these are strictly stylistic?  It seems lame that we assign objectStore = this and then use objectStore instead of just passing PassRefPtr<IDBObjectStoreBackendImpl>(this) to transaction->scheduleTask.  Though maybe I'm missing a reason that that's not desirable.

[Joshua Bell]

Comment on attachment 125363 [details]
Patch

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

> I think you can delete at least the FIXME at line 211.  Can you change the line 214-217 if (!valueAfterInjection) clause to an ASSERT?

Will do.

> I'm annoyed on behalf of the theoretical developer that we can't give error messages along with the exception so they don't have to consult the spec or other reference to figure out what, e.g., DATA_ERR actually means.

No argument there. Even worse, in the latest revs of the spec several of the IDB-specific exception types are replaced with generic DOM4 exception types. Wheee. (I should file a bug on that, actually...)

>> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:156
>> +            if (autoIncrement && !keyPathKey) {
> 
> Bummer that we have to do another sync call out to the utility process while the renderer process is synchronously waiting for the browser process.  Hopefully idb.next saves us from this madness soon.

Indeed. :(

(As an aside, there's an incremental step we could consider: we could pull all of these precondition checks to the front end where they could be done in-proc. The only reason we can't do it trivially right now is that the metadata about the store is only stored in the back end and lookups like this->autoIncrement() are sync IPC call to the back end.)

>> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:-171
>> -        const bool hasKeyPath = !objectStore->m_keyPath.isNull();
> 
> sanity check: these are strictly stylistic?  It seems lame that we assign objectStore = this and then use objectStore instead of just passing PassRefPtr<IDBObjectStoreBackendImpl>(this) to transaction->scheduleTask.  Though maybe I'm missing a reason that that's not desirable.

Yes, stylistic.

Most of the uses in put() stem from my moving of checks from the static putInternal() function to the put() method. But the "objectStore = this" pattern is used throughout the IDB*BackendImpl.cpp files. The "objectStore = this" pattern is used in several methods; it would only be necessary if the scheduleTask call would deref |this| (which it would if the transaction was finished) and our refcount was already 0 (which it shouldn't be). I'm not sure it was put in for a reason, or just cargo cult programming.

[David Grogan]

(In reply to comment #4)
> >> Source/WebCore/storage/IDBObjectStoreBackendImpl.cpp:-171
> >> -        const bool hasKeyPath = !objectStore->m_keyPath.isNull();
> > 
> > sanity check: these are strictly stylistic?  It seems lame that we assign objectStore = this and then use objectStore instead of just passing PassRefPtr<IDBObjectStoreBackendImpl>(this) to transaction->scheduleTask.  Though maybe I'm missing a reason that that's not desirable.
> 
> Yes, stylistic.
> 
> Most of the uses in put() stem from my moving of checks from the static putInternal() function to the put() method. But the "objectStore = this" pattern is used throughout the IDB*BackendImpl.cpp files. The "objectStore = this" pattern is used in several methods; it would only be necessary if the scheduleTask call would deref |this| (which it would if the transaction was finished) and our refcount was already 0 (which it shouldn't be). I'm not sure it was put in for a reason, or just cargo cult programming.

I think the refcount on IDBObjectStoreBackendImpl can be decremented on the webkit thread in response to an incoming IPC message.  The PassRefPtr needs to be used to guard against being deallocated in between scheduling the task and executing it.

git blame directed me here: https://bugs.webkit.org/show_bug.cgi?id=46823 where Andrei talks about it.

[Joshua Bell]

Created attachment 125435 [details]
Patch

[Joshua Bell]

(In reply to comment #5)
> I think the refcount on IDBObjectStoreBackendImpl can be decremented on the webkit thread in response to an incoming IPC message.  The PassRefPtr needs to be used to guard against being deallocated in between scheduling the task and executing it.

*shakes fist at IPC* 

Okay. Although not necessary, I rolled back the stylistic changes in the latest patch so they're consistent. I added the ASSERT but left the "if" in place, changing the error type to UNKNOWN_ERR.

[Joshua Bell]

Created attachment 125448 [details]
Patch

[Joshua Bell]

(In reply to comment #8)
> Created an attachment (id=125448) [details]
> Patch

Rebased & fixed merge conflict with other patch.

tony@, can you review?

[WebKit Review Bot]

Comment on attachment 125448 [details]
Patch

Clearing flags on attachment: 125448

Committed r106830: <http://trac.webkit.org/changeset/106830>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.