891797 - [OTA][Data Migration][MediaDB] Data of Gallery, Video, and Music app is different than original after update from v1.0.1 to v1.1.0.

Status: VERIFIED FIXED

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

Description

[Hubert Lu[:hlu] <[e-mail]>]

Attachment: logcat

The playlist of highest rated and most played is not kept as original lists after update from v1.0.1 to 1.1.0.

* Build Number (Both Gecko and Gaia)
Gaia:     e251ee6bdab13d8620afa8f9c2d5f14e5e6a4f99
  B-D     2013-07-09 18:41:51
Gecko:    http://hg.mozilla.org/releases/mozilla-b2g18/rev/5b34e0cda635
BuildID   20130709230203
Version   18.0

* Reproduce Steps
1. Flash device to v1.0.1 build from pvt
2. Change update URL to http://update.boot2gecko.org/inari/1.1.0/nightly/update.xml
3. Push some musics into SD card.
4. Open music app.
5. Check the musics
6. Play some musics and rate some musics.
7. Download and install gaia/gecko OTA update.
8. After auto-restart, launch music app.
9. Check the list of highest rated and most played.

* Actual results:
The list of higest rated and most played is different with the original lists after update

* Expected results:
The list should be kept as original list after update.

Comment 1

[Hubert Lu[:hlu] <[e-mail]>]

Attachment: The pictures for playlist of highest rated and most played.

Comment 2

[John Hu [:johnhu][:johu][:醬糊小弟]]

After some investigations, the video app, gallery app are also affected. The watched videos are marked as unwatched. The gallery app deletes all items and re-add all of them back at the first start up.

This bug may be originated from mediadb.

Comment 3

[John Hu [:johnhu][:johu][:醬糊小弟]]

After a bunch of tests on mediadb.js, the root of this bug is caused by DeviceStorage returns absolute file path in v1.1 but relative path in v1.0.1. It is made by bug 858416. There is another camera bug met this before: bug 873971.

After upgraded, the old indexedDB contains file name with relative path, like: DCIM/103MZLLA/VID_0001.3gp, DCIM/103MZLLA/VID_0003.3gp, etc. But the DeviceStorage of v1.1 returns the path like: /sdcard/DCIM/103MZLLA/VID_0001.3gp, /sdcard/DCIM/103MZLLA/VID_0003.3gp, etc. That makes mediadb removes all records[1] and inserts all with absolute path back[2]. Like the following logs:
E/GeckoConsole( 1759): Content JS LOG at app://music.gaiamobile.org/shared/js/mediadb.js:1347 in compareLists: case 5: /sdcard/DCIM/103MZLLA/VID_0001.3gp
E/GeckoConsole( 1759): Content JS LOG at app://music.gaiamobile.org/shared/js/mediadb.js:1347 in compareLists: case 5: /sdcard/DCIM/103MZLLA/VID_0003.3gp
E/GeckoConsole( 1759): Content JS LOG at app://music.gaiamobile.org/shared/js/mediadb.js:1347 in compareLists: case 5: /sdcard/DCIM/103MZLLA/VID_0046.3gp
...
E/GeckoConsole( 1759): Content JS LOG at app://music.gaiamobile.org/shared/js/mediadb.js:1314 in compareLists: case 3: DCIM/103MZLLA/VID_0001.3gp
E/GeckoConsole( 1759): Content JS LOG at app://music.gaiamobile.org/shared/js/mediadb.js:1314 in compareLists: case 3: DCIM/103MZLLA/VID_0003.3gp
E/GeckoConsole( 1759): Content JS LOG at app://music.gaiamobile.org/shared/js/mediadb.js:1314 in compareLists: case 3: DCIM/103MZLLA/VID_0046.3gp
....

[1] https://github.com/mozilla-b2g/gaia/blob/f1d2e3fd806dc55f167c72ac8ef7a3b6baed915e/shared/js/mediadb.js#L1284
[2] https://github.com/mozilla-b2g/gaia/blob/f1d2e3fd806dc55f167c72ac8ef7a3b6baed915e/shared/js/mediadb.js#L1307

