Last Comment Bug 687361 - Implement IndexedDB setVersion API changes
: Implement IndexedDB setVersion API changes
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete
Product: Core
Classification: Components
Component: DOM: IndexedDB (show other bugs)
: unspecified
: All All
: P1 normal (vote)
: mozilla10
Assigned To: Kyle Huey [:khuey] (khuey@mozilla.com)
:
Mentors:
: 687360 (view as bug list)
Depends on: 696194 696362 697247
Blocks: idb
  Show dependency treegraph
 
Reported: 2011-09-18 11:38 PDT by Kyle Huey [:khuey] (khuey@mozilla.com)
Modified: 2013-01-03 08:25 PST (History)
4 users (show)
khuey: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (133.39 KB, patch)
2011-09-21 08:16 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Patch (131.13 KB, patch)
2011-09-22 08:22 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Rename IDBVersionChangeRequest to IDBOpenDBRequest (23.31 KB, patch)
2011-09-24 08:00 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Test fixes (10.39 KB, patch)
2011-09-27 06:17 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Take Two (172.15 KB, patch)
2011-09-29 17:22 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Patch (173.73 KB, patch)
2011-09-30 07:18 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Patch (171.20 KB, patch)
2011-10-06 10:35 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Patch (173.92 KB, patch)
2011-10-07 11:06 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Patch (213.75 KB, patch)
2011-10-08 14:36 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
bent.mozilla: review+
Details | Diff | Splinter Review
Fixes (23.22 KB, patch)
2011-10-19 08:23 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
no flags Details | Diff | Splinter Review
Final Patch (218.59 KB, patch)
2011-10-20 09:26 PDT, Kyle Huey [:khuey] (khuey@mozilla.com)
khuey: review+
Details | Diff | Splinter Review

Description Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-18 11:38:37 PDT

    
Comment 1 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-21 08:16:01 PDT
Created attachment 561473 [details] [diff] [review]
Patch

This implements the setVersion API changes.  The general idea is that when the OpenDatabaseHelper goes back to the main thread, it sees if a VERSION_CHANGE transaction is needed and if so 1) fires off a SetVersionHelper and 2) avoids firing any events itself.

Modifying the tests was pretty straightforward, there were several that relied on db.setVersion(foo) to close db, which no longer occurs, of course.

I tested the upgrade path manually, but writing an automated tests for it is absurdly painful (dropping stuff in the profile directory is hard :-/).
Comment 2 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-22 08:22:15 PDT
Created attachment 561743 [details] [diff] [review]
Patch

A version that actually compiles on gcc.
Comment 3 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-24 08:00:55 PDT
Created attachment 562239 [details] [diff] [review]
Rename IDBVersionChangeRequest to IDBOpenDBRequest
Comment 4 Jonas Sicking (:sicking) PTO Until July 5th 2011-09-26 14:28:49 PDT
Comment on attachment 561743 [details] [diff] [review]
Patch

