892497 - [contacts] the upgrade path is error-prone

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Julien Wajsberg [:julienw]]

When investigating bug 887701 with Alexandre Lissy, we saw that the Contacts DB has the same problem than the SMS DB: thanks to the asynchronous awesomeness of IndexedDB, we're running all upgrade paths concurrently.

Hopefully nothing has bitten us yet (after all, we don't do migration on users' device yet), but this will.

What we must do is having a recursive approach rather than an iterative approach, repetitively calling the upgrade function with currVersion when "this.result" is null in the cursor success handler.

This needs to be leo+, otherwise we'll have migration hazard.

Comment 1

[Julien Wajsberg [:julienw]]

for reference, see https://bugzilla.mozilla.org/attachment.cgi?id=774003 for a proposal on how to fix this in the Sms DB

Comment 2

[Reuben Morais [:reuben]]

How are those operations running concurrently if they're created in the same versionchange transaction? They will be queued and ran in order, right? If that's not the case, all my assumptions of how IDB works are wrong :) Bent, can you confirm if this is how it works?

And if this is the case, I don't see how changing the upgrade code to be recursive makes any difference. Julien, can you clarify exactly what the bug is here?

(In reply to Julien Wajsberg [:julienw] from comment #0)
> Hopefully nothing has bitten us yet (after all, we don't do migration on
> users' device yet), but this will.

Our upgrade code has bitten us already in Contacts, with the Geeksphone devices.

Comment 3

[Julien Wajsberg [:julienw]]

(In reply to Reuben Morais [:reuben] from comment #2)
> How are those operations running concurrently if they're created in the same
> versionchange transaction? They will be queued and ran in order, right? If
> that's not the case, all my assumptions of how IDB works are wrong :) Bent,
> can you confirm if this is how it works?

I think they're queued in order they're asked for. But the problem is that you'll have multiple cursors running in parallel, and you don't know which callback will be called when.

We actually discovered this in bug 887701. :)

> 
> And if this is the case, I don't see how changing the upgrade code to be
> recursive makes any difference. Julien, can you clarify exactly what the bug
> is here?

I meant that we should execute completely one upgrade before doing the next one. That's what the patch to bug 887701 does for the sms DB: when "this.result" is null in the cursor handler, we call the "next" callback which executes the next upgrade path. Therefore everything is linear again. 

Not sure if I'm clear, but the patch in https://bug887701.bugzilla.mozilla.org/attachment.cgi?id=774003 should make it clearer.

Comment 4

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 5

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 6

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 7

[Reuben Morais [:reuben]]

Comment on attachment 774405 [details] [diff] [review]
patch

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

The error message changes are unrelated to this, as far as I can tell? Can you put them in a different patch/bug?

::: dom/contacts/fallback/ContactDB.jsm
@@ +418,5 @@
> +      }
> +    };
> +    next();
> +    while (!done) {
> +      Services.tm.currentThread.processNextEvent(true);

D:

Just make adding the default contacts the last step, and document that it should always be the last one.

@@ +423,3 @@
>      }
>  
> +    this.incrementRevision(aTransaction);

We want to increment the revision on every individual version bump after 11, you have to call it after each step.

Comment 8

[Gregor Wagner [:gwagner]]

(In reply to Reuben Morais [:reuben] from comment #7)
> Comment on attachment 774405 [details] [diff] [review]
> patch
> 
> Review of attachment 774405 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The error message changes are unrelated to this, as far as I can tell? Can
> you put them in a different patch/bug?

It's very related to this patch. If the upgrade code fails we get a good error msg because the first test is increment and without it we just get the useless error: 0 message.

> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +418,5 @@
> > +      }
> > +    };
> > +    next();
> > +    while (!done) {
> > +      Services.tm.currentThread.processNextEvent(true);
> 
> D:
> 
> Just make adding the default contacts the last step, and document that it
> should always be the last one.

No more documentation about what should happen in the upgrade code. We need a foolproof approach to minimize the risk. 

> 
> @@ +423,3 @@
> >      }
> >  
> > +    this.incrementRevision(aTransaction);
> 
> We want to increment the revision on every individual version bump after 11,
> you have to call it after each step.