And before doing this, I had dumped the rating value from indexedDB. It is existed:
E/GeckoConsole( 1759): Content JS LOG at app://music.gaiamobile.org/shared/js/mediadb.js:1275 in compareLists: name: VoiceRecorder/2013-04-17 10-46-25.mp3
E/GeckoConsole( 1759): Content JS LOG at app://music.gaiamobile.org/shared/js/mediadb.js:1277 in compareLists: rated: 5
E/GeckoConsole( 1759): Content JS LOG at app://music.gaiamobile.org/shared/js/mediadb.js:1275 in compareLists: name: VoiceRecorder/2013-04-17 11-20-17.mp3
E/GeckoConsole( 1759): Content JS LOG at app://music.gaiamobile.org/shared/js/mediadb.js:1277 in compareLists: rated: 4
E/GeckoConsole( 1759): Content JS LOG at app://music.gaiamobile.org/shared/js/mediadb.js:1275 in compareLists: name: VoiceRecorder/2013-06-09 01-01-04.mp3
E/GeckoConsole( 1759): Content JS LOG at app://music.gaiamobile.org/shared/js/mediadb.js:1277 in compareLists: rated: 3


If it is true that v1.0.1 uses relative path and v1.1 uses absolute path in DeviceStorage, it is possible to reconstruct all relative path back to absolute path. But the following questions need to be answered:
1. What's the mount point of storage in v1.0.1??
2. Are all of the mount point the same in v1.0.1 with different devices?
3. If a devices has internal and external storage, which one will be mounted as storage as v1.0.1??
4. Is it possible to know the full path of internal and external storage in v1.1??

Dave, would you mind to give us some feedbacks about above questions???

Comment 4

[Dave Hylands [:dhylands]]

