Closed Bug 960741 Opened 11 years ago Closed 11 years ago

messages app fails to upgrade database (JS exception in upgradeSchema14) after updating from 1.2 to 1.3

Categories

(Firefox OS Graveyard :: RIL, defect)

Product:
Firefox OS Graveyard
Component:
RIL
Platform:
ARM
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(blocking-b2g:1.3+, firefox27 wontfix, firefox28 fixed, firefox29 fixed, b2g-v1.2 unaffected, b2g-v1.3 fixed, b2g-v1.4 fixed)

Status:
RESOLVED FIXED
Milestone:
1.3 C3/1.4 S3(31jan)
Project Flags:
blocking-b2g 1.3+
Tracking Flags:
Tracking Status
firefox27 --- wontfix
firefox28 --- fixed
firefox29 --- fixed
b2g-v1.2 --- unaffected
b2g-v1.3 --- fixed
b2g-v1.4 --- fixed

People

(Reporter: dbaron, Assigned: bevis)

References

Details

(Keywords: dataloss, regression)

Attachments

(3 files, 5 obsolete files)

logcat around opening the messages app, with DEBUG = true
(deleted), text/plain
Details
Patch v3: Add deliveryInfo property into messageRecord if not available. r=gene
(deleted), patch
airpingu
: review+
Details | Diff | Splinter Review
Patch v3: [1.3] Add deliveryInfo property into messageRecord if not available. r=gene a=1.3+
(deleted), patch
airpingu
: review+
Details | Diff | Splinter Review
I updated my B2G phone from 1.2 to 1.3 a few days ago (my own builds), and the messages app no longer functions: * it shows no SMS history * I don't receive SMSes When I start the Messages app, I see in logcat: E/GeckoConsole( 139): [JavaScript Error: "undefined has no properties" {file: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js" line: 1078}] E/GeckoConsole( 605): Content JS LOG at app://sms.gaiamobile.org/js/message_manager.js:344 in onerror: Reading the database. Error: InternalError which seems likely to explain it. That exception appears to be here: upgradeSchema14: function upgradeSchema14(transaction, next) { let messageStore = transaction.objectStore(MESSAGE_STORE_NAME); messageStore.openCursor().onsuccess = function(event) { let cursor = event.target.result; if (!cursor) { next(); return; } let messageRecord = cursor.value; if (messageRecord.type == "sms") { messageRecord.deliveryTimestamp = 0; } else if (messageRecord.type == "mms") { let deliveryInfo = messageRecord.deliveryInfo; for (let i = 0; i < deliveryInfo.length; i++) { // THIS IS LINE 1078 deliveryInfo[i].deliveryTimestamp = 0; } } cursor.update(messageRecord); cursor.continue(); }; },
If I set DEBUG = true at the top of the file, my logcat is: I/Gecko ( 1326): MobileMessageDatabaseService: Getting thread list I/Gecko ( 1326): MobileMessageDatabaseService: Getting next thread in list I/Gecko ( 1326): MobileMessageDatabaseService: Opening transaction for object stores: thread I/Gecko ( 1326): MobileMessageDatabaseService: Database needs upgrade: sms 13 20 I/Gecko ( 1326): MobileMessageDatabaseService: Correct new database version: true I/Gecko ( 1326): MobileMessageDatabaseService: Upgrade to version 14. Fix the wrong participants. I/Gecko ( 1326): MobileMessageDatabaseService: Upgrade to version 15. Add deliveryTimestamp. E/GeckoConsole( 1326): [JavaScript Error: "undefined has no properties" {file: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js" line: 1078}] I/Gecko ( 1326): MobileMessageDatabaseService: Could not open database: Error opening database! I/Gecko ( 1326): MobileMessageDatabaseService: Error opening database! I/Gecko ( 1326): MobileMessageDatabaseService: collect: message ID = -1 I/Gecko ( 1326): MobileMessageDatabaseService: An previous error found E/GeckoConsole( 1477): Content JS LOG at app://sms.gaiamobile.org/js/message_manager.js:344 in onerror: Reading the database. Error: InternalError though, oddly, I see another occurrence prior to this of: E/GeckoConsole( 1326): [JavaScript Error: "undefined has no properties" {file: "jar:file:///system/b2g/omni.ja!/components/MobileMessageDatabaseService.js" line: 1078}]
blocking-b2g: --- → 1.3?
messageRecord.toSource() is the following: ({headers:{'x-mms-message-type':132, 'x-mms-transaction-id':"2tid16509999999_2gx u8q", 'x-mms-mms-version':16, from:{address:"+14151111111", type:"PLMN"}, subjec t:"no subject", 'x-mms-message-class':"personal", 'x-mms-message-size':614400, ' x-mms-expiry':259200, 'x-mms-content-location':{uri:"http://atl2mmsget.msg.eng.t -mobile.com/mms/wapenc?location=16509999999_2gxu8q&rid=070"}, 'message-id':"0210 0298BF8E00008B100001~sun5x1035014", date:(new Date(1382975778000)), to:{address: "+16509999999", type:"PLMN"}, 'x-mms-priority':129, 'x-mms-delivery-report':fals e, 'x-mms-read-report':false, 'content-type':{media:"application/vnd.wap.multipa rt.related", params:{type:"application/smil", start:"<0000>"}}}, type:"mms", del ivery:"received", deliveryStatus:["success"], timestamp:1382977203616, sender:"+ 14151111111", transactionId:"2tid16509999999_2gxu8q", receivers:["+16509999999"] , readIndex:[1, 1382977203616], read:1, transactionIdIndex:"2tid16509999999_2gxu 8q", deliveryIndex:["received", 1382977203616], id:79, threadId:11, threadIdInde x:[11, 1382977203616], participantIdsIndex:[[11, 1382977203616]], parts:[{index: 0, headers:{'content-type':{media:"application/smil", params:{name:"123_1.smil"} }, 'content-length':410, 'content-id':"<0000>", 'content-location':"0.smil"}, co ntent:"<smil>\n <head>\n <layout>\n <root-layout height=\"160\" width=\ "240\"/>\n <region fit=\"meet\" height=\"67%\" id=\"Image\" left=\"0%\" top =\"0%\" width=\"100%\"/>\n <region fit=\"meet\" height=\"33%\" id=\"Text\" left=\"0%\" top=\"67%\" width=\"100%\"/>\n </layout>\n </head>\n <body>\n <par dur=\"8000ms\">\n <img region=\"Image\" src=\"cid:274\"/>\n <te xt region=\"Text\" src=\"cid:275\"/>\n </par>\n </body>\n</smil>\n"}, {index :1, headers:{'content-type':{media:"image/jpeg", params:{name:"IMG_0425.jpg"}}, 'content-length':389284, 'content-id':"<274>"}, content:({})}, {index:2, headers :{'content-type':{media:"text/plain", params:{charset:{charset:"utf-8"}, name:"t ext_1.txt"}}, 'content-length':130, 'content-id':"<275>"}, content:({})}]}) (I sanitized the phone numbers by replacing the last 7 digits of the sender's number with 1111111 and the last 7 digits of my number with 9999999.) Note that it does not have a deliveryInfo property.
When I received the MMS, I was running 1.1 (built from source). I upgraded to 1.2 in December and to 1.3 earlier this week.
OK, I investigated this with khuey. The problem was a bad backport of bug 914060. Bug 914060 was backported to 1.2 by RyanVM, with a different database version number than on central. So my profile on 1.2 has database version 13, but database version 13 has a different meaning on central and 1.2. Essentially, my profile has had the upgrade performed that we think of as 13->14, but has *not* had the upgrade performed that we think of as 12->13.
Blocks: 914060
No longer blocks: 887159
I think the fully correct fix for this involves making the upgrade code deal with all the combinations (essentially merging the 12->13 and 13->14 upgrading, and making that merged code handle either upgrade already having been performed). Though if we're willing to break upgrading for people who have been on trunk one could effectively just swap the 13 and 14 version numbers on 1.3 and master after-the-fact (which is what I'm going to do locally tofix my profile).
No longer blocks: 914060
Status: NEW → RESOLVED
blocking-b2g: 1.3? → ---
Closed: 11 years ago
Resolution: --- → DUPLICATE
Let's dupe these the other way, since the problem is already identified and a solution is already proposed here.
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Assignee: nobody → btseng
Root cause found. 1. The DB VERSION mapping in v1.2 and v1.3 is mismatch. 2. VERSION 12 in v1.2 mapped to VERSION 13 in v1.3 which means the logic of VERSION 12 in v.13 was skipped in v1.2. 3. This cause a reference to the null property of messageRecord.deliveryInfo while upgradeSchema14() in MobileMessageDatabaseService.js Solution will be ready after more testing.
Nice catch! Bevis! This bug happened when we attempt to uplift patches crossing branches. Just follow up Bevis' discovers to address the lesson we learnt: v1.3 has the following DB updates: ... upgradeSchema11() upgradeSchema12() upgradeSchema13() upgradeSchema14() ... However, what the upgradeSchema13() did is also a blocker of v1.2, So, we uplift upgradeSchema13() to V1.2 as upgradeSchema12(): ... upgradeSchema11() upgradeSchema12() <-- This is upgradeSchema13() in v1.3 Lesson learnt: if we want to uplift some patches for updating schema, we have to uplift all of its previous DB updates even if they're not supposed to be blockers.
The patch to fix the DB migration problem for master branch.
Attachment #8361504 - Flags: review?(gene.lian)
Attachment #8361505 - Flags: review?(gene.lian)
Comment on attachment 8361504 [details] [diff] [review] Patch Part1: Bug 960741 - messages app fails to upgrade database (JS exception in upgradeSchema14) after updating from 1.2 to 1.3. r=gene Per off-line discussion, cancel the review for now.
Attachment #8361504 - Flags: review?(gene.lian)
Comment on attachment 8361505 [details] [diff] [review] Patch Part1: [1.3] Bug 960741 - messages app fails to upgrade database (JS exception in upgradeSchema14) after updating from 1.2 to 1.3. r=gene a=1.3+ Per off-line discussion, cancel the review for now.
Attachment #8361505 - Flags: review?(gene.lian)
> Lesson learnt: if we want to uplift some patches for updating schema, we have to uplift all > of its previous DB updates even if they're not supposed to be blockers. I had another idea when we had to do this in Contacts (but finally we managed to not need it): have idempotent upgrade steps (so that playing them several times is not an issue), store the current firefox os version in the db (eg: 1.2) and keep an array containing the first step to run when we're moving from 1.2 to the current version.
Update patch v2 to keep upgradeSchema12() unchanged.
Attachment #8361504 - Attachment is obsolete: true
Attachment #8361564 - Flags: review?(gene.lian)
Update patch v2 to keep upgradeSchema12() unchanged.
Attachment #8361505 - Attachment is obsolete: true
Attachment #8361565 - Flags: review?(gene.lian)
Did you check that upgradeSchema13 correctly handles being run twice?
Yes, we also discussed this offline and tested it locally. It shall be safe to run it twice and the test result looks fine.
Comment on attachment 8361564 [details] [diff] [review] Patch Part1 v2: Add deliveryInfo property into messageRecord if not available. r=gene Review of attachment 8361564 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm @@ +189,5 @@ > if (DEBUG) debug("Upgrade to version 13. Replaced deliveryStatus by deliveryInfo."); > self.upgradeSchema12(event.target.transaction, next); > break; > case 13: > if (DEBUG) debug("Upgrade to version 14. Fix the wrong participants."); Add a comment here: // A workaround to check if we need to re-upgrade the DB schema 12. We missed this // because we didn't properly uplift that logic to b2g_v1.2 and errors could happen // when migrating b2g_v1.2 to b2g_v1.3. Please see Bug 960741 for details. @@ +190,5 @@ > self.upgradeSchema12(event.target.transaction, next); > break; > case 13: > if (DEBUG) debug("Upgrade to version 14. Fix the wrong participants."); > + self._workAroundSchema12(event.target.transaction, function(isNeeded) { s/_workAroundSchema12/needReUpgradeSchema12 @@ +2595,5 @@ > return cursor; > + }, > + > + /** > + * Workaround to add deliveryInfo if migrated from b2g_v1.2 with DB_VERSION == 12. * Check if we need to re-upgrade the DB schema 12. @@ +2597,5 @@ > + > + /** > + * Workaround to add deliveryInfo if migrated from b2g_v1.2 with DB_VERSION == 12. > + */ > + _workAroundSchema12: function (transaction, callback) { s/_workAroundSchema12/needReUpgradeSchema12 I'd prefer naming the function as it is supposed to do and put the workaround-related description in the comment of upgrading schemas. And move this function up to somewhere around the DB schema updating logic. Please don't add space between "function" and "(";
Attachment #8361564 - Flags: review?(gene.lian)
Thanks for the comments. :) I'll modify them in next patch. (In reply to Gene Lian [:gene] (PTO Dec. 25 ~ Jan. 5) from comment #23) > Comment on attachment 8361564 [details] [diff] [review] > Patch Part1 v2: Add deliveryInfo property into messageRecord if not > available. r=gene > > Review of attachment 8361564 [details] [diff] [review]: > ----------------------------------------------------------------- > > ::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm > @@ +189,5 @@ > > if (DEBUG) debug("Upgrade to version 13. Replaced deliveryStatus by deliveryInfo."); > > self.upgradeSchema12(event.target.transaction, next); > > break; > > case 13: > > if (DEBUG) debug("Upgrade to version 14. Fix the wrong participants."); > > Add a comment here: > > // A workaround to check if we need to re-upgrade the DB schema 12. We > missed this > // because we didn't properly uplift that logic to b2g_v1.2 and errors could > happen > // when migrating b2g_v1.2 to b2g_v1.3. Please see Bug 960741 for details. > > @@ +190,5 @@ > > self.upgradeSchema12(event.target.transaction, next); > > break; > > case 13: > > if (DEBUG) debug("Upgrade to version 14. Fix the wrong participants."); > > + self._workAroundSchema12(event.target.transaction, function(isNeeded) { > > s/_workAroundSchema12/needReUpgradeSchema12 > > @@ +2595,5 @@ > > return cursor; > > + }, > > + > > + /** > > + * Workaround to add deliveryInfo if migrated from b2g_v1.2 with DB_VERSION == 12. > > * Check if we need to re-upgrade the DB schema 12. > > @@ +2597,5 @@ > > + > > + /** > > + * Workaround to add deliveryInfo if migrated from b2g_v1.2 with DB_VERSION == 12. > > + */ > > + _workAroundSchema12: function (transaction, callback) { > > s/_workAroundSchema12/needReUpgradeSchema12 > > I'd prefer naming the function as it is supposed to do and put the > workaround-related description in the comment of upgrading schemas. > > And move this function up to somewhere around the DB schema updating logic. > > Please don't add space between "function" and "(";
Attachment #8361565 - Flags: review?(gene.lian)
update patch_v_3 to address the suggestions in comment#23
Attachment #8361564 - Attachment is obsolete: true
Attachment #8362363 - Flags: review?(gene.lian)
update patch_v_3 to address the suggestions in comment#23
Attachment #8361565 - Attachment is obsolete: true
Attachment #8362365 - Flags: review?(gene.lian)
Attachment #8362363 - Attachment description: Patch v2: Add deliveryInfo property into messageRecord if not available. r=gene → Patch v3: Add deliveryInfo property into messageRecord if not available. r=gene
Component: DOM: Device Interfaces → RIL
Product: Core → Firefox OS
Version: Trunk → unspecified
Attachment #8362363 - Flags: review?(gene.lian) → review+
Attachment #8362365 - Flags: review?(gene.lian) → review+
Attachment #8361391 - Attachment is obsolete: true
https://hg.mozilla.org/integration/b2g-inbound/rev/032eee590ad8 Also, let me just say that I'm sorry for the role I played in creating this situation. I will be a lot more careful/paranoid about uplifts that touch the SMS DB in the future :)
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/032eee590ad8
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → 1.3 C3/1.4 S3(31jan)
https://hg.mozilla.org/releases/mozilla-aurora/rev/2d9d1d387854 Good time to suggest that we should have automated tests for such regressions? :)
status-b2g-v1.2: --- → unaffected
status-b2g-v1.3: --- → fixed
status-b2g-v1.4: --- → fixed
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
status-firefox29: --- → fixed
Flags: in-testsuite?
I'm not sure this is doable... Or at least not "automated tests to run at each commit" type. Maybe we could have "automated tests that we can run manually to test migrations".
The tests would have to maintain state across checkins.
(In reply to Ryan VanderMeulen [:RyanVM UTC-5] from comment #32) > Good time to suggest that we should have automated tests for such > regressions? :) Filed as bug 963426.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: