892520 - [Dialer] CallLog upgrade path error prone

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect)

Description

[:gerard-majax]

When investigating bug 887701 with :julienw, 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

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #0)
> When investigating bug 887701 with :julienw, we saw that the Contacts DB has
                                                               ^^^^^^^^^^^

Call log, of course.

Comment 2

[:gerard-majax]

Attachment: Link to Github https://github.com/mozilla-b2g/gaia/pull/10929

Please find a link to Github pull request https://github.com/mozilla-b2g/gaia/pull/10929

Comment 3

[:gerard-majax]

Comment on attachment 774009 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10929

Putting back r? now that the code has been successfully tested

Comment 4

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 774009 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10929

Thanks Alexandre!

I am sorry, but I don't really see the point for this patch alone :|. The current upgrade path has not any asynchronous call that requires this kind of synchronization mechanism.

I would suggest to add this code when the upgrade code actually requires it.

In any case, I am not a peer of this code, so I'll let Etienne decide if this is required or not. I added a few comments to the PR though.

Comment 5

[Etienne Segonzac (:etienne)]

FWIW I don't think we have a strong case for leo+.

I'm all in favor of setting up a safer approach for future database changes (what this patch does), but we can't argue that this is blocking since the current upgrade code works.

Comment 6

[Etienne Segonzac (:etienne)]

Comment on attachment 774009 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10929

:ferjm is definitely the best reviewer for this patch, happy to give feedback though :)

Comment 7

[:gerard-majax]

(In reply to Fernando Jiménez Moreno [:ferjm] (needinfo instead of CC, please) from comment #4)
> Comment on attachment 774009 [details]
> Link to Github https://github.com/mozilla-b2g/gaia/pull/10929
> 
> Thanks Alexandre!
> 
> I am sorry, but I don't really see the point for this patch alone :|. The
> current upgrade path has not any asynchronous call that requires this kind
> of synchronization mechanism.
> 
> I would suggest to add this code when the upgrade code actually requires it.
> 
> In any case, I am not a peer of this code, so I'll let Etienne decide if
> this is required or not. I added a few comments to the PR though.

Okay, thanks for your feedback to both of you. I'll address the remarks next week, since it's not an urgent matter.

Comment 8

[bhavana bajaj [:bajaj]]

Going by comment #5 I don't think this is a blocker, also I am not aware on any user scenario where this has bitten us yet so the user impact is a bit unclear here.

Comment 9

[Julien Wajsberg [:julienw]]

The only asynchronous upgrade path is the last one so I agree this should not be leo+.

However if we need to add a new upgrade path as part of leo+ for some reason we'll need to uplift this one as well.

An uplift conflict should light up a warning if this happens :)

Comment 10

[:gerard-majax]

while finalizing bug 889245, we are now needing this.

Comment 11

[:gerard-majax]

Basically, current state of bug 889245 makes that we have two upgrades (v4 and v5) that needs to be performed for moving from 1.0.1 and 1.1 or master. The current code lets the cursors running in parallel, but I ran into issues while testing that were symptomatic of parallels cursors getting finished at different times.

Symptom of this was that the call log upgrade was declared finished and yet the call log was empty: because the database was still under upgrade. Once you switched tab (all/missed) or that you kill/restart the dialer, it was okay, but upgrading this way might and will result in dataloss as bug 887701

Comment 12

[:gerard-majax]

Comment on attachment 774009 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10929

Putting r? on this as agreed face to face, but this code will evolve not to be dependant on bug 889245.

Comment 13

[:gerard-majax]

Evolution has been performed.

Comment 14

[bhavana bajaj [:bajaj]]

Understood this block a blocker, what is the risk of this patch here given how close we are to 1.1 ?

Comment 15

[Julien Wajsberg [:julienw]]

The risk is to not use this patch.

Comment 16

[Preeti Raghunath(:Preeti)]

leo+ for uplifting this patch and fixing 889245

Comment 17

[Etienne Segonzac (:etienne)]

Comment on attachment 774009 [details]
Link to Github https://github.com/mozilla-b2g/gaia/pull/10929

All good! (commit a09494d).

Thanks!

Comment 18

[Etienne Segonzac (:etienne)]

https://github.com/mozilla-b2g/gaia/commit/d2856fd2d87c78e0f7b34c3a555cb8cd3a409aff

Comment 19

[John Ford [:jhford] CET/CEST Berlin Time]

Uplifted d2856fd2d87c78e0f7b34c3a555cb8cd3a409aff to:
v1-train: 3f5e92af9c34b2726d12b6db01b4ccd2b5da00de

Comment 20

[Tim Guan-tin Chien [:timdream] (please needinfo)]

v1.1.0hd: 3f5e92af9c34b2726d12b6db01b4ccd2b5da00de