Bug 53728 - indexeddb: make setVersion fire blocked event if other connections are open
Summary: indexeddb: make setVersion fire blocked event if other connections are open
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-02-03 15:59 PST by David Grogan
Modified: 2011-02-16 16:55 PST (History)
2 users (show)

See Also:


Attachments
Patch (31.93 KB, patch)
2011-02-03 16:06 PST, David Grogan
no flags Details | Formatted Diff | Diff
obsoleted Patch 1 (37.16 KB, patch)
2011-02-07 19:00 PST, David Grogan
no flags Details | Formatted Diff | Diff
obsoleted patch 2 (46.06 KB, patch)
2011-02-09 13:26 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (47.82 KB, patch)
2011-02-11 15:36 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (48.65 KB, patch)
2011-02-11 16:52 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (47.99 KB, patch)
2011-02-14 13:52 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (47.91 KB, patch)
2011-02-14 16:07 PST, David Grogan
no flags Details | Formatted Diff | Diff
Patch (47.48 KB, patch)
2011-02-16 14:39 PST, David Grogan
jorlow: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description David Grogan 2011-02-03 15:59:33 PST
indexeddb: make setVersion fire blocked event if other connections are open
Comment 1 David Grogan 2011-02-03 16:06:24 PST
Created attachment 81132 [details]
Patch
Comment 2 David Grogan 2011-02-03 16:10:58 PST
Comment on attachment 81132 [details]
Patch

This first patch doesn't change any behavior, it's just some architectural support.  Sending it now to get early feedback from Jeremy.
Comment 3 Jeremy Orlow 2011-02-03 16:28:08 PST
Comment on attachment 81132 [details]
Patch

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

not too far off

this will require a couple stages to land, but let's get it all good first and then we can worry aobut that

> Source/WebCore/WebCore.gypi:4006
> +            'storage/IDBBlockedEvent.cpp',

i think you'll need an idl, right?

> Source/WebCore/storage/IDBBlockedEvent.cpp:2
> + * Copyright (C) 2010 Google Inc. All rights reserved.

Use this here and elsewhere: http://webkit.org/coding/bsd-license.html

2011

> Source/WebCore/storage/IDBBlockedEvent.cpp:45
> +    : IDBEvent(eventNames().abortEvent, 0) // FIXME: set the source?

implement the fixme....this should take in a PassRefPtr<IDBAny> source attribute and pass it through to IDBEvent

> Source/WebCore/storage/IDBDatabase.cpp:97
> +    RefPtr<IDBVersionChangeRequest> request = IDBVersionChangeRequest::create(context, IDBAny::create(this), 0);

get rid of the 0 since we'll never have a transaction to pass in

> Source/WebCore/storage/IDBDatabase.cpp:135
> +      return;

4 spaces

> Source/WebCore/storage/IDBDatabase.cpp:137
> +    m_backend->decrementConnectionCount();

why not just leave this "close()"?  also shouldn't you pass "this" in?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:98
> +    , m_openConnectionCount(0)

I thouhght you were going to maintain a set since you'll need that in the next patch?

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:215
> +    // Call onblocked here if connection count is >0

Use complete sentences.

> Source/WebCore/storage/IDBFactoryBackendImpl.cpp:216
> +        (it->second)->incrementConnectionCount();

()'s unneeded

> Source/WebCore/storage/IDBRequest.h:72
> +    virtual void onBlocked() { ASSERT_NOT_REACHED(); }

maybe move the body into the .cpp since a lot of files compile this

> Source/WebCore/storage/IDBVersionChangeRequest.cpp:34
> +#include "Event.h"

Please cut down this list.

> Source/WebCore/storage/IDBVersionChangeRequest.idl:33
> +        EventTarget

inherit from IDBRequest...i think it should mostly do the right thing...?

> Source/WebCore/storage/IDBVersionChangeRequest.idl:37
> +        const unsigned short LOADING = 1;

get rid of this stuff?

> Source/WebCore/storage/IDBVersionChangeRequest.idl:42
> +        attribute EventListener onsuccess;

remove onsuccess and on error

> Source/WebCore/storage/IDBVersionChangeRequest.idl:46
> +        // EventTarget interface

get rid of this stuff?