Why? Once after the upgrade is enough.

Comment 9

[Reuben Morais [:reuben]]

(In reply to Gregor Wagner [:gwagner] from comment #8)
> It's very related to this patch. If the upgrade code fails we get a good
> error msg because the first test is increment and without it we just get the
> useless error: 0 message.

Makes sense.

> > ::: dom/contacts/fallback/ContactDB.jsm
> > @@ +418,5 @@
> > > +      }
> > > +    };
> > > +    next();
> > > +    while (!done) {
> > > +      Services.tm.currentThread.processNextEvent(true);
> > 
> > D:
> > 
> > Just make adding the default contacts the last step, and document that it
> > should always be the last one.
> 
> No more documentation about what should happen in the upgrade code. We need
> a foolproof approach to minimize the risk. 

These changes make a lot of difference in correctness, but not in "foolproofness". If you call next() at the wrong time, you're screwed. Analyzing your upgrade pass and finding out when it's appropriate to continue will still be required unless we start using one transaction per version.

> > @@ +423,3 @@
> > >      }
> > >  
> > > +    this.incrementRevision(aTransaction);
> > 
> > We want to increment the revision on every individual version bump after 11,
> > you have to call it after each step.
> 
> Why? Once after the upgrade is enough.

Hmm, I remember explicitly choosing to put the increment inside of the loop as opposed to simply incrementing if an upgrade is run, but I now can't remember why.

Comment 10

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 774405 [details] [diff] [review]
patch

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

This looks much better! r=me with this stuff addressed:

::: dom/contacts/fallback/ContactDB.jsm
@@ +97,5 @@
>      let db = aDb;
>      let objectStore;
> +
> +    let steps = [
> +      function () {

Nit: I think it would be more self-documenting if you named these... "function upgrade0to1()", "function upgrade1to2()", etc.? Otherwise you have to count every time you read this code. But I won't insist.

@@ +121,5 @@
>          objectStore.createIndex("givenNameLowerCase",  "search.givenName",  { multiEntry: true });
>          objectStore.createIndex("telLowerCase",        "search.tel",        { multiEntry: true });
>          objectStore.createIndex("emailLowerCase",      "search.email",      { multiEntry: true });
> +        next();
> +        return;

Nit: return kinda unnecessary

@@ +398,2 @@
>  
> +    var index = aOldVersion;

Nit: let here and elsewhere.

@@ +398,4 @@
>  
> +    var index = aOldVersion;
> +    let done = false;
> +    function next() {

Nit: I think this would be better as "upgradeNextSchemaVersion" or something, but I don't care too strongly so whatever.

@@ +398,5 @@
>  
> +    var index = aOldVersion;
> +    let done = false;
> +    function next() {
> +      if (index >= aNewVersion) {

Shouldn't this just be == ? It isn't possible to have indexedDB call you with a oldVersion > newVersion so unless you have a logic error somewhere in here this should be enough.

@@ +405,4 @@
>        }
> +      if (index >= steps.length) {
> +        dump("Contacts DB upgrade error!");
> +        done = true;

Maybe abort the transaction here too? Continuing with such an obvious problem probably won't work well...

Actually, you could just do an early check before any of this that aNewVersion == steps.length and abort the transaction otherwise. Then you wouldn't have to worry about it.

@@ +411,5 @@
> +        var i = index++;
> +        if (DEBUG) debug("Upgrade step: " + i + "\n");
> +        steps[i]();
> +      } catch(ex) {
> +        dump("Caught exception" + ex);

I think this should trigger an abort, really. Otherwise what state will your db end up in?

@@ +418,5 @@
> +      }
> +    };
> +    next();
> +    while (!done) {
> +      Services.tm.currentThread.processNextEvent(true);

Eeeeek! This should be totally removable if you call your default contacts stuff from inside next() only where you currently set done to true. Then it will always run after the other upgrade steps and you won't have to do anything extra. You can remove 'done' too.

Comment 11

[Gregor Wagner [:gwagner]]

Attachment: patch

Comment 12

[Reuben Morais [:reuben]]

Comment on attachment 775059 [details] [diff] [review]
patch

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

::: dom/contacts/fallback/ContactDB.jsm
@@ +796,5 @@
>      this.newTxn("readonly", REVISION_STORE, function (txn, store) {
>        store.get(REVISION_KEY).onsuccess = function (e) {
>          aSuccessCb(e.target.result);
>        };
> +    }, aErrorCb);

You want |null, aErrorCb|. The first one is the transaction success callback.

@@ +805,5 @@
>      this.newTxn("readonly", STORE_NAME, function (txn, store) {
>        store.count().onsuccess = function (e) {
>          aSuccessCb(e.target.result);
>        };
> +    }, aErrorCb);

Same here.

Comment 13

[Julien Wajsberg [:julienw]]

BTW, I've seen that the failure cb is not used a lot, and when there is an error but no failureCb, the IndexedDBHelper has a very unpretty error because it tries to call it inconditionally. I'd suggest to add a log even if DEBUG is false in that case. (should I file a new bug ?)

Comment 14

[Gregor Wagner [:gwagner]]

(In reply to Reuben Morais [:reuben] from comment #12)
> Comment on attachment 775059 [details] [diff] [review]
> patch
> 
> Review of attachment 775059 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/contacts/fallback/ContactDB.jsm
> @@ +796,5 @@
> >      this.newTxn("readonly", REVISION_STORE, function (txn, store) {
> >        store.get(REVISION_KEY).onsuccess = function (e) {
> >          aSuccessCb(e.target.result);
> >        };
> > +    }, aErrorCb);
> 
> You want |null, aErrorCb|. The first one is the transaction success callback.
> 
> @@ +805,5 @@
> >      this.newTxn("readonly", STORE_NAME, function (txn, store) {
> >        store.count().onsuccess = function (e) {
> >          aSuccessCb(e.target.result);
> >        };
> > +    }, aErrorCb);
> 
> Same here.

Huh good catch! I wonder why it was working for me. Probably a bad merge.

Comment 15

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/projects/birch/rev/3bc72e3c8e3a

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3bc72e3c8e3a

Comment 17

[bhavana bajaj [:bajaj]]

Can we get some unit tests or some kind of automation here ?

Comment 18

[Michael Henretty [:mikehenrty][:mhenretty]]

bhavana,

Reuben has been working on testing the upgrade path as part of this bug:
https://bugzilla.mozilla.org/show_bug.cgi?id=889239

More specifically, here are the tests: 
https://bugzilla.mozilla.org/attachment.cgi?id=773693&action=diff

I think those test will be good enough to test the changes here as well.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

Needs a branch-specific patch for uplift.

Comment 20

[Gregor Wagner [:gwagner]]

Attachment: b2g18 version

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Thanks!

https://hg.mozilla.org/releases/mozilla-b2g18/rev/90230132386b

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/90230132386b

Comment 23

[Gregor Wagner [:gwagner]]

(In reply to Michael Henretty [:mikehenrty] from comment #18)
> bhavana,
> 
> Reuben has been working on testing the upgrade path as part of this bug:
> https://bugzilla.mozilla.org/show_bug.cgi?id=889239
> 
> More specifically, here are the tests: 
> https://bugzilla.mozilla.org/attachment.cgi?id=773693&action=diff
> 
> I think those test will be good enough to test the changes here as well.

It might be good to have a chrome test that generates a json file with contact that we can load as initialContacts.

Comment 24

[Julien Wajsberg [:julienw]]

btw, I don't know if this is related to this (I'll try to do more testing later) but I got a seemingly empty contact db after an upgrade today.

Comment 25

[Preeti Raghunath(:Preeti)]

:julienw, any results per your testing?

Comment 26

[Julien Wajsberg [:julienw]]

I tested again (but with a slightly different db) and have not reproduced...