(In reply to John Hu [:johnhu] from comment #3)
> After a bunch of tests on mediadb.js, the root of this bug is caused by
> DeviceStorage returns absolute file path in v1.1 but relative path in
> v1.0.1. It is made by bug 858416. There is another camera bug met this
> before: bug 873971.
...snip...
> If it is true that v1.0.1 uses relative path and v1.1 uses absolute path in
> DeviceStorage, it is possible to reconstruct all relative path back to
> absolute path. But the following questions need to be answered:

1.1 always returns fully-qualified paths. However, relative paths should still work. There is a setting (which is also reflected into a preference) introduced in 1.1 which names the "default" storage name.

The setting (and preference) is called: device.storage.writable.name (treat the word writable as default).

> 1. What's the mount point of storage in v1.0.1??''

It doesn't matter.  It isn't really an absolute path, but rather a fully qualified path of the form:

/storage-name/storage-relative-path

So, for all of our 1.0.1 devices, storage-name will be "sdcard".

The translation from the fully qualified /storage-name/storage-relative-path into the absolute pathname: /storage-mount-point/storage-relative-path takes place internally in device storage.

You can determine the names of all of the storage areas by enumerating the device storage objects returned by navigator.getDeviceStorages and looking at the storageName attribute.

> 2. Are all of the mount point the same in v1.0.1 with different devices?

You don't use mount points, but rather storage-names, which are independent from the mount-point.

> 3. If a devices has internal and external storage, which one will be mounted
> as storage as v1.0.1??

1.0.1 doesn't support multiple storage areas. There is only one, and its called sdcard.

In 1.1 all storage areas will be mounted at the same time (if they're available). relative paths will be converted into fully-qualified paths using the setting mentioned above.

> 4. Is it possible to know the full path of internal and external storage in
> v1.1??

You don't need to know.

Comment 5

[John Hu [:johnhu][:johu][:醬糊小弟]]

Thanks Dave.

'device.storage.writable.name' really helps.

This bug is caused by the mismatching of file name between mediadb and device storage. With this setting, it is possible to convert relative path to absolute for matching.

I will start work on that.

Comment 6

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: upgrade with ObjectStore.mozGetAll()

This patch does the following changes:
1. check the name of dbfiles at fullscan
2. update relative name to full qualified name
3. remove the relative name record
4. put the full qualified name back.

Comment 7

[David Flanagan [:djf]]

Comment on attachment 778340 [details]
upgrade with ObjectStore.mozGetAll()

John,

This is a hard bug. Thank you for diagnosing it and creating a patch.

I've left a number of comments on github.  I think you're on the right track here, but I'd like to see you do the upgrade in the onupgradeneeded callback.  I think you should always hardcode "/sdcard/" as the absolute file prefix.  And I suspect that you can do the various db adds and deletes independently of each other, without having to wait for one to complete before starting the next.

I am not an IndexedDB expert, but let me know if you'd like help with this.

Comment 8

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 778340 [details]
upgrade with ObjectStore.mozGetAll()

This patch has the following changes:
1. Moves the upgrade code to onupgradeneeded.
2. use "sdcard" directly
3. update indexedDB in a single transaction

I think I should explain why I not put the code inside onupgradeneeded in last patch. It is because the version number from indexedDB can not give us significant information. There are two versions in mediadb: MediaDB.VERSION, and options.version. But we use "multiply" to concat them. When we need to do an upgrade with only a single value, it is not possible to know which one is changed. In this case, MediaDB.VERSION is changed, but options.version is not changed. So that, we can't use the old version in the event of onupgradeneeded to know how many operations we need to do, like which case should be deleted and which case should do the data upgrade. So, I put the code in the fullscan. In this method, we may do the upgrade totally in the background and fullscan is not called frequently.

BTW, it may be nice to use shift operation to concat two version numbers, like the concept of upper word and lower word which keeps two shorts within an int. So that we can know which part should do the upgrade and introduce the onupgradeneeded to the consumer of mediadb, like music, video, gallery, etc.

Comment 9

[David Flanagan [:djf]]

(In reply to John Hu [:johnhu] from comment #8)
> Comment on attachment 778340 [details]
> update the file name from relative to full qualified name
> 
> This patch has the following changes:
> 1. Moves the upgrade code to onupgradeneeded.
> 2. use "sdcard" directly
> 3. update indexedDB in a single transaction

Does this seem to work okay? This was the part I was not entirely sure about. I think if there is an error on any one of the updates, the entire transaction will fail, but I don't think we should get an error, so it should be okay.

> I think I should explain why I not put the code inside onupgradeneeded in
> last patch. 

This is a very good explanation. Thank you. In the future, feel free to explain things like this to me first before updating your patch. It is fine to argue with me (or any reviewer) if you feel that my review comments indicate that I didn't understand an important part of the patch.

It is because the version number from indexedDB can not give us
> significant information. There are two versions in mediadb: MediaDB.VERSION,
> and options.version. But we use "multiply" to concat them. When we need to
> do an upgrade with only a single value, it is not possible to know which one
> is changed. In this case, MediaDB.VERSION is changed, but options.version is
> not changed. So that, we can't use the old version in the event of
> onupgradeneeded to know how many operations we need to do, like which case
> should be deleted and which case should do the data upgrade.

Let's try to fix that as part of this bug, so that in the future we don't have this problem.  I don't think that deleting the object stores is every going to be the right thing to do, actually.  A full solution will require apps to be able to pass an updateRecord() function in, I think.  But maybe we can defer that.

 So, I put the
> code in the fullscan. In this method, we may do the upgrade totally in the
> background and fullscan is not called frequently.

I didn't think about the issue of doing it in the background. How long does an upgrade typically take? The screen remains blank during the upgrade, doesn't it?

Could we compromise here and use onupgradeneeded to determine if an upgrade is necessary and then set a flag, and then check the flag during the full scan?

> 
> BTW, it may be nice to use shift operation to concat two version numbers,
> like the concept of upper word and lower word which keeps two shorts within
> an int. So that we can know which part should do the upgrade and introduce
> the onupgradeneeded to the consumer of mediadb, like music, video, gallery,
> etc.

Yes, that is an excellent idea.  How about options.version << 8 + MediaDB.VERSION?

Do you think it is safe to make that change in this patch so that we don't have the problem in the future?

Comment 10

[David Flanagan [:djf]]

Comment on attachment 778340 [details]
upgrade with ObjectStore.mozGetAll()

John,

I have not actually looked at the current patch because (as commented above) your explanation of how you did it the first time make me think that my original review was partially mistaken.

If you make changes based on my comments here, just reset r? when you're ready, or if you want me to review it as it is, just reset r?

Comment 11

[David Flanagan [:djf]]

Now that I think about it some more, I'm realizing that even when doing the upgrade during the first scan you still end up with a blank screen, because the media app will enumerate the database, get out-of-date records, and may or may not be able to display correct data (because of the incomplete paths).  So in that sense, waiting to do the update during the scan is too late.

If all media apps were guaranteed to do a complete enumerate() call when they first started up (Gallery and Video do this, but I think Music does not) then we could do the upgrade during that initial enumerate() call, so that apps always see good, updated, records and never see the bad records.

If we can't count on that, then updating in onupgradeneeded might be the only truely safe way to do this.  But then we'd probably want some UX to display an "upgrading database; please wait" message to the user.

In this particular case, none of the actually devices upgrading from 1.0.1 to 1.1 will have internal storage available (I think). So relative paths will always work for retrieving the files (right?). If I understand correctly, the gallery app (for example) will actually display the photos, and then on the scan will delete them all and add them all back.

So maybe your original patch, (without updating the version number at all) is the best way to fix this.

Obviously, I'm not confused about the best way to fix this, John. I think you've thought about it more than I have.  Sorry to be a bad reviewer!

Comment 12

[David Flanagan [:djf]]

I've taken some more time to think about this bug and the two approaches we've discussed.  Here are my most recent thoughts. I hope they are someone clearer than the comments above.

1) The first approach you tried was to update database records during the full scan. This did not involve changing the database version number and did not trigger or use the onupgradeneeded callback.  

Advantages: because of the details of this upgrade, the media apps should be able to display their data using the old relative filenames before the upgrade happens, so the user still sees a responsive app. But the scanning process takes longer in the background.

Disadvantages: this is a special case patch that works for this upgrade but doesn't help us with future upgrades, and does nothing to prevent the same data loss issue if the media apps ever update their version number.

Also, I think there is a bug with this approach.  The gallery app enumerates all database records when it starts up, before the scan starts, and it retains those records in memory forever. The upgrade during the scanning process won't affect the in-memory copy of the db records, so during the first run after the v1.0->v1.1 update, we'll have relative filenames in memory and (after the scan and db update) absolute filenames in the db. If some other app now deletes one of the photos while the gallery app is still running (I don't think any of the built-in apps could do this, but imagine a third party file manager app) the Gallery will get an event from MediaDB about the file deletion. It will specify an absolute filename that will not match any of the relative filenames that Gallery has in memory and Gallery won't be able to figure out which thumbnail to remove.

I think the solution to this is that the mediadb enumerate() method has to be modified to prefix any relative filenames with /sdcard/.  But if it does that, then it is reporting a DB key that does not yet actually exist in the db.  So perhaps that means that the enumerate() method should do the db update for any relative filename record it finds?

I think that Gallery and Video both do a full enumeration before scanning, so the db could be fully updated during that enumeration.  But I'm not certain that the Music app does a full enumeration before the scan. (I think it might just look for albums on startup, not for all tracks). So the patch would still have to update records with relative paths during the scan process as well.

If you take this approach, I think you should file a followup bug (you can assign it to me) to make some of the MediaDB version number and upgrade improvements from approach 2.

2) The second approach we've discussed is to increment the MediaDB version number and do the update in the IndexedDB onupgradeneeded callback.

Advantages: This bug has taught me that MediaDB can't handle db updates without data loss. If we fix it this way, we can help minimize similar upgrade problems in future releases. Also, doing the db update before the media apps ever use the db is cleaner: the apps will never see the old relative filenames. 

Disadvantages: the db update is at application startup time and would require additional UX to let the user know it is happening. This approach is a more general fix than is strictly required for this bug, so it might be slightly riskier.

To fix the bug this way, I think we'd have to do these things:

1) Update MediaDB.VERSION to 3.

2) Change the calculation of the indexeddb version number as you suggested.