> Source/WebKit/chromium/src/IDBCallbacksProxy.cpp:111
> +{

call the IDBRequest version
Comment 4 David Grogan 2011-02-07 19:00:58 PST
Created attachment 81565 [details]
obsoleted Patch 1
Comment 5 David Grogan 2011-02-07 19:07:13 PST
Comment on attachment 81565 [details]
obsoleted Patch 1

This isn't ready to be committed, but close.

I don't think the call to close() in IDBDatabase::~IDBDatabase() is right.  If you load the layout test in chromium everything is fine until you hit refresh.  The destructor on the IDBDatabase object from the first page load isn't called until after the new page loads, causing setVersion to call blocked when it's supposed to succeed.

I'm not sure what to do about this.
Comment 6 Jeremy Orlow 2011-02-07 19:32:39 PST
Comment on attachment 81565 [details]
obsoleted Patch 1

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

closer

> LayoutTests/storage/indexeddb/set_version_blocked.html:1
> +<html>

You need to create expected results.

> LayoutTests/storage/indexeddb/set_version_blocked.html:20
> +    if ('webkitIndexedDB' in window) {

no {} for one liner

> LayoutTests/storage/indexeddb/set_version_blocked.html:58
> +function setVersion(tries) {

newline

> LayoutTests/storage/indexeddb/set_version_blocked.html:65
> +function unexpectedBlockCallback() {

put in the shared.js file...and call it "Blocked" not "Block"

> Source/WebCore/dom/EventTarget.h:143
> +        virtual IDBVersionChangeRequest* toIDBVersionChangeRequest();

alpha order

> Source/WebCore/storage/IDBDatabase.cpp:135
> +    if (m_noNewTransactions)

Stuff like this should be tested in the layout test.

> Source/WebCore/storage/IDBDatabaseBackendImpl.h:65
> +    virtual void open();

You'll need to add this to the WebKit:: stuff.

> Source/WebCore/storage/IDBRequest.cpp:190
> +void IDBRequest::scheduleBlockedEvent()

You need to sync your repo.  This has all changed a bunch.

> Source/WebCore/storage/IDBVersionChangeRequest.h:32
> +#include "Timer.h"

Why is this needed?

> Source/WebCore/storage/IDBVersionChangeRequest.h:33
> +#include <wtf/Vector.h>

why is this needed?

> Source/WebCore/storage/IDBVersionChangeRequest.h:37
> +class IDBTransactionBackendInterface;

why is this needed?

> Source/WebCore/storage/IDBVersionChangeRequest.h:41
> +    static PassRefPtr<IDBVersionChangeRequest> create(ScriptExecutionContext* context, PassRefPtr<IDBAny> source) { return adoptRef(new IDBVersionChangeRequest(context, source)); }

lately i've been shying away from inlining these.  Maybe move it?

> Source/WebCore/storage/IDBVersionChangeRequest.h:43
> +    virtual void onBlocked();

usually we do a newline between the constructor/destructor stuff and other stuff

> Source/WebCore/storage/IDBVersionChangeRequest.h:49
> +

no blank line
Comment 7 David Grogan 2011-02-09 13:26:03 PST
Created attachment 81862 [details]
obsoleted patch 2
Comment 8 David Grogan 2011-02-09 13:30:27 PST
Comment on attachment 81565 [details]
obsoleted Patch 1

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

>> LayoutTests/storage/indexeddb/set_version_blocked.html:1
>> +<html>
> 
> You need to create expected results.

done

>> LayoutTests/storage/indexeddb/set_version_blocked.html:20
>> +    if ('webkitIndexedDB' in window) {
> 
> no {} for one liner

done

>> LayoutTests/storage/indexeddb/set_version_blocked.html:58
>> +function setVersion(tries) {
> 
> newline

done

>> LayoutTests/storage/indexeddb/set_version_blocked.html:65
>> +function unexpectedBlockCallback() {
> 
> put in the shared.js file...and call it "Blocked" not "Block"

done

>> Source/WebCore/dom/EventTarget.h:143
>> +        virtual IDBVersionChangeRequest* toIDBVersionChangeRequest();
> 
> alpha order

done

>> Source/WebCore/storage/IDBDatabase.cpp:135
>> +    if (m_noNewTransactions)
> 
> Stuff like this should be tested in the layout test.

done

>> Source/WebCore/storage/IDBDatabaseBackendImpl.h:65
>> +    virtual void open();
> 
> You'll need to add this to the WebKit:: stuff.

This round?  It seems that nothing would use it when only doing ref counting.

>> Source/WebCore/storage/IDBRequest.cpp:190
>> +void IDBRequest::scheduleBlockedEvent()
> 
> You need to sync your repo.  This has all changed a bunch.

done

>> Source/WebCore/storage/IDBVersionChangeRequest.h:32
>> +#include "Timer.h"
> 
> Why is this needed?

it's not, deleted

>> Source/WebCore/storage/IDBVersionChangeRequest.h:33
>> +#include <wtf/Vector.h>
> 
> why is this needed?

it's not, deleted

>> Source/WebCore/storage/IDBVersionChangeRequest.h:37
>> +class IDBTransactionBackendInterface;
> 
> why is this needed?

it's not, deleted

>> Source/WebCore/storage/IDBVersionChangeRequest.h:41
>> +    static PassRefPtr<IDBVersionChangeRequest> create(ScriptExecutionContext* context, PassRefPtr<IDBAny> source) { return adoptRef(new IDBVersionChangeRequest(context, source)); }
> 
> lately i've been shying away from inlining these.  Maybe move it?

I'm a fan of "no implementation in the .h files"; moved.

>> Source/WebCore/storage/IDBVersionChangeRequest.h:43
>> +    virtual void onBlocked();
> 
> usually we do a newline between the constructor/destructor stuff and other stuff

done

>> Source/WebCore/storage/IDBVersionChangeRequest.h:49
>> +
> 
> no blank line

done
Comment 9 Jeremy Orlow 2011-02-09 13:53:19 PST
(In reply to comment #8)
> (From update of attachment 81565 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=81565&action=review
> >> Source/WebCore/storage/IDBDatabaseBackendImpl.h:65
> >> +    virtual void open();
> > 
> > You'll need to add this to the WebKit:: stuff.
> 
> This round?  It seems that nothing would use it when only doing ref counting.

Oops, you're right.
Comment 10 Jeremy Orlow 2011-02-09 14:13:30 PST
Comment on attachment 81862 [details]
obsoleted patch 2

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

close

> LayoutTests/storage/indexeddb/set_version_blocked.html:31
> +    verifyResult(result);

In new tests, I'm voiding these because at this point they pretty much just add noise

> LayoutTests/storage/indexeddb/set_version_blocked.html:39
> +    verifySuccessEvent(event);

Ditto....fix throughout

> LayoutTests/storage/indexeddb/set_version_blocked.html:42
> +    if (connections.length < 3) {

no {}

> LayoutTests/storage/indexeddb/set_version_blocked.html:44
> +    } else {

no {}

> LayoutTests/storage/indexeddb/set_version_blocked.html:63
> +// Try 1: Close a connection.

Stuff like this should be in shouldBe/testPassed/debug()'s.  The idea is that you should be able to follow along via the on-screen output.

> Source/WebCore/storage/IDBBlockedEvent.cpp:42
> +    : IDBEvent(eventNames().blockedEvent, source, false/*canBubble*/)

space before /*?

> Source/WebCore/storage/IDBBlockedEvent.h:45
> +    virtual String version();

newline

> Source/WebCore/storage/IDBBlockedEvent.h:48
> +    IDBBlockedEvent(PassRefPtr<IDBAny> source, const String& version);

newline

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:217
> +        return;

You need to add this to a queue and start the queue once there's only one connection left.  And put a FIXME to only fire the event if the DB is still open.

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:260
>  void IDBDatabaseBackendImpl::close()

On close, you need to re-try the set-version transaction.

> Source/WebCore/storage/IDBDatabaseBackendImpl.h:65
> +    virtual void open();

shouldn't be virtual.  Group up above with the other non-virtual/not-in-idl stuff (like id)

> Source/WebCore/storage/IDBRequest.h:85
> +    virtual void enqueueEvent(PassRefPtr<Event>);

don't make stuff virtual unless necessary

> Source/WebCore/storage/IDBRequest.h:86
> +    virtual IDBAny* source();

I don't see any reason not to just make this public.  But don't make virtual.

> Source/WebCore/storage/IDBVersionChangeRequest.cpp:40
> +IDBVersionChangeRequest::IDBVersionChangeRequest(ScriptExecutionContext* context, PassRefPtr<IDBAny> source, const String& version)

newline

> Source/WebCore/storage/IDBVersionChangeRequest.h:46
> +    String m_version;

maybe a newline between these?
Comment 11 David Grogan 2011-02-11 15:13:49 PST
Comment on attachment 81862 [details]
obsoleted patch 2

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

>> LayoutTests/storage/indexeddb/set_version_blocked.html:31
>> +    verifyResult(result);
> 
> In new tests, I'm voiding these because at this point they pretty much just add noise

done

>> LayoutTests/storage/indexeddb/set_version_blocked.html:39
>> +    verifySuccessEvent(event);
> 
> Ditto....fix throughout

done

>> LayoutTests/storage/indexeddb/set_version_blocked.html:42
>> +    if (connections.length < 3) {
> 
> no {}

done

>> LayoutTests/storage/indexeddb/set_version_blocked.html:44

> 
> no {}

done

>> LayoutTests/storage/indexeddb/set_version_blocked.html:63

> 
> Stuff like this should be in shouldBe/testPassed/debug()'s.  The idea is that you should be able to follow along via the on-screen output.

gotcha

>> Source/WebCore/storage/IDBBlockedEvent.cpp:42
>> +    : IDBEvent(eventNames().blockedEvent, source, false/*canBubble*/)
> 
> space before /*?

done

>> Source/WebCore/storage/IDBBlockedEvent.h:45
>> +    virtual String version();
> 
> newline

done

>> Source/WebCore/storage/IDBBlockedEvent.h:48
>> +    IDBBlockedEvent(PassRefPtr<IDBAny> source, const String& version);
> 
> newline

done

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:217
>> +        return;
> 
> You need to add this to a queue and start the queue once there's only one connection left.  And put a FIXME to only fire the event if the DB is still open.

done

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:260
>>  void IDBDatabaseBackendImpl::close()
> 
> On close, you need to re-try the set-version transaction.

done

>> Source/WebCore/storage/IDBDatabaseBackendImpl.h:65
>> +    virtual void open();
> 
> shouldn't be virtual.  Group up above with the other non-virtual/not-in-idl stuff (like id)

done

>> Source/WebCore/storage/IDBRequest.h:85
>> +    virtual void enqueueEvent(PassRefPtr<Event>);
> 
> don't make stuff virtual unless necessary

done

>> Source/WebCore/storage/IDBRequest.h:86
>> +    virtual IDBAny* source();
> 
> I don't see any reason not to just make this public.  But don't make virtual.

done

>> Source/WebCore/storage/IDBVersionChangeRequest.cpp:40
>> +IDBVersionChangeRequest::IDBVersionChangeRequest(ScriptExecutionContext* context, PassRefPtr<IDBAny> source, const String& version)
> 
> newline

done

>> Source/WebCore/storage/IDBVersionChangeRequest.h:46
>> +    String m_version;
> 
> maybe a newline between these?

done
Comment 12 David Grogan 2011-02-11 15:36:22 PST
Created attachment 82197 [details]
Patch
Comment 13 Jeremy Orlow 2011-02-11 15:57:29 PST
Comment on attachment 82197 [details]
Patch

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

almost there

> LayoutTests/storage/indexeddb/set_version_queue.html:84
> +    debug("inSetVersion2");

testPassed

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:217
> +        prpCallbacks->onBlocked();

Usually the first thing we do in a function is assign the prp types to normal RefPtr types.  I know we don't do that some in these files, but we should probably try to get back to it.  So move that stuff up and then s/prpC/c/ here.

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:218
> +        RefPtr<PendingSetVersionCall> pendingSetVersionCall = adoptRef(new PendingSetVersionCall);

the class should have a factory and new should be private.

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:219
> +        pendingSetVersionCall->m_callbacks = prpCallbacks;

use callbacks

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:269
> +    if (m_openConnectionCount == 1) {

Just do a return if > 1 so the rest isn't nested.  newline after the return too, i'd think

> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:271
> +            // FIXME: Do something better with ec?

ASSERT(!ec || whatever else it might be that's OK)...and remove the fixme

> Source/WebCore/storage/IDBDatabaseBackendImpl.h:102
> +    struct PendingSetVersionCall : RefCounted<PendingSetVersionCall> {

I'm not sure we really use structs much in WebKit.  Just forward declare the class here and define it in the .cpp
Comment 14 David Grogan 2011-02-11 16:50:08 PST
Comment on attachment 82197 [details]
Patch

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

>> LayoutTests/storage/indexeddb/set_version_queue.html:84
>> +    debug("inSetVersion2");
> 
> testPassed

done

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:217
>> +        prpCallbacks->onBlocked();
> 
> Usually the first thing we do in a function is assign the prp types to normal RefPtr types.  I know we don't do that some in these files, but we should probably try to get back to it.  So move that stuff up and then s/prpC/c/ here.

done

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:218
>> +        RefPtr<PendingSetVersionCall> pendingSetVersionCall = adoptRef(new PendingSetVersionCall);
> 
> the class should have a factory and new should be private.

done

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:219
>> +        pendingSetVersionCall->m_callbacks = prpCallbacks;
> 
> use callbacks

done

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:269
>> +    if (m_openConnectionCount == 1) {
> 
> Just do a return if > 1 so the rest isn't nested.  newline after the return too, i'd think

done

>> Source/WebCore/storage/IDBDatabaseBackendImpl.cpp:271
>> +            // FIXME: Do something better with ec?
> 
> ASSERT(!ec || whatever else it might be that's OK)...and remove the fixme

done.  I don't know what might be ok so went conservative with ASSERT(!ec)

>> Source/WebCore/storage/IDBDatabaseBackendImpl.h:102
>> +    struct PendingSetVersionCall : RefCounted<PendingSetVersionCall> {
> 
> I'm not sure we really use structs much in WebKit.  Just forward declare the class here and define it in the .cpp

done
Comment 15 David Grogan 2011-02-11 16:52:55 PST
Created attachment 82209 [details]
Patch
Comment 16 David Grogan 2011-02-14 13:52:55 PST
Created attachment 82358 [details]
Patch
Comment 17 David Grogan 2011-02-14 13:55:08 PST
Comment on attachment 82358 [details]
Patch

Removes small change to webkit whose bug was https://bugs.webkit.org/show_bug.cgi?id=54329
Comment 18 Jeremy Orlow 2011-02-14 13:58:57 PST
Comment on attachment 82358 [details]
Patch

rubber stamp assuming nothing else changed
Comment 19 Jeremy Orlow 2011-02-14 13:59:33 PST
Comment on attachment 82358 [details]
Patch

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

> Source/WebKit/chromium/ChangeLog:8
> +        * public/WebIDBCallbacks.h:

remove these 2 lines
Comment 20 David Grogan 2011-02-14 16:07:36 PST
Comment on attachment 82358 [details]
Patch

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

>> Source/WebKit/chromium/ChangeLog:8
>> +        * public/WebIDBCallbacks.h:
> 
> remove these 2 lines

done
Comment 21 David Grogan 2011-02-14 16:07:59 PST
Created attachment 82378 [details]
Patch
Comment 22 WebKit Commit Bot 2011-02-14 18:17:09 PST
Comment on attachment 82378 [details]
Patch

Rejecting attachment 82378 [details] from commit-queue.

Failed to run "['./Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '--bot-id=cr-jail-7', 'apply-..." exit_code: 2

Last 500 characters of output:
st.h
patching file Source/WebCore/storage/IDBVersionChangeRequest.idl
patching file Source/WebKit/chromium/ChangeLog
patching file Source/WebKit/chromium/src/IDBCallbacksProxy.cpp
patching file Source/WebKit/chromium/src/IDBCallbacksProxy.h
patching file Source/WebKit/chromium/src/WebIDBCallbacksImpl.cpp
patching file Source/WebKit/chromium/src/WebIDBCallbacksImpl.h

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', u'--reviewer', u'Jeremy Orlow', u'--for..." exit_code: 1

Full output: http://queues.webkit.org/results/7905790
Comment 23 Jeremy Orlow 2011-02-14 18:17:29 PST
Comment on attachment 82378 [details]
Patch

Crap.  The patch I just landed will mess this up.  Sorry.
Comment 24 David Grogan 2011-02-16 14:39:05 PST
Created attachment 82700 [details]
Patch
Comment 25 Jeremy Orlow 2011-02-16 16:45:12 PST
Comment on attachment 82700 [details]
Patch

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

> Source/WebCore/dom/EventTarget.cpp:192
> +IDBVersionChangeRequest* EventTarget::toIDBVersionChangeRequest()

alpha order