Bug 91146

Created attachment 152060 [details] Patch

dgrogan@, alecflett@ - please take a look The enum naming is lame. Maybe rename EarlyTask => SynchronousTask, NormalTask => AsynchronousTask ? (But that's a lie, as the former is still not really synchronous.)

while I think this is *a* solution, I'm in the middle of rewriting this stuff in bug 91925 such that the end result happens in IDBObjectStore.cpp. This looks fine to me but I'm a bit dreading the fact that I have additional tests to pass when I finish working, and we'll likely want to remove the 'Early' stuff once that has landed (since it would have been the only consumer of it)

I guess I'm wondering, can we delay fixing this until bug 91125 lands? It may, just *may* be a lot more straightforward there.

I'm okay with delaying. I don't think this is high priority, but once the fix occurred to me I had to implement it.

heh, at the very least we have some good test cases now.

ok, I've finally got this test running with my createIndex() work, and the third case (verifyPutThenCreate()) is I think inline with the spec. In particular, the test checks if the put request results in an error callback, but I think all that is really supposed to happen is that the transaction should abort. A few choice bits from the spec: > If the referenced object store already contains data which violates these constraints, this must not cause the implementation of createIndex to throw an exception or affect what it returns. The implementation must still create and return an IDBIndex object. Instead the implementation must queue up an operation to abort the "versionchange" transaction which was used for the createIndex call. And: > In some implementations it's possible for the implementation to asynchronously run into problems creating the index after the createIndex function has returned. For example in implementations where metadata about the newly created index is queued up to be inserted into the database asynchronously, or where the implementation might need to ask the user for permission for quota reasons. Such implementations must still create and return an IDBIndex object. Instead, once the implementation realizes that creating the index has failed, it must abort the transaction using the steps for aborting a transaction using the appropriate error as error parameter. For example if creating the index failed due to quota reasons, QuotaError must be used as error and if the index can't be created due to unique constraints, ConstraintError must be used as error. When I look at "steps for aborting a transaction" I see that only outstanding requests should fire an error but as I understand it, the 2nd put() is no longer outstanding

(In reply to comment #7) > ok, I've finally got this test running with my createIndex() work, and the third case (verifyPutThenCreate()) is I think inline with the spec. > > In particular, the test checks if the put request results in an error callback, but I think all that is really supposed to happen is that the transaction should abort. ... > When I look at "steps for aborting a transaction" I see that only outstanding requests should fire an error but as I understand it, the 2nd put() is no longer outstanding In that test, the abort SHOULD have nothing to do with the creation of the index. It should behave the same as verifyPreExistingIndex(). In script, the call order is: 1. store.put(1) 2. store.put(2) 3. store.createIndex() But in terms of when the actual requests are processed, it should be: 1. store.createIndex() // synchronous operation 2. store.put(1) // async request processed 3. store.put(2) // async request processed And here's what should happen (and works with either the original patch, or in Firefox): 1. The index creation occurs before either of the requests are processed. 2. The first request is then processed, and the store and index are updated. 3. The second request is then processed, and the store fails due to the index constraint violation. This causes the error to be fired at the request itself - not because of the abort, but because of the constraint violation. If preventDefault() was called on the error the transaction would not abort (and we should add a test for that!), but it isn't, so that constraint error causes the transaction to abort.

The thing is, the createIndex() itself to create the sort of stub-index is synchronous, but as the spec says, the population can happen asynchronously, so what I see is: 1. store.createIndex() // synchronous operation 2. store.put(1) // async request processed 3. store.put(2) // async request processed 4. createIndex population // async request processed, transaction aborted I guess I'm reading the spec that index population *can* happen asynchronously, and the only requirement is that the transaction aborts. I think it's better to populate the index asynchronously so that we don't have to lock up the renderer. (because to do that we'd have to hold the renderer thread hostage while we wait for indexing to finish) The only way to do this asynchronously but cleanly in our implementation is to run things in the order above. (believe me I tried to do it otherwise!)

(In reply to comment #9) > The thing is, the createIndex() itself to create the sort of stub-index is synchronous, but as the spec says, the population can happen asynchronously, so what I see is: > > 1. store.createIndex() // synchronous operation > 2. store.put(1) // async request processed > 3. store.put(2) // async request processed > 4. createIndex population // async request processed, transaction aborted > > I guess I'm reading the spec that index population *can* happen asynchronously, and the only requirement is that the transaction aborts. I think it's better to populate the index asynchronously so that we don't have to lock up the renderer. (because to do that we'd have to hold the renderer thread hostage while we wait for indexing to finish) The only way to do this asynchronously but cleanly in our implementation is to run things in the order above. (believe me I tried to do it otherwise!) I agree the index population can and should be async. Why not simply process the puts after the index population? That's how the patch attached to this bug did it - a secondary queue is introduced; index population requests go on the "hi priority" queue, others go on the "low priority" queue. The renderer can't be held hostage, but it's not responsible for processing the async requests. So far as I can tell, the big problem is what we've known about since the "pre-compute the index keys" refactor began: the pre-computed keys may be rendered invalid by index changes, and the browser may need to call back in to the renderer to compute them.

(sorry I put more explanation in bug 91125 - I have been basically trying to make the early/normal task queue stuff work for the last week...)

Alec followed up with the standards list, and this interpretation of the spec was rejected. (Which is good - simplifies the implementation!). bug 91125 covers the the appropriate cases with a derivative of the tests here, so resolving this issue.