3) In the onupgradeneeded callback, we'd have to fire mediadb events "upgradestart" and "upgradeend" so that client apps can display a message to the user.  The upgradeend event should maybe include a boolean indicating whether the db transaction completed successfully or not so that apps can detect update failures if those ever occur.

4) The Gallery, Video and Music apps would have to be modified to listen for those events and display appropriate messages. All three of those apps already have code for displaying similar messages, so this should be easy to do (except for the late l10n part).

5) We'd have to remove the code that deletes the existing object stores when the version number changes.

6) We'd add your code for doing this relative to absolute path update when changing from MediaDB version 2 to version 3.

7) We'd also want to add code for handling changes to the client app's version numbers.  I imagine that if the video app increments options.version, and options.updateRecord is specified, then we'd pass each db record to updateRecord. If it returned an object, then we'd pass that object to updateMetadata()  (Or something like that: we don't have a concrete use case for this yet, so I'm not sure what is best here.)

I think I prefer the second approach, but I would also be okay with the first approach. Which seems better to you, John?

Comment 13

[John Hu [:johnhu][:johu][:醬糊小弟]]

I also think the second approach in onupdateneeded is the correct way. It is clean and more straight forward. But this may make this patch be a huge patch. If it is ok, I can do it now.

Replying to a question in Comment 9:
I had tried to use separate transactions to do the upgrade. But it failed with invalid state error: a mutation operation was attempted on a database that did not allow mutations. It happens when I try to use the db in onupgradeneeded[1] to create transaction.

