Bug 53728

Summary:

indexeddb: make setVersion fire blocked event if other connections are open

Product:

WebKit

Reporter:

David Grogan <dgrogan>

Component:

New Bugs

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, jorlow

Priority:

Version:

528+ (Nightly build)

Hardware:

Other

OS:

OS X 10.5

Attachments:

Description	Flags
Patch	none
obsoleted Patch 1	none
obsoleted patch 2	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	jorlow: review+

David Grogan

Reported 2011-02-03 15:59:33 PST

indexeddb: make setVersion fire blocked event if other connections are open

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
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

David Grogan

Comment 1 2011-02-03 16:06:24 PST

Created attachment 81132 [details] Patch

David Grogan

Comment 2 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.

Jeremy Orlow

Comment 3 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

David Grogan

Comment 4 2011-02-07 19:00:58 PST

Created attachment 81565 [details] obsoleted Patch 1

David Grogan

Comment 5 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.

Jeremy Orlow

Comment 6 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

David Grogan

Comment 7 2011-02-09 13:26:03 PST

Created attachment 81862 [details] obsoleted patch 2

David Grogan

Comment 8 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

Jeremy Orlow

Comment 9 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.

Jeremy Orlow

Comment 10 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?

David Grogan

Comment 11 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

David Grogan

Comment 12 2011-02-11 15:36:22 PST

Created attachment 82197 [details] Patch

Jeremy Orlow

Comment 13 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

David Grogan

Comment 14 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

David Grogan

Comment 15 2011-02-11 16:52:55 PST

Created attachment 82209 [details] Patch

David Grogan

Comment 16 2011-02-14 13:52:55 PST

Created attachment 82358 [details] Patch

David Grogan

Comment 17 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

Jeremy Orlow

Comment 18 2011-02-14 13:58:57 PST

Comment on attachment 82358 [details] Patch rubber stamp assuming nothing else changed

Jeremy Orlow

Comment 19 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

David Grogan

Comment 20 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

David Grogan

Comment 21 2011-02-14 16:07:59 PST

Created attachment 82378 [details] Patch

WebKit Commit Bot

Comment 22 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

Jeremy Orlow

Comment 23 2011-02-14 18:17:29 PST

Comment on attachment 82378 [details] Patch Crap. The patch I just landed will mess this up. Sorry.

David Grogan

Comment 24 2011-02-16 14:39:05 PST

Created attachment 82700 [details] Patch

Jeremy Orlow

Comment 25 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

Note You need to log in before you can comment on or make changes to this bug.