Review of attachment 561743 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/indexedDB/IDBEvents.cpp
@@ +154,2 @@
>  {
> +  NS_ENSURE_ARG_POINTER(aOldVersion);

Remove this. It just clutters the code and risks that someone will actually call this function with a null pointer. XPConnect will never pass in a null pointer here, and neither should C++ callers.

Same in GetNewVersion.

::: dom/indexedDB/IDBEvents.h
@@ +84,4 @@
>    }
>  
>    inline static already_AddRefed<nsIDOMEvent>
> +  CreateBlocked(PRInt64 aOldVersion,

Don't you need a CreateUpgradeNeeded here too? Same for the runnable.
Comment 5 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-27 06:17:10 PDT
Created attachment 562743 [details] [diff] [review]
Test fixes

IndexedDB has browser chrome tests too ... who knew?

Jonas, please redirect all these review requests to bent if appropriate.
Comment 6 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-27 09:01:29 PDT
(In reply to Jonas Sicking (:sicking) from comment #4)
> Comment on attachment 561743 [details] [diff] [review] [diff] [details] [review]
> ::: dom/indexedDB/IDBEvents.h
> @@ +84,4 @@
> >    }
> >  
> >    inline static already_AddRefed<nsIDOMEvent>
> > +  CreateBlocked(PRInt64 aOldVersion,
> 
> Don't you need a CreateUpgradeNeeded here too? Same for the runnable.

These patches don't implement upgradeneeded.  I was planning on doing that separately (we have a *lot* of updating to do).
Comment 7 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-27 15:14:33 PDT
Comment on attachment 561743 [details] [diff] [review]
Patch

/me grumbles about misreading the spec
Comment 8 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-29 17:22:55 PDT
Created attachment 563604 [details] [diff] [review]
Take Two
Comment 9 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-30 06:38:17 PDT
Things that aren't implemented here:

- Refusing to open a DB with version 0
- Optional version syntax for opening a DB
- DeleteDatabase
- Firing a VersionChange event with oldVersion set to the string on upgrade from old schema DBs
Comment 10 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-30 06:38:40 PDT
*** Bug 687360 has been marked as a duplicate of this bug. ***
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-09-30 07:18:08 PDT
Created attachment 563734 [details] [diff] [review]
Patch

Now with unsigned long long instead of long long.
Comment 12 Jonas Sicking (:sicking) PTO Until July 5th 2011-09-30 08:23:15 PDT
So actually, my understanding of the xpconnect code is that we can't use unsigned long long as type for the second argument of indexedDB.open

The problem is that if the user passes -1, xpconnect will wrap that to PR_UINT64_MAX before calling the C++ function. So at that point it's too late to do range checks.
Comment 13 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-10-04 16:03:35 PDT
Comment on attachment 563734 [details] [diff] [review]
Patch

Review of attachment 563734 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMError.h
@@ +104,5 @@
> +#define NS_ERROR_DOM_INDEXEDDB_QUOTA_ERR         NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,11)
> +#define NS_ERROR_DOM_INDEXEDDB_VERSION_ERR       NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,12)
> +
> +#define NS_ERROR_DOM_INDEXEDDB_RECOVERABLE_ERR   NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,1001)
> +#define NS_ERROR_DOM_INDEXEDDB_DEADLOCK_ERR   NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,1002)

Can we just remove these? They're unused.

::: dom/indexedDB/AsyncConnectionHelper.cpp
@@ +181,5 @@
>  
>  NS_IMETHODIMP
>  AsyncConnectionHelper::Run()
>  {
> +  nsresult rv;

Hm, can you revert these changes? I like the old way better.

@@ +386,5 @@
> +{
> +  // This is maybe not the most elegant place to do this, but the helper that
> +  // doesn't want to fire a generic event is also the one that doesn't want to
> +  // set the request as done.
> +  mRequest->SetDone();

No, let's not do this. It's totally unintuitive. We should chat about what you're trying to do here and figure out another way.

@@ +407,5 @@
>    bool dummy;
>    nsresult rv = mRequest->DispatchEvent(event, &dummy);
>    NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
>  
> +  nsCOMPtr<nsIPrivateDOMEvent> privateEvent = do_QueryInterface(event);

This is gross too, let's keep returning a raw nsDOMEvent so we don't have to QI (and hit the CC hash table) here.

::: dom/indexedDB/AsyncConnectionHelper.h
@@ +133,5 @@
> +   * OnSuccess is called.  A subclass can override this to fire an event other
> +   * than "success" at the request.  Arbitrary pre-success processing can be
> +   * done here too.
> +   */
> +  virtual already_AddRefed<nsIDOMEvent> CreateSuccessEvent();

We already have a place to do pre-success processing (OnSuccess) so please don't mention that here. Also, change type to nsDOMEvent.

::: dom/indexedDB/IDBFactory.cpp
@@ +94,3 @@
>  class OpenDatabaseHelper : public AsyncConnectionHelper
>  {
> +  friend class SetVersionHelper;

Rather than make a friend class just expose a new public method.

@@ +107,5 @@
>  
>    nsresult DoDatabaseWork(mozIStorageConnection* aConnection);
>    nsresult GetSuccessResult(JSContext* aCx,
>                              jsval* aVal);
> +  NS_IMETHOD Run();

Woah... What?

@@ +123,1 @@
>    PRUint32 mDataVersion;

Hm, please add an initializer for the new member, and for mDataVersion (yikes)

@@ +450,5 @@
>  
>    if (schemaVersion != DB_SCHEMA_VERSION) {
> +    // This logic needs to change next time we change the schema!
> +    PR_STATIC_ASSERT(DB_SCHEMA_VERSION == 5);
> +    if (schemaVersion == 4) {

Can you move this into a separate function? Nice to keep the upgrade logic separate from the open logic.

@@ +518,5 @@
> +      NS_ENSURE_SUCCESS(rv, rv);
> +
> +      rv = connection->SetSchemaVersion(DB_SCHEMA_VERSION);
> +      NS_ENSURE_SUCCESS(rv, rv);
> +    } else {

Nit: No cuddling here.

@@ +788,5 @@
>    }
>  
> +  PRInt64 version;
> +  rv = stmt->GetInt64(0, &version);
> +  *aVersion = version;

Assert that version >= 0 before you do this.

@@ +1022,5 @@
> +
> +  if (mCurrentVersion > mRequestedVersion)
> +    return NS_ERROR_DOM_INDEXEDDB_VERSION_ERR;
> +
> +  mNeedsSetVersion = true;

How about this:

  if (mCurrentVersion > mRequestedVersion)
    return NS_ERROR_DOM_INDEXEDDB_VERSION_ERR;

  mNeedsSetVersion = mCurrentVersion != mRequestedVersion;
  return NS_OK;

@@ +1030,5 @@
> +NS_IMETHODIMP
> +OpenDatabaseHelper::Run()
> +{
> +  // If we need to queue up a SetVersionHelper, do that here.
> +  if (NS_IsMainThread() && mNeedsSetVersion) {

I don't think you want to do this if there was an error on the DB thread, right?  You should probably override AsyncConnectionHelper::OnSuccess() rather than mess with nsIRunnable::Run().

@@ +1032,5 @@
> +{
> +  // If we need to queue up a SetVersionHelper, do that here.
> +  if (NS_IsMainThread() && mNeedsSetVersion) {
> +    nsresult rv = LoadDatabase();
> +    NS_ENSURE_SUCCESS(rv, rv);

Returning a failure code here does nothing, you need to call the OnError handler. I think you had best not override this function.

@@ +1034,5 @@
> +  if (NS_IsMainThread() && mNeedsSetVersion) {
> +    nsresult rv = LoadDatabase();
> +    NS_ENSURE_SUCCESS(rv, rv);
> +
> +    NS_ASSERTION(mNeedsSetVersion, "Why are we here?");

That was checked just above... Remove?

@@ +1050,5 @@
> +
> +    IndexedDatabaseManager* mgr = IndexedDatabaseManager::Get();
> +    NS_ASSERTION(mgr, "This should never be null!");
> +
> +    IDBOpenDBRequest* request = static_cast<IDBOpenDBRequest*>(mRequest.get());

I'm not a fan of static casting like this... These objects are cheap, just make a separate member to hold the right type (can be a raw pointer, not a nsRefPtr).

@@ +1068,2 @@
>  nsresult
> +OpenDatabaseHelper::LoadDatabase()

Hm... I want a better name for this (load made me think "runs on DB thread"). How about "EnsureSuccessResult"?

@@ +1215,5 @@
> +
> +  // Step 9
> +  NS_ASSERTION(mTransaction, "Better have a transaction!");
> +  // 9.2
> +  static_cast<IDBOpenDBRequest*>(mRequest.get())->SetTransaction(mTransaction);

Let's avoid this cast too.

@@ +1229,5 @@
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> +
> +  PRUint64 oldVersion = mOpenHelper->mCurrentVersion;
> +  PRUint64 newVersion = mOpenHelper->mRequestedVersion;
> +  NS_ASSERTION(oldVersion < newVersion, "Huh?");

Nit: Lose the temporaries.

@@ +1239,5 @@
> +// and redispatches the OpenDatabaseHelper to finish the steps for opening
> +// the database.
> +nsresult
> +SetVersionHelper::NotifyTransactionComplete(IDBTransaction* aTransaction,
> +                                            eCompletionState aState)

'aState' is unused. Remove it?

@@ +1251,5 @@
> +  mOpenRequest->Reset();
> +
> +  // Dispatch the OpenDatabaseHelper back to the main thread
> +  // XXXkhuey should we run it synchronously right here?
> +  NS_DispatchToMainThread(mOpenHelper);

NS_DispatchToCurrentThread. And check the return value.

::: dom/indexedDB/IDBRequest.h
@@ +82,5 @@
>  
>    void Reset();
>  
> +  nsresult NotifyHelperCompleted(AsyncConnectionHelper* aHelper);
> +  void SetDone();

I'm unclear on why you separated this. How does this help?

@@ +114,5 @@
>  
>    PRUint16 mErrorCode;
> +  bool mResultValRooted : 1;
> +  bool mHaveResultOrErrorCode : 1;
> +  bool mDone : 1;

I don't think this is important, please revert :)

@@ +133,3 @@
>    Create(nsISupports* aSource,
>           nsIScriptContext* aScriptContext,
> +         nsPIDOMWindow* aOwner);

Hm, 'aSource' is always going to be null, right? Let's lose the useless parameter.

@@ +142,5 @@
>  protected:
> +  nsRefPtr<nsDOMEventListenerWrapper> mOnblockedListener;
> +  nsRefPtr<nsDOMEventListenerWrapper> mOnupgradeneededListener;
> +
> +  ~IDBOpenDBRequest();

Nit: Move this above the members.

::: dom/indexedDB/IDBTransaction.cpp
@@ +979,5 @@
> +    // Tell the listener (if we have one) that we're done
> +    if (mListener)
> +      mListener->NotifyTransactionComplete(mTransaction,
> +        mAborted ? IDBTransactionListener::ROLLED_BACK :
> +                   IDBTransactionListener::COMMITTED);

Nit: Please add braces.

::: dom/indexedDB/IDBTransaction.h
@@ +68,5 @@
> +#define IDBTRANSACTIONLISTENER_IID                     \
> +  { 0x9585a438, 0x71b2, 0x4820,                        \
> +    {0xb7, 0xed, 0xeb, 0xed, 0x91, 0x97, 0xa3, 0x35} }
> +
> +class IDBTransactionListener : public nsISupports

Can you nuke the nsISupports/IID goop and just add AddRef/Release methods?

@@ +114,5 @@
>  
>    void OnNewRequest();
>    void OnRequestFinished();
>  
> +  void RegisterListener(IDBTransactionListener* aListener);

'Register' usually goes along with multiple listeners. Since we only have one let's do 'SetTransactionListener'.

@@ +235,5 @@
>    NS_DECL_ISUPPORTS
>    NS_DECL_NSIRUNNABLE
>  
> +  CommitHelper(IDBTransaction* aTransaction,
> +               already_AddRefed<IDBTransactionListener> aListener);

I think this is a little dangerous... You can pass a raw pointer here by accident and introduce an early free. Just stick with the normal pattern and eat the extra addref/release.

::: dom/indexedDB/nsIIDBDatabase.idl
@@ +55,5 @@
>  interface nsIIDBDatabase : nsISupports
>  {
>    readonly attribute DOMString name;
>  
> +  readonly attribute unsigned long long version;

As Jonas said, I think we have to leave this signed so that we can distinguish negative numbers from very large positive numbers. Lame :(

::: dom/indexedDB/nsIIDBFactory.idl
@@ +53,5 @@
>  interface nsIIDBFactory : nsISupports
>  {
>    [implicit_jscontext]
> +  nsIIDBOpenDBRequest
> +  open(in AString name, in unsigned long long version);

Signed here too.

::: dom/indexedDB/nsIIDBVersionChangeEvent.idl
@@ +38,5 @@
>   * ***** END LICENSE BLOCK ***** */
>  
>  #include "nsIDOMEvent.idl"
>  
>  [scriptable, uuid(6a232c30-1bc4-4d5b-9ce0-6e7c08934755)]

This uuid needs a bump.

::: dom/indexedDB/test/test_add_twice_failure.html
@@ +14,5 @@
>      {
>        const name = window.location.pathname;
>        const description = "My Test Database";
>  
> +      let request = mozIndexedDB.open(name, 1, description);

Ooh... Since you're here, can you you remove all the 'description' args in these files? They disappeared a long time ago.

::: dom/indexedDB/test/test_setVersion.html
@@ +23,3 @@
>    request.onerror = errorHandler;
>    request.onsuccess = grabEventAndContinueHandler;
>    let event = yield;

Wait... How does this work? Shouldn't that fire an upgradeneeded event, not success?

::: js/src/xpconnect/src/qsgen.py
@@ +661,5 @@
>      'long':
>          "    return xpc_qsInt32ToJsval(cx, result, ${jsvalPtr});\n",
>  
>      'long long':
> +        "    return xpc_qsInt64ToJsval(cx, result, ${jsvalPtr});\n",

Ha!
Comment 14 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-06 10:35:13 PDT
(In reply to ben turner [:bent] from comment #13)
> ::: dom/base/nsDOMError.h
> @@ +104,5 @@
> > +#define NS_ERROR_DOM_INDEXEDDB_QUOTA_ERR         NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,11)
> > +#define NS_ERROR_DOM_INDEXEDDB_VERSION_ERR       NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,12)
> > +
> > +#define NS_ERROR_DOM_INDEXEDDB_RECOVERABLE_ERR   NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,1001)
> > +#define NS_ERROR_DOM_INDEXEDDB_DEADLOCK_ERR   NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,1002)
> 
> Can we just remove these? They're unused.

No, they're not, unfortunately.

> ::: dom/indexedDB/AsyncConnectionHelper.cpp
> @@ +181,5 @@
> >  
> >  NS_IMETHODIMP
> >  AsyncConnectionHelper::Run()
> >  {
> > +  nsresult rv;
> 
> Hm, can you revert these changes? I like the old way better.

grumble grumble

> @@ +386,5 @@
> > +{
> > +  // This is maybe not the most elegant place to do this, but the helper that
> > +  // doesn't want to fire a generic event is also the one that doesn't want to
> > +  // set the request as done.
> > +  mRequest->SetDone();
> 
> No, let's not do this. It's totally unintuitive. We should chat about what
> you're trying to do here and figure out another way.

This is unnecessary anyways.

> @@ +407,5 @@
> >    bool dummy;
> >    nsresult rv = mRequest->DispatchEvent(event, &dummy);
> >    NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> >  
> > +  nsCOMPtr<nsIPrivateDOMEvent> privateEvent = do_QueryInterface(event);
> 
> This is gross too, let's keep returning a raw nsDOMEvent so we don't have to
> QI (and hit the CC hash table) here.

Fixed.

> ::: dom/indexedDB/AsyncConnectionHelper.h
> @@ +133,5 @@
> > +   * OnSuccess is called.  A subclass can override this to fire an event other
> > +   * than "success" at the request.  Arbitrary pre-success processing can be
> > +   * done here too.
> > +   */
> > +  virtual already_AddRefed<nsIDOMEvent> CreateSuccessEvent();
> 
> We already have a place to do pre-success processing (OnSuccess) so please
> don't mention that here. Also, change type to nsDOMEvent.

Fixed.

> ::: dom/indexedDB/IDBFactory.cpp
> @@ +94,3 @@
> >  class OpenDatabaseHelper : public AsyncConnectionHelper
> >  {
> > +  friend class SetVersionHelper;
> 
> Rather than make a friend class just expose a new public method.

Done (well, avoided the need for, really).

> @@ +107,5 @@
> >  
> >    nsresult DoDatabaseWork(mozIStorageConnection* aConnection);
> >    nsresult GetSuccessResult(JSContext* aCx,
> >                              jsval* aVal);
> > +  NS_IMETHOD Run();
> 
> Woah... What?

If we need a SetVersionHelper, we need to avoid doing any of the stuff that we normally do on the main thread until that completes.

> @@ +123,1 @@
> >    PRUint32 mDataVersion;
> 
> Hm, please add an initializer for the new member, and for mDataVersion
> (yikes)

Done.

> @@ +450,5 @@
> >  
> >    if (schemaVersion != DB_SCHEMA_VERSION) {
> > +    // This logic needs to change next time we change the schema!
> > +    PR_STATIC_ASSERT(DB_SCHEMA_VERSION == 5);
> > +    if (schemaVersion == 4) {
> 
> Can you move this into a separate function? Nice to keep the upgrade logic
> separate from the open logic.

Done.

> @@ +518,5 @@
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +      rv = connection->SetSchemaVersion(DB_SCHEMA_VERSION);
> > +      NS_ENSURE_SUCCESS(rv, rv);
> > +    } else {
> 
> Nit: No cuddling here.

grumble.

> @@ +788,5 @@
> >    }
> >  
> > +  PRInt64 version;
> > +  rv = stmt->GetInt64(0, &version);
> > +  *aVersion = version;
> 
> Assert that version >= 0 before you do this.

I don't think it's appropriate to assert that, but I added code that ensures that we always return an aVersion of at least 1.

> @@ +1022,5 @@
> > +
> > +  if (mCurrentVersion > mRequestedVersion)
> > +    return NS_ERROR_DOM_INDEXEDDB_VERSION_ERR;
> > +
> > +  mNeedsSetVersion = true;
> 
> How about this:
> 
>   if (mCurrentVersion > mRequestedVersion)
>     return NS_ERROR_DOM_INDEXEDDB_VERSION_ERR;
> 
>   mNeedsSetVersion = mCurrentVersion != mRequestedVersion;
>   return NS_OK;
> 

Ok.

> @@ +1030,5 @@
> > +NS_IMETHODIMP
> > +OpenDatabaseHelper::Run()
> > +{
> > +  // If we need to queue up a SetVersionHelper, do that here.
> > +  if (NS_IsMainThread() && mNeedsSetVersion) {
> 
> I don't think you want to do this if there was an error on the DB thread,
> right?  You should probably override AsyncConnectionHelper::OnSuccess()
> rather than mess with nsIRunnable::Run().

OnSuccess is too late (because we will have already told the request that we're done).  Nothing that can fail runs on the DB thread in ACH::Run after DoDatabaseWork finishes.

> @@ +1032,5 @@
> > +{
> > +  // If we need to queue up a SetVersionHelper, do that here.
> > +  if (NS_IsMainThread() && mNeedsSetVersion) {
> > +    nsresult rv = LoadDatabase();
> > +    NS_ENSURE_SUCCESS(rv, rv);
> 
> Returning a failure code here does nothing, you need to call the OnError
> handler. I think you had best not override this function.

The alternative is probably to have some PreOnSuccess hook that gets called before we tell the request that we're done.  We could do that ...

> @@ +1034,5 @@
> > +  if (NS_IsMainThread() && mNeedsSetVersion) {
> > +    nsresult rv = LoadDatabase();
> > +    NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    NS_ASSERTION(mNeedsSetVersion, "Why are we here?");
> 
> That was checked just above... Remove?

Yeah, think that's cruft.

> @@ +1050,5 @@
> > +
> > +    IndexedDatabaseManager* mgr = IndexedDatabaseManager::Get();
> > +    NS_ASSERTION(mgr, "This should never be null!");
> > +
> > +    IDBOpenDBRequest* request = static_cast<IDBOpenDBRequest*>(mRequest.get());
> 
> I'm not a fan of static casting like this... These objects are cheap, just
> make a separate member to hold the right type (can be a raw pointer, not a
> nsRefPtr).

Done.

> @@ +1068,2 @@
> >  nsresult
> > +OpenDatabaseHelper::LoadDatabase()
> 
> Hm... I want a better name for this (load made me think "runs on DB
> thread"). How about "EnsureSuccessResult"?

Done.

> @@ +1215,5 @@
> > +
> > +  // Step 9
> > +  NS_ASSERTION(mTransaction, "Better have a transaction!");
> > +  // 9.2
> > +  static_cast<IDBOpenDBRequest*>(mRequest.get())->SetTransaction(mTransaction);
> 
> Let's avoid this cast too.

Done.

> @@ +1229,5 @@
> > +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> > +
> > +  PRUint64 oldVersion = mOpenHelper->mCurrentVersion;
> > +  PRUint64 newVersion = mOpenHelper->mRequestedVersion;
> > +  NS_ASSERTION(oldVersion < newVersion, "Huh?");
> 
> Nit: Lose the temporaries.

Done.

> @@ +1239,5 @@
> > +// and redispatches the OpenDatabaseHelper to finish the steps for opening
> > +// the database.
> > +nsresult
> > +SetVersionHelper::NotifyTransactionComplete(IDBTransaction* aTransaction,
> > +                                            eCompletionState aState)
> 
> 'aState' is unused. Remove it?

Done.

> @@ +1251,5 @@
> > +  mOpenRequest->Reset();
> > +
> > +  // Dispatch the OpenDatabaseHelper back to the main thread
> > +  // XXXkhuey should we run it synchronously right here?
> > +  NS_DispatchToMainThread(mOpenHelper);
> 
> NS_DispatchToCurrentThread. And check the return value.

Done.

> ::: dom/indexedDB/IDBRequest.h
> @@ +82,5 @@
> >  
> >    void Reset();
> >  
> > +  nsresult NotifyHelperCompleted(AsyncConnectionHelper* aHelper);
> > +  void SetDone();
> 
> I'm unclear on why you separated this. How does this help?

It's gone.

> @@ +114,5 @@
> >  
> >    PRUint16 mErrorCode;
> > +  bool mResultValRooted : 1;
> > +  bool mHaveResultOrErrorCode : 1;
> > +  bool mDone : 1;
> 
> I don't think this is important, please revert :)

Done.

> @@ +133,3 @@
> >    Create(nsISupports* aSource,
> >           nsIScriptContext* aScriptContext,
> > +         nsPIDOMWindow* aOwner);
> 
> Hm, 'aSource' is always going to be null, right? Let's lose the useless
> parameter.

Done.

> @@ +142,5 @@
> >  protected:
> > +  nsRefPtr<nsDOMEventListenerWrapper> mOnblockedListener;
> > +  nsRefPtr<nsDOMEventListenerWrapper> mOnupgradeneededListener;
> > +
> > +  ~IDBOpenDBRequest();
> 
> Nit: Move this above the members.

Done.

> ::: dom/indexedDB/IDBTransaction.cpp
> @@ +979,5 @@
> > +    // Tell the listener (if we have one) that we're done
> > +    if (mListener)
> > +      mListener->NotifyTransactionComplete(mTransaction,
> > +        mAborted ? IDBTransactionListener::ROLLED_BACK :
> > +                   IDBTransactionListener::COMMITTED);
> 
> Nit: Please add braces.

Done.

> ::: dom/indexedDB/IDBTransaction.h
> @@ +68,5 @@
> > +#define IDBTRANSACTIONLISTENER_IID                     \
> > +  { 0x9585a438, 0x71b2, 0x4820,                        \
> > +    {0xb7, 0xed, 0xeb, 0xed, 0x91, 0x97, 0xa3, 0x35} }
> > +
> > +class IDBTransactionListener : public nsISupports
> 
> Can you nuke the nsISupports/IID goop and just add AddRef/Release methods?

Done.

> @@ +114,5 @@
> >  
> >    void OnNewRequest();
> >    void OnRequestFinished();
> >  
> > +  void RegisterListener(IDBTransactionListener* aListener);
> 
> 'Register' usually goes along with multiple listeners. Since we only have
> one let's do 'SetTransactionListener'.

Done.

> @@ +235,5 @@
> >    NS_DECL_ISUPPORTS
> >    NS_DECL_NSIRUNNABLE
> >  
> > +  CommitHelper(IDBTransaction* aTransaction,
> > +               already_AddRefed<IDBTransactionListener> aListener);
> 
> I think this is a little dangerous... You can pass a raw pointer here by
> accident and introduce an early free. Just stick with the normal pattern and
> eat the extra addref/release.

Done.

> ::: dom/indexedDB/nsIIDBDatabase.idl
> @@ +55,5 @@
> >  interface nsIIDBDatabase : nsISupports
> >  {
> >    readonly attribute DOMString name;
> >  
> > +  readonly attribute unsigned long long version;
> 
> As Jonas said, I think we have to leave this signed so that we can
> distinguish negative numbers from very large positive numbers. Lame :(

This only matters on IDBFactory.open, no?

> ::: dom/indexedDB/nsIIDBFactory.idl
> @@ +53,5 @@
> >  interface nsIIDBFactory : nsISupports
> >  {
> >    [implicit_jscontext]
> > +  nsIIDBOpenDBRequest
> > +  open(in AString name, in unsigned long long version);
> 
> Signed here too.

Done.

> ::: dom/indexedDB/nsIIDBVersionChangeEvent.idl
> @@ +38,5 @@
> >   * ***** END LICENSE BLOCK ***** */
> >  
> >  #include "nsIDOMEvent.idl"
> >  
> >  [scriptable, uuid(6a232c30-1bc4-4d5b-9ce0-6e7c08934755)]
> 
> This uuid needs a bump.

Done.

> ::: dom/indexedDB/test/test_add_twice_failure.html
> @@ +14,5 @@
> >      {
> >        const name = window.location.pathname;
> >        const description = "My Test Database";
> >  
> > +      let request = mozIndexedDB.open(name, 1, description);
> 
> Ooh... Since you're here, can you you remove all the 'description' args in
> these files? They disappeared a long time ago.
> 
> ::: dom/indexedDB/test/test_setVersion.html
> @@ +23,3 @@
> >    request.onerror = errorHandler;
> >    request.onsuccess = grabEventAndContinueHandler;
> >    let event = yield;
> 
> Wait... How does this work? Shouldn't that fire an upgradeneeded event, not
> success?

Sure, it fires upgradeneeded, we don't handle it and nothing happens, and then we get a success event later.

> ::: js/src/xpconnect/src/qsgen.py
> @@ +661,5 @@
> >      'long':
> >          "    return xpc_qsInt32ToJsval(cx, result, ${jsvalPtr});\n",
> >  
> >      'long long':
> > +        "    return xpc_qsInt64ToJsval(cx, result, ${jsvalPtr});\n",
> 
> Ha!

Srsly.
Comment 15 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-06 10:35:59 PDT
Created attachment 565274 [details] [diff] [review]
Patch
Comment 16 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-07 11:06:56 PDT
Created attachment 565589 [details] [diff] [review]
Patch

This handles throwing during the upgradeneeded event per-spec.
Comment 17 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-08 14:36:09 PDT
Created attachment 565757 [details] [diff] [review]
Patch

The only change since the last version is that I rewrote OpenDatabaseHelper to not use AsyncConnectionHelper.
Comment 18 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-08 15:18:29 PDT
Also, I'm done changing this for now.  It's safe to review ;-)
Comment 19 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-10-17 15:07:16 PDT
Comment on attachment 565757 [details] [diff] [review]
Patch

Review of attachment 565757 [details] [diff] [review]:
-----------------------------------------------------------------

I think this looks good. There are a few nits and a few minor things, but otherwise I think we're done here. Awesome!

::: dom/base/nsDOMError.h
@@ +104,5 @@
> +#define NS_ERROR_DOM_INDEXEDDB_QUOTA_ERR         NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,11)
> +#define NS_ERROR_DOM_INDEXEDDB_VERSION_ERR       NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,12)
> +
> +#define NS_ERROR_DOM_INDEXEDDB_RECOVERABLE_ERR   NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,1001)
> +#define NS_ERROR_DOM_INDEXEDDB_DEADLOCK_ERR   NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,1002)

Ok, let's go remove any uses of these and kill these now.

::: dom/indexedDB/AsyncConnectionHelper.cpp
@@ +414,5 @@
>        mTransaction->IsOpen()) {
>      rv = mTransaction->Abort();
>      NS_ENSURE_SUCCESS(rv, rv);
> +
> +    return NS_ERROR_DOM_INDEXEDDB_ABORT_ERR;

I think this is wrong, we shouldn't call OnError if we've already called OnSuccess.

::: dom/indexedDB/AsyncConnectionHelper.h
@@ +60,5 @@
>  class IDBTransaction;
>  
> +// A common base class for AsyncConnectionHelper and OpenDatabaseHelper that
> +// IDBRequest can use.
> +class HelperBase : public nsIRunnable

This is too generic... DatabaseHelperBase? DBHelperBase?

::: dom/indexedDB/IDBFactory.cpp
@@ +383,5 @@
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>  
> +  // XXXkhuey the spec doesn't say what error to throw here ...
> +  if (aVersion < 1)
> +    return NS_ERROR_DOM_INDEXEDDB_VERSION_ERR;

NON_TRANSIENT_ERR for bad arguments in almost all cases.

And please use braces.

@@ +426,5 @@
>      }
>    }
>  
> +  nsRefPtr<IDBOpenDBRequest> request =
> +    IDBOpenDBRequest::Create(context, window);

Nit: Will this fit on one line?

::: dom/indexedDB/IDBRequest.cpp
@@ +201,5 @@
>  {
>    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
>  
> +  *aReadyState = mHaveResultOrErrorCode ? nsIIDBRequest::DONE :
> +                                          nsIIDBRequest::LOADING;

Nit:

  a = b ?
      c :
      d;

::: dom/indexedDB/IndexedDatabase.h
@@ +61,5 @@
>    using namespace mozilla::dom::indexedDB;
>  
>  BEGIN_INDEXEDDB_NAMESPACE
>  
> +#define DB_SCHEMA_VERSION 5

Nit: Move above USING line

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +298,5 @@
> +  return stmt->Execute();
> +}
> +
> +nsresult
> +UpgradeSchemaFrom4To5(mozIStorageConnection* aConnection)

This needs to be part of the same transaction as the other open stuff. Otherwise if something fails in here the db will be in an unstable state.

@@ +333,5 @@
> +  PRInt64 dataVersion;
> +  rv = stmt->GetInt64(2, &dataVersion);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  stmt = nsnull; // DROP TABLE fails unless this statement is destroyed.

You should use a statement scoper here.

@@ +491,5 @@
> +
> +  // Need an upgradeneeded event here.
> +  already_AddRefed<nsDOMEvent> CreateSuccessEvent();
> +
> +  nsresult NotifyTransactionComplete(IDBTransaction* aTransaction);

Nit: add a newline here.

@@ +502,5 @@
> +};
> +
> +} // anonymous namespace
> +
> +NS_IMPL_ISUPPORTS_INHERITED0(SetVersionHelper, AsyncConnectionHelper);

Why? No need for this.

@@ +504,5 @@
> +} // anonymous namespace
> +
> +NS_IMPL_ISUPPORTS_INHERITED0(SetVersionHelper, AsyncConnectionHelper);
> +
> +

Nit: Extra newline here

@@ +506,5 @@
> +NS_IMPL_ISUPPORTS_INHERITED0(SetVersionHelper, AsyncConnectionHelper);
> +
> +
> +nsresult
> +SetVersionHelper::DoDatabaseWork(mozIStorageConnection* aConnection)

Nit: Please put these helper implementations after the methods of OpenDatabaseHelper (the class for which the file is named).

@@ +508,5 @@
> +
> +nsresult
> +SetVersionHelper::DoDatabaseWork(mozIStorageConnection* aConnection)
> +{
> +  NS_PRECONDITION(aConnection, "Passing a null connection!");

NS_ASSERTION please.

@@ +529,5 @@
> +  return NS_OK;
> +}
> +
> +// This method implements steps 7-9 (but not 9.3!, that happens in 
> +// AsyncConnectionHelper::OnSuccess) of the VERSION_CHANGE transaction steps.

Listing the step numbers here is kind of unhelpful... The spec is still in flux and we're far more likely to forget to change these comments if the numbers change. If you want to describe what it does, well, that would be better. Let's remove the numbers, here and below.

@@ +555,5 @@
> +
> +already_AddRefed<nsDOMEvent>
> +SetVersionHelper::CreateSuccessEvent()
> +{
> +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");

You don't really need to assert this, AsyncConnectionHelper does so for you.

@@ +590,5 @@
> +
> +NS_IMPL_THREADSAFE_ISUPPORTS1(OpenDatabaseHelper, nsIRunnable);
> +
> +nsresult
> +OpenDatabaseHelper::Dispatch(nsIEventTarget* aTarget)

You could inline this, right?

@@ +599,5 @@
> +  return aTarget->Dispatch(this, NS_DISPATCH_NORMAL);
> +}
> +
> +nsresult
> +OpenDatabaseHelper::RunImmediately()

This too.

@@ +640,5 @@
> +  NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> +
> +  bool exists;
> +  rv = dbDirectory->Exists(&exists);
> +  NS_ENSURE_SUCCESS(rv, rv);

Please make sure you only ever return IndexedDB error codes.

@@ +712,5 @@
> +  }
> +
> +  // See if we need to do a VERSION_CHANGE transaction
> +  if (mCurrentVersion > mRequestedVersion)
> +    return NS_ERROR_DOM_INDEXEDDB_VERSION_ERR;

Nit: braces please.

@@ +715,5 @@
> +  if (mCurrentVersion > mRequestedVersion)
> +    return NS_ERROR_DOM_INDEXEDDB_VERSION_ERR;
> +
> +  mState = (mCurrentVersion != mRequestedVersion) ? eSetVersionPending :
> +                                                    eFiringEvents;

Nit:

  mState = mCurrentVersion != mRequestedVersion ?
           eSetVersionPending :
           eFiringEvents;

@@ +765,5 @@
> +    if (mState == eSetVersionPending) {
> +      nsresult rv = StartSetVersion();
> +
> +      if (NS_SUCCEEDED(rv))
> +        return rv;

Nit: Braces

@@ +930,5 @@
> +  return NS_DispatchToCurrentThread(this);
> +}
> +
> +nsresult
> +OpenDatabaseHelper::WrapNative(JSContext* aCx,

Can't you share this in the base class you made?

@@ +951,5 @@
> +  return NS_OK;
> +}
> +
> +void
> +OpenDatabaseHelper::DispatchSuccessEvent()

Ditto

@@ +967,5 @@
> +  mOpenDBRequest->DispatchEvent(event, &dummy);
> +}
> +
> +void
> +OpenDatabaseHelper::DispatchErrorEvent()

Ditto

::: dom/indexedDB/OpenDatabaseHelper.h
@@ +39,5 @@
> +#ifndef mozilla_dom_indexeddb_opendatabasehelper_h__
> +#define mozilla_dom_indexeddb_opendatabasehelper_h__
> +
> +#include "AsyncConnectionHelper.h"
> +#include "IndexedDatabase.h"

Nit: IndexedDatabase is included in the first, remove this.

@@ +62,5 @@
> +      mRequestedVersion(aRequestedVersion), mCurrentVersion(0),
> +      mDataVersion(DB_SCHEMA_VERSION), mDatabaseId(0), mLastObjectStoreId(0),
> +      mLastIndexId(0), mState(eCreated), mResultCode(NS_OK)
> +  {
> +  }

Nit: { }

@@ +122,5 @@
> +    eSetVersionPending, // Waiting on a SetVersionHelper
> +    eSetVersionCompleted, // SetVersionHelper is done
> +  };
> +  OpenDatabaseState mState;
> +  bool mNeededSetVersion;

This isn't initialized as far as I can tell.
Comment 20 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-19 08:21:32 PDT
(In reply to ben turner [:bent] from comment #19)
> Comment on attachment 565757 [details] [diff] [review] [diff] [details] [review]
> Patch
> 
> Review of attachment 565757 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> I think this looks good. There are a few nits and a few minor things, but
> otherwise I think we're done here. Awesome!
> 
> ::: dom/base/nsDOMError.h
> @@ +104,5 @@
> > +#define NS_ERROR_DOM_INDEXEDDB_QUOTA_ERR         NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,11)
> > +#define NS_ERROR_DOM_INDEXEDDB_VERSION_ERR       NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,12)
> > +
> > +#define NS_ERROR_DOM_INDEXEDDB_RECOVERABLE_ERR   NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,1001)
> > +#define NS_ERROR_DOM_INDEXEDDB_DEADLOCK_ERR   NS_ERROR_GENERATE_FAILURE(NS_ERROR_MODULE_DOM_INDEXEDDB,1002)
> 
> Ok, let's go remove any uses of these and kill these now.

Let's do this in a follow up.  It's not immediately clear to me what these should be replaced by.

> ::: dom/indexedDB/AsyncConnectionHelper.cpp
> @@ +414,5 @@
> >        mTransaction->IsOpen()) {
> >      rv = mTransaction->Abort();
> >      NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +    return NS_ERROR_DOM_INDEXEDDB_ABORT_ERR;
> 
> I think this is wrong, we shouldn't call OnError if we've already called
> OnSuccess.

Per http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#dfn-fire-a-success-event if an exception is thrown, we abort the transaction.  Per http://dvcs.w3.org/hg/IndexedDB/raw-file/tip/Overview.html#dfn-steps-for-aborting-a-transaction, an error event is fired at the request.

> ::: dom/indexedDB/AsyncConnectionHelper.h
> @@ +60,5 @@
> >  class IDBTransaction;
> >  
> > +// A common base class for AsyncConnectionHelper and OpenDatabaseHelper that
> > +// IDBRequest can use.
> > +class HelperBase : public nsIRunnable
> 
> This is too generic... DatabaseHelperBase? DBHelperBase?

It's in mozilla::dom::indexedDB, does it really need more namespacing?

> ::: dom/indexedDB/IDBFactory.cpp
> @@ +383,5 @@
> >    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> >  
> > +  // XXXkhuey the spec doesn't say what error to throw here ...
> > +  if (aVersion < 1)
> > +    return NS_ERROR_DOM_INDEXEDDB_VERSION_ERR;
> 
> NON_TRANSIENT_ERR for bad arguments in almost all cases.
> 
> And please use braces.

Done.

> 
> @@ +426,5 @@
> >      }
> >    }
> >  
> > +  nsRefPtr<IDBOpenDBRequest> request =
> > +    IDBOpenDBRequest::Create(context, window);
> 
> Nit: Will this fit on one line?

Not without going beyond 80 columns.

> ::: dom/indexedDB/IDBRequest.cpp
> @@ +201,5 @@
> >  {
> >    NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> >  
> > +  *aReadyState = mHaveResultOrErrorCode ? nsIIDBRequest::DONE :
> > +                                          nsIIDBRequest::LOADING;
> 
> Nit:
> 
>   a = b ?
>       c :
>       d;

Done.

> ::: dom/indexedDB/IndexedDatabase.h
> @@ +61,5 @@
> >    using namespace mozilla::dom::indexedDB;
> >  
> >  BEGIN_INDEXEDDB_NAMESPACE
> >  
> > +#define DB_SCHEMA_VERSION 5
> 
> Nit: Move above USING line

Done.

> ::: dom/indexedDB/OpenDatabaseHelper.cpp
> @@ +298,5 @@
> > +  return stmt->Execute();
> > +}
> > +
> > +nsresult
> > +UpgradeSchemaFrom4To5(mozIStorageConnection* aConnection)
> 
> This needs to be part of the same transaction as the other open stuff.
> Otherwise if something fails in here the db will be in an unstable state.

I don't follow.  Are you saying I need to wrap this all in a mozStorageTransaction?

> @@ +333,5 @@
> > +  PRInt64 dataVersion;
> > +  rv = stmt->GetInt64(2, &dataVersion);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> > +
> > +  stmt = nsnull; // DROP TABLE fails unless this statement is destroyed.
> 
> You should use a statement scoper here.

Ok.

> @@ +491,5 @@
> > +
> > +  // Need an upgradeneeded event here.
> > +  already_AddRefed<nsDOMEvent> CreateSuccessEvent();
> > +
> > +  nsresult NotifyTransactionComplete(IDBTransaction* aTransaction);
> 
> Nit: add a newline here.

Done.

> @@ +502,5 @@
> > +};
> > +
> > +} // anonymous namespace
> > +
> > +NS_IMPL_ISUPPORTS_INHERITED0(SetVersionHelper, AsyncConnectionHelper);
> 
> Why? No need for this.

Unnecessary yes, but it helps see these in leak stats.

> 
> @@ +504,5 @@
> > +} // anonymous namespace
> > +
> > +NS_IMPL_ISUPPORTS_INHERITED0(SetVersionHelper, AsyncConnectionHelper);
> > +
> > +
> 
> Nit: Extra newline here

Fixed.

> @@ +506,5 @@
> > +NS_IMPL_ISUPPORTS_INHERITED0(SetVersionHelper, AsyncConnectionHelper);
> > +
> > +
> > +nsresult
> > +SetVersionHelper::DoDatabaseWork(mozIStorageConnection* aConnection)
> 
> Nit: Please put these helper implementations after the methods of
> OpenDatabaseHelper (the class for which the file is named).

Generally convention in Mozilla code seems to be putting helper classes at the beginning, but ok.

> @@ +508,5 @@
> > +
> > +nsresult
> > +SetVersionHelper::DoDatabaseWork(mozIStorageConnection* aConnection)
> > +{
> > +  NS_PRECONDITION(aConnection, "Passing a null connection!");
> 
> NS_ASSERTION please.

Done.

> 
> @@ +529,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +// This method implements steps 7-9 (but not 9.3!, that happens in 
> > +// AsyncConnectionHelper::OnSuccess) of the VERSION_CHANGE transaction steps.
> 
> Listing the step numbers here is kind of unhelpful... The spec is still in
> flux and we're far more likely to forget to change these comments if the
> numbers change. If you want to describe what it does, well, that would be
> better. Let's remove the numbers, here and below.

Done.

> @@ +555,5 @@
> > +
> > +already_AddRefed<nsDOMEvent>
> > +SetVersionHelper::CreateSuccessEvent()
> > +{
> > +  NS_ASSERTION(NS_IsMainThread(), "Wrong thread!");
> 
> You don't really need to assert this, AsyncConnectionHelper does so for you.

Done.

> 
> @@ +590,5 @@
> > +
> > +NS_IMPL_THREADSAFE_ISUPPORTS1(OpenDatabaseHelper, nsIRunnable);
> > +
> > +nsresult
> > +OpenDatabaseHelper::Dispatch(nsIEventTarget* aTarget)
> 
> You could inline this, right?
> 
> @@ +599,5 @@
> > +  return aTarget->Dispatch(this, NS_DISPATCH_NORMAL);
> > +}
> > +
> > +nsresult
> > +OpenDatabaseHelper::RunImmediately()
> 
> This too.
>
> @@ +640,5 @@
> > +  NS_ENSURE_SUCCESS(rv, NS_ERROR_DOM_INDEXEDDB_UNKNOWN_ERR);
> > +
> > +  bool exists;
> > +  rv = dbDirectory->Exists(&exists);
> > +  NS_ENSURE_SUCCESS(rv, rv);
> 
> Please make sure you only ever return IndexedDB error codes.

Fixed.

> @@ +712,5 @@
> > +  }
> > +
> > +  // See if we need to do a VERSION_CHANGE transaction
> > +  if (mCurrentVersion > mRequestedVersion)
> > +    return NS_ERROR_DOM_INDEXEDDB_VERSION_ERR;
> 
> Nit: braces please.

Done.

> @@ +715,5 @@
> > +  if (mCurrentVersion > mRequestedVersion)
> > +    return NS_ERROR_DOM_INDEXEDDB_VERSION_ERR;
> > +
> > +  mState = (mCurrentVersion != mRequestedVersion) ? eSetVersionPending :
> > +                                                    eFiringEvents;
> 
> Nit:
> 
>   mState = mCurrentVersion != mRequestedVersion ?
>            eSetVersionPending :
>            eFiringEvents;

Done.

> @@ +765,5 @@
> > +    if (mState == eSetVersionPending) {
> > +      nsresult rv = StartSetVersion();
> > +
> > +      if (NS_SUCCEEDED(rv))
> > +        return rv;
> 
> Nit: Braces

Done.

> @@ +930,5 @@
> > +  return NS_DispatchToCurrentThread(this);
> > +}
> > +
> > +nsresult
> > +OpenDatabaseHelper::WrapNative(JSContext* aCx,
> 
> Can't you share this in the base class you made?

Yes.

> @@ +951,5 @@
> > +  return NS_OK;
> > +}
> > +
> > +void
> > +OpenDatabaseHelper::DispatchSuccessEvent()
> 
> Ditto
>
> @@ +967,5 @@
> > +  mOpenDBRequest->DispatchEvent(event, &dummy);
> > +}
> > +
> > +void
> > +OpenDatabaseHelper::DispatchErrorEvent()
> 
> Ditto

The event stuff is kind of tied up with everything else, and I'd rather not reuse it.

> ::: dom/indexedDB/OpenDatabaseHelper.h
> @@ +39,5 @@
> > +#ifndef mozilla_dom_indexeddb_opendatabasehelper_h__
> > +#define mozilla_dom_indexeddb_opendatabasehelper_h__
> > +
> > +#include "AsyncConnectionHelper.h"
> > +#include "IndexedDatabase.h"
> 
> Nit: IndexedDatabase is included in the first, remove this.

Done.

> @@ +62,5 @@
> > +      mRequestedVersion(aRequestedVersion), mCurrentVersion(0),
> > +      mDataVersion(DB_SCHEMA_VERSION), mDatabaseId(0), mLastObjectStoreId(0),
> > +      mLastIndexId(0), mState(eCreated), mResultCode(NS_OK)
> > +  {
> > +  }
> 
> Nit: { }

Done.

> @@ +122,5 @@
> > +    eSetVersionPending, // Waiting on a SetVersionHelper
> > +    eSetVersionCompleted, // SetVersionHelper is done
> > +  };
> > +  OpenDatabaseState mState;
> > +  bool mNeededSetVersion;
> 
> This isn't initialized as far as I can tell.

It's actually not needed at all now.  Removed.
Comment 21 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-19 08:23:54 PDT
Created attachment 568066 [details] [diff] [review]
Fixes

Was this what you had in mind (particularly around the UpdateSchemaFrom4To5 bits)?
Comment 22 Ben Turner (not reading bugmail, use the needinfo flag!) 2011-10-19 15:24:12 PDT
Comment on attachment 568066 [details] [diff] [review]
Fixes

Review of attachment 568066 [details] [diff] [review]:
-----------------------------------------------------------------

The statement scopers look fine, but the SQL transaction that you create needs to be the same as the IDB setVersion transaction... If the setVersion transaction is aborted, for instance, then the schema should go back to 4. Make sense?

::: dom/indexedDB/AsyncConnectionHelper.h
@@ +70,5 @@
>    virtual nsresult GetSuccessResult(JSContext* aCx,
>                                      jsval* aVal) = 0;
> +
> +protected:
> +  HelperBase(IDBRequest* aRequest) : mRequest(aRequest) {}

Multiline this please.

@@ +81,5 @@
> +                      nsISupports* aNative,
> +                      jsval* aResult);
> +
> +  
> +

Extra newline.

::: dom/indexedDB/OpenDatabaseHelper.cpp
@@ +892,5 @@
>    bool dummy;
>    mOpenDBRequest->DispatchEvent(event, &dummy);
>  }
>  
> +NS_IMPL_ISUPPORTS_INHERITED0(SetVersionHelper, AsyncConnectionHelper);

Yeah, we don't do this anywhere else and it's never been a problem. Let's yank it.
Comment 23 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-20 07:11:19 PDT
(In reply to ben turner [:bent] from comment #22)
> Comment on attachment 568066 [details] [diff] [review] [diff] [details] [review]
> Fixes
> 
> Review of attachment 568066 [details] [diff] [review] [diff] [details] [review]:
> -----------------------------------------------------------------
> 
> The statement scopers look fine, but the SQL transaction that you create
> needs to be the same as the IDB setVersion transaction... If the setVersion
> transaction is aborted, for instance, then the schema should go back to 4.
> Make sense?

We decided on IRC yesterday that this is not the case, and it's ok to allow the schema upgrade to happen even if the setVersion transaction is cancelled.

> ::: dom/indexedDB/AsyncConnectionHelper.h
> @@ +70,5 @@
> >    virtual nsresult GetSuccessResult(JSContext* aCx,
> >                                      jsval* aVal) = 0;
> > +
> > +protected:
> > +  HelperBase(IDBRequest* aRequest) : mRequest(aRequest) {}
> 
> Multiline this please.
> 
> @@ +81,5 @@
> > +                      nsISupports* aNative,
> > +                      jsval* aResult);
> > +
> > +  
> > +
> 
> Extra newline.
> 
> ::: dom/indexedDB/OpenDatabaseHelper.cpp
> @@ +892,5 @@
> >    bool dummy;
> >    mOpenDBRequest->DispatchEvent(event, &dummy);
> >  }
> >  
> > +NS_IMPL_ISUPPORTS_INHERITED0(SetVersionHelper, AsyncConnectionHelper);
> 
> Yeah, we don't do this anywhere else and it's never been a problem. Let's
> yank it.

Done.
Comment 24 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-20 09:26:55 PDT
Created attachment 568416 [details] [diff] [review]
Final Patch
Comment 25 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-10-20 09:30:45 PDT
https://hg.mozilla.org/mozilla-central/rev/6cd262091470
Comment 26 Jean-Yves Perrier [:teoli] 2011-12-05 01:24:10 PST
Ok, this one was somewhat complex to handle for the documentation as info was scattered in several articles.

So what I did is the following:
I deprecated the article for the interface IDBVersionChangeRequest: https://developer.mozilla.org/en/IndexedDB/IDBVersionChangeRequest
I created the article for the new interface IDBOpenDBRequest: https://developer.mozilla.org/en/IndexedDB/IDBOpenDBRequest
I updated IDBDatabase: https://developer.mozilla.org/en/IndexedDB/IDBDatabase where I had to significantly rewrite the basic steps and the example.
I updated IndexedDB w/ the new IDBOpenDBRequest interface: https://developer.mozilla.org/en/IndexedDB
I updated IDBVersionChangeEvent w/ the two new version attribute (and marked the previous one as removed): https://developer.mozilla.org/en/IndexedDB/IDBVersionChangeEvent

I have also updated the more generic articles:
https://developer.mozilla.org/en/IndexedDB/Basic_Concepts_Behind_IndexedDB and
https://developer.mozilla.org/en/IndexedDB/Using_IndexedDB

and, of course, Fx 10 for developers:
https://developer.mozilla.org/en/Firefox_10_for_developers#section_8


I have one question though: what does happen if we try to open a database with a version _lower_ than the one on the disk (or opened in another tab). I didn't find anything in the spec and the code and if I try it, it just seems to me that nothing happened no exception and no callback called. Is this correct and/or expected?
Comment 27 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-12-05 07:16:50 PST
(In reply to Jean-Yves Perrier [:teoli] from comment #26)
> I have one question though: what does happen if we try to open a database
> with a version _lower_ than the one on the disk (or opened in another tab).
> I didn't find anything in the spec and the code and if I try it, it just
> seems to me that nothing happened no exception and no callback called. Is
> this correct and/or expected?

You should get an error event with a VERSION_ERR code.
Comment 28 Jean-Yves Perrier [:teoli] 2011-12-06 00:28:14 PST
Thank you. It works (VER_ERR == 12, btw).

I've added that last bit of information on the following articles:
https://developer.mozilla.org/en/IndexedDB/IDBOpenDBRequest
https://developer.mozilla.org/en/IndexedDB/IDBRequest
https://developer.mozilla.org/en/IndexedDB/IDBDatabaseException
https://developer.mozilla.org/en/IndexedDB/Using_IndexedDB

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