To solve it, I had read the code of calendar to find out how they did. I found there is a opened transaction for data upgrade in the event supplied with onupgradeneeded[2] and they also use it to do the data upgrade. So, that's the only transaction I can get. That's why I group all data manipulations into this transaction.

[1] https://github.com/mozilla-b2g/gaia/blob/72d0d7a29b725cf554c8241ea2204625b27dc3ed/shared/js/mediadb.js#L424
[2] https://github.com/mozilla-b2g/gaia/blob/72d0d7a29b725cf554c8241ea2204625b27dc3ed/apps/calendar/js/db.js#L153
[3] https://github.com/mozilla-b2g/gaia/blob/72d0d7a29b725cf554c8241ea2204625b27dc3ed/apps/calendar/js/db.js#L270

Comment 14

[John Hu [:johnhu][:johu][:醬糊小弟]]

The updateMetadata can't be used, because we can't use media.db to create a transaction in this case, a mutation exception. I can write a function to put metadata back to file when client returns the updated metadata.

Comment 15

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 778340 [details]
upgrade with ObjectStore.mozGetAll()

This patch does the following changes:
1. change the calculation of db version to use shift operation
2. fire upgradestart, upgradeend events
3. introduce updateRecord, updateIndexes method to client app
4. show upgrading dialog when mediadb upgrades itself.
5. add another state: upgrading to mediadb
6. convert file path from relative path to absolute path.

To have consistency data upgrading, the upgrade from mediadb will be the first, the upgrade of index change from client app is the second, and the upgrade of data from client app is the final. This way makes client app get the latest version of data before they upgrade the indexes and records.

As to error handling, I keep the code to recreate object stores when the upgrade of mediadb is failed. This is because we will only ship correct code to do upgrade, and that should be our promise to do so. Although all client apps are made by us, client apps should handle the error by themselves.

I had created a new state "upgrading" which will be the middle of opening and ready, if everything is fine. With this state, the client app can know what's going on when they miss the event.

I also update the document at the top of mediadb.js. Hope I don't miss anything.

I don't think my English is good enough to write the correct upgrade message. If you can provide me your version, I can change it ASAP.

Finally, thanks for your previous reviewing and discussing.

Comment 16

[David Flanagan [:djf]]

Comment on attachment 778340 [details]
upgrade with ObjectStore.mozGetAll()

John,

This patch is looking great, but isn't quite ready to land yet. See my comments on github.

Thank you for thinking about the update case where the client app wants to update the set of indexes. I would have forgotten that case. My most substantive comment is that you don't need an updateIndexes function to handle that.

Please treat most of my comments as suggestions. If they don't make sense to you that might be because they don't actually make sense :-)

I'm not sure how to best test this patch. Along with updating the PR would you describe how you have tested it?

Comment 17

[John Hu [:johnhu][:johu][:醬糊小弟]]

I tested it with the following procedure:

1. flash device with 1.0.1 image from pvtbuild
2. use the tool from https://github.com/Mozilla-TWQA/B2G-flash-tool to change the OTA server, command: ./change_OTA_URL.sh -u http://update.boot2gecko.org/inari/1.1.0/nightly/update.xml
   (After that your device will reboot.
   In this step, if you use a Mac computer, you may suffer a strange path error. In this case, you need to use: `adb shell ls /data/b2g/mozilla/` command to find a folder name ending with .default. And modify the change_OTA_URL.sh to have the correct prefs_path.)
3. use app to scan media files in music/video/gallery. Open music/video to do some operations, like rating, viewing video, etc.
4. check if there is upgrade notification, if not use Settings app -> device information -> check for updates -> check now to force checking.
5. tap the upgrade notification, wait for image download/uncompressing, and install it.

After those 5 steps, we may have a v1.1 device with old indexedDB. It may consume about 5 mins to setup the testing environment. To have an easy way to redo the tests, we need to backup the old indexedDB, but the device may not allow it, (/system is mount as readonly):

6. remount the system partition to put busybox, commands:
   a. adb shell    (enter shell mode of device)
   b. mount    
      (you can find a record looks like /dev/block/mtdblock? /system yaffs2 ro,relatime 0 0
      The ? is a number. The most important thing is /system. We need to make it become rw)
   c. mount -o remount,rw /dev/block/mtdblock? /system
      (The ? is the number you found at step b.)
   d. exit
7. push busybox-armv6l in build folder to device, commands:
   a. push build/busybox-armv6l /system/bin/busybox
   b. adb shell chmod 755 /system/bin/busy box
8. back up the folder /data/local/indexedDB, commands:
   a. adb shell    (enter shell mode of device)
   b. cd /data/local/
   c. busybox cp -r indexedDB indexedDB-bk
      In this step, you can copy to other folder, like /sdcard/. But you need to know the the file system of that partition. If in sdcard, it should be fat32. You may loose all permissions, so that when you copy back to /data/local/, you need to do chmod for all files and folders.

After those 8 steps, we may have a backup of indexedDB. It's time to put the modified code to device. But, we CAN'T use normal "make install-gaia APP=music" to put v1-train code in. That results the calculation of folder name of indexedDB changed. I don't know why. But it is that. If we list the folder /data/local/indexedDB/ it starts with 4 digitals followed by a few + and app name. When we do install-gaia, it changes to 1 or 2 digitals followed by a few + and app name. It still can use, but we may need to change the backup folder and file name to the other one. That's not convenient. The way I use is just replace the application.zip to test it:

1. make      (this will build all files under profile folder)
2. adb push profile/webapps/music.gaiamobile.org/application.zip /system/b2g/webapps/music.gaiamobile.org/application.zip
   (you can change directory to profile/webapps/music.gaiamobile.org/ for typing shortly)
3. start app and see the console.

We have to reset the indexedDB once a run, commands:
1. adb shell        (enter shell mode of device)
2. cd /data/local/
3. rm -r indexedDB
4. busybox cp -r indexedDB-bk indexedDB

If we need to test the code to handle inexistent db, we can remove the indexedDB with command: rm -r indexedDB.

If we need to test the upgrade of client app, we can change the code in app, and use the same way to push it back.

Comment 18

[John Hu [:johnhu][:johu][:醬糊小弟]]

Sorry, there is some typo in the comment 17:

7, b.: adb shell chmod 755 /system/bin/busybox

Comment 19

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 778340 [details]
upgrade with ObjectStore.mozGetAll()

I had change the patch with the following items:
1. enlarge the version number to 32 bits, upper 16 bits for client app, lower 16 bits for mediadb
2. remove the updateIndexes method and use a new method upgradeIndexesChanges to compare and update the changes of indexes
3. remove upgradeend event and rename the upgradestart to upgrading
4. move inexistent database handling code to onupgardeneeded and don't dispatch event for it.
5. some naming changes.
6. remove the code of handling transaction abort.

Actually, I met a transaction problem at item 6. So, I removed the code about transaction abort handling.

With the previous patch, I had double checked the behavior of event "onabort" and "onerror". From MDN[1], the default behavior of onerror event is to abort the transaction. When we meet multiple errors, the onerror event is dispatched multiple times but one abort. That may not be the correct to handle it inside onerror. I changed to use onabort. We can receive the onabort event only once no matter how many onerror events dispatched. But in that situation, we can't use this transaction anymore, because it is aborted. From MDN[2], "You can create or delete an object store only in a `versionchange transaction` which is the mode of the transaction used in the handler for the upgradeneeded event". With an aborted transaction, I can't create another versionchange transaction with the db from both event.target.result or openRequest.result. And I know the createObjectStore is the member function of IDBDatabase and may not be related to transaction. But no matter to create a transaction or not, it always gives us "A mutation operation was attempted on a database that did not allow mutations" when calling createObjectStore". I don't know why. So, I remove the code and let it fallback to openRequest.onerror to handle the error.

The way I testing transaction abort is to throw a uncatched Error inside of handleUpgrade function. If we throw the error after the update of file records, we  can receive multiple onerror event calls, and one onabort event call. If we throw the error before the update of file records, we can receive one onerror event call, and one onabort event call. But we can't use transaction anymore no matter onerror or onabort.

[1] https://developer.mozilla.org/en-US/docs/IndexedDB/Using_IndexedDB#Adding_data_to_the_database 
[2] https://developer.mozilla.org/en-US/docs/Web/API/IDBDatabase#Basic_concepts

Comment 20

[David Flanagan [:djf]]

Comment on attachment 778340 [details]
upgrade with ObjectStore.mozGetAll()

John,

I continue to be very happy with the work you're doing on this bug. Thank you.

But r- again, sorry.  I'd like you to simplify upgradeIndexChanges() and, if possible, call it before doing the db enumeration, so that if that is the only change being made the enumeration doesn't even need to happen.

Also, I noticed this time, that by copying updateMetadata() for the updateRecord() callback, there is no way for a client app to delete metadata properties from records.  Sorry I didn't see that sooner.

See my comments on github for the details.

Thank you also for the long comment explaining how to test this patch. I don't think I'll have time to test myself today, but it sounds like you're testing carefully.

We've had a number of bugs in the media apps about the wrong message overlays being displayed. As you've probably noticed, the logic in those apps is somewhat convoluted. So I'd ask you to test the upgrade with no sdcard in the phone and with and sdcard that is mounted for mass storage.  I think you should see the "database upgrade" message and then it should switch to the "no sdcard" or "sdcard in use" message.

And I just realized that you should also test the case where the upgrade is done in response to a pick request. (Like selecting wallpaper, or adding an attachment to an MMS message)

Finally, have you tested the upgrade performance for medium-to-large databases?  If there are 500 photos or 500 songs, for example, how long does the upgrade take? I'd be concerned if it is more than 10 seconds or so.

Comment 21

[John Hu [:johnhu][:johu][:醬糊小弟]]

Testing result:

1. upgrade without sdcard: (all three apps are the same)
    It shows upgrading message. After upgraded, it shows no sdcard message
2. upgrade with sdcard mounted as USB storage: (all three apps are the same)
    It shows upgrading message. After upgraded, it shows sdcard is mounted as usb device
3. upgrade with pick activity: (all three apps are the same)
    It shows upgrading message. After upgraded, it lists all items for picking
4. benchmark: (only tested with music app)
    I had did the four version: with use ObjectStore.mozGetAll(), ObjectStore.openCursor() manipulating with both cursor and store, ObjectStore.openCursor() manipulating with store, and ObjectStore.openCursor() manipulating with store with gaia optimized option:

ObjectStore.mozGetAll()
======================================================================
Records          Time
57               766
157              2823
257              5314
357              8048
457              10857
557              13549
-----------------------------------------------------------------------

ObjectStore.openCursor() manipulating with both cursor and store (note #1)
======================================================================
Records          Time
57               1906
157              7168
257              12252
357              20848
457              28025
557              34695
-----------------------------------------------------------------------

ObjectStore.openCursor() manipulating with store (note #2)
======================================================================
Records          Time
57               1138
157              4177
257              7260
357              10941
457              13832
557              20308
-----------------------------------------------------------------------

ObjectStore.openCursor() manipulating with store with gaia optimized options
======================================================================
Records          Time
57               1082
157              4038
257              7002
357              10670
457              13781
557              18447
-----------------------------------------------------------------------

note #1: There may be a question here: why not all cursor? This is because we need to delete the relative file path and add it back. Once cursor.delete() is called, the cursor position is become null. If we try to use cursor.update to add the absolute file path back, it says object store's key path does not match the key in this cursor's position. So, I use cursor to delete, and store to add and put.

note #2: After previous test, I found it still possible to use store to do data manipulation. I change code to use cursor to list all records and use store to delete/add/put records.

note #3: Another curious question here: if we use cursor or store to delete/add/input, does cursor.continue() go to the added record? I had did this test, too. It doesn't advance to the new added record.

Comment 22

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: benchmark datasheet

Comment 23

[John Hu [:johnhu][:johu][:醬糊小弟]]

Comment on attachment 778340 [details]
upgrade with ObjectStore.mozGetAll()

1. simplify upgradeIndexChanges
2. separate index upgrade and data upgrade.
3. add checking code for unnecessary data upgrade
4. change the way to deal with metadata update

This PR uses ObjectStore.mozGetAll() to do data upgrade. Please see benchmark datasheet to have more information about its performance.

Comment 24

[John Hu [:johnhu][:johu][:醬糊小弟]]

Attachment: upgrade with cursor but update with ObjectStore

1. simplify upgradeIndexChanges
2. separate index upgrade and data upgrade.
3. add checking code for unnecessary data upgrade
4. change the way to deal with metadata update
5. list all files with IDBCursor and simplify the upgrade code of db ver 2 to 3 and client app upgrade.

This PR uses ObjectStore.openCursor() to list all data, and use ObjectStore.delete()/add()/put() to do data manipulation. Please see benchmark datasheet to have more information about its performance.

Comment 25

[John Hu [:johnhu][:johu][:醬糊小弟]]

David, 

Sorry for two patch for this bug. Please choose a version and mark the other one as r-.

After the benchmark and tests, I prefer listing files with cursor, IMHO. I can accept slow but not OOM. Although all of them don't meet your requirement (less than 10 seconds), I still prefer the cursor version.

Finally, I forgot an important thing. The testing device is Inari:

CPU clock: 500 MHz (running upgarde)
       I can't make sure if it boosts up itself to the maximum clock.
Memory: 256MB
Testing File: a 126KB MP3 file copied 500 times with 57 other 3gp, mp4, mp3 files.

All time are in ms. Wifi is turned off which running. The benchmark is only testing the upgrade of MediaDB version 2 to 3. If we do some upgrade inside client app, the result becomes worse.

If we use Leo or Unagi, those devices have CPU with 1 GHz while boosting, we may run the whole upgrade within 10 sec because the usage time seems be linear. But it still can't while client app upgrade exists.

Comment 26

[David Flanagan [:djf]]

Comment on attachment 778340 [details]
upgrade with ObjectStore.mozGetAll()

r- on this one since we both prefer the cursor-based solution.

Comment 27

[David Flanagan [:djf]]

Comment on attachment 780895 [details]
upgrade with cursor but update with ObjectStore

I have a few minor code nits, but feel free to land this once you have addressed them.

I'm disappointed about the upgrade performance, but I think that is a matter of IndexedDB performance, not MediaDB.  I'd guess that Gallery and Video will be faster than Music because they have fewer indexes to update.  

In my github comments I note that eventually we might want to fire the upgrading event multiple times to give progress reports so that client apps can display a progress bar duing the upgrade.  But I think adding upgrade progress to the media apps is feature work that is outside the scope of this bug, so I don't think we need to add that now.

Thank you for your hard work on this, including your careful testing and peformance measurements. MediaDB is a better library now that it can support updates.

Comment 28

[John Hu [:johnhu][:johu][:醬糊小弟]]

Landed to master:
https://github.com/mozilla-b2g/gaia/commit/ccf877b99ace72e621eb5172f042c6c20aa1297a

Comment 29

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

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 ccf877b99ace72e621eb5172f042c6c20aa1297a
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 30

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi John Ford, 

I checked the code which is conflicted because of lacking the result of 830239.

Comment 31

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi John Ford, 

Sorry for last incomplete comment. 

I had checked the code. The conflict part is caused by lacking of the patch of bug 830239. To have more consistent codebase, I suggest to have bug 830239 in the v1-train. That patch can give better experience. And it use more proper way to show the l10n text. I will nominate it as leo?. If this bug can't be approved for v1-train, I will create another patch for this.

Comment 32

[John Hu [:johnhu][:johu][:醬糊小弟]]

They reject to uplift it. I will change the patch for v1-train.

Comment 33

[John Hu [:johnhu][:johu][:醬糊小弟]]

Hi John, 

I had fixed the conflict. Please find the branch on my github and help to uplift:
https://github.com/huchengtw-moz/gaia/tree/mediadb/Bug_891797-v1-train

Thanks.

Comment 34

[David Flanagan [:djf]]

We can't uplift bug 899098 until this one is uplifted.

Comment 35

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

v1-train: 8cbb1028877e086f522020583bbcf0dc9f45f07c

Comment 36

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

v1.1.0hd: 8cbb1028877e086f522020583bbcf0dc9f45f07c

Comment 37

[Hubert Lu[:hlu] <[e-mail]>]

Inari
from v1.0.1
  Gaia:     054cdc27404e2daca91d3065d9783681032b2151
  B-D     2013-07-06 19:39:29
  Gecko:    http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9c62297d11b0
  BuildID   20130812043202
  Version   18.0

to v1.1.0 http://update.boot2gecko.org/inari/1.1.0/nightly/update.xml
  Gaia:     c60e3507d9df160e006d2e25967d26df2dc543cb
  B-D     2013-08-11 21:53:25
  Gecko:    http://hg.mozilla.org/releases/mozilla-b2g18/rev/36bbc5943448
  BuildID   20130812041203
  Version   18.0

Verified.