807463 - [sms] Possible wrong SMS DB upgrade code.

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect, P1)

Description

[Gregor Wagner [:gwagner]]

In Bug 742790 we did a sms db upgrade but we didn't modify existing entries.
The result is that the new "read" field is missing and this results in an error when we read messages out of the DB.
This means we throw an error and we never display any messages any more.

Comment 1

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

We wouldn't hold the release on this bug, but I'm bb+'ing it for auto-approval for the fix.

Comment 2

[Gregor Wagner [:gwagner]]

This happened because the messageClass is not set correctly which results in an error here:
https://mxr.mozilla.org/mozilla-central/source/dom/sms/src/SmsMessage.cpp#101

Comment 3

[Gregor Wagner [:gwagner]]

Attachment: patch

I will run more tests with this code.

Comment 4

[Gregor Wagner [:gwagner]]

Attachment: patch

We have to update existing entries for messageClass and deliveryStatus. These fields were added in bug 797277 and bug 742790.
Entries that are not updated throw an error around here:
http://hg.mozilla.org/mozilla-central/annotate/c6ccd1d30c15/dom/sms/src/SmsMessage.cpp#l78

Without this patch we probably don't display any messages (not even new messages) but they should show up again with this patch.

Comment 5

[Philipp von Weitershausen [:philikon]]

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

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

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +257,5 @@
>  
> +  upgradeSchema2: function upgradeSchema2(objectStore) {
> +    if (!objectStore) {
> +      objectStore = aTransaction.objectStore(STORE_NAME);
> +    }

Under which circumstances would we enter this `if` block? Where's `aTransaction` coming from? Seems to me this block can just go away.

@@ +262,5 @@
> +    objectStore.openCursor().onsuccess = function(event) {
> +      let cursor = event.target.result;
> +      if (cursor) {
> +        if (DEBUG) debug("upgrade before: " + JSON.stringify(cursor.value));
> +        if (!cursor.value.messageClass) {

Two readability nits:

(1)

  if (!cursor) {
    return;
  }

(2) 

  let message = cursor.value;
  ... // use message instead of cursor.value

@@ +263,5 @@
> +      let cursor = event.target.result;
> +      if (cursor) {
> +        if (DEBUG) debug("upgrade before: " + JSON.stringify(cursor.value));
> +        if (!cursor.value.messageClass) {
> +          cursor.value.messageClass = "normal";

Please use MESSAGE_CLASS_NORMAL.

@@ +266,5 @@
> +        if (!cursor.value.messageClass) {
> +          cursor.value.messageClass = "normal";
> +        }
> +        if (!cursor.value.deliveryStatus) {
> +          cursor.value.deliveryStatus = "not-applicable";

Please use DELIVERY_STATUS_NOT_APPLICABLE.

Comment 6

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/edbe5d1301fc

Comment 7

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

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

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

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +168,5 @@
>            if (DEBUG) debug("New database");
>            self.createSchema(db);
>            break;
>  
>          case 1:

You'll need to handle the 1->2 upgrade as well as the 0->2...

@@ +256,5 @@
>    },
>  
> +  upgradeSchema2: function upgradeSchema2(objectStore) {
> +    if (!objectStore) {
> +      objectStore = aTransaction.objectStore(STORE_NAME);

This whole block looks wrong... You shouldn't ever get here because objectStore can't be null, but if it was, what's this 'aTransaction'?

@@ +258,5 @@
> +  upgradeSchema2: function upgradeSchema2(objectStore) {
> +    if (!objectStore) {
> +      objectStore = aTransaction.objectStore(STORE_NAME);
> +    }
> +    objectStore.openCursor().onsuccess = function(event) {

This is an async operation... It's fine, I think, because you should only be called from a versionchange transaction and that won't complete until all outstanding requests complete... But it's not obvious!

@@ +261,5 @@
> +    }
> +    objectStore.openCursor().onsuccess = function(event) {
> +      let cursor = event.target.result;
> +      if (cursor) {
> +        if (DEBUG) debug("upgrade before: " + JSON.stringify(cursor.value));

You probably don't want to do this for every message right? Even if this is a debug thing...

@@ +262,5 @@
> +    objectStore.openCursor().onsuccess = function(event) {
> +      let cursor = event.target.result;
> +      if (cursor) {
> +        if (DEBUG) debug("upgrade before: " + JSON.stringify(cursor.value));
> +        if (!cursor.value.messageClass) {

You shouldn't need the if() here and just below, right? I mean, who would have added those properties without running this code?

Comment 8

[Gregor Wagner [:gwagner]]

Attachment: patch

New version

Comment 9

[Gregor Wagner [:gwagner]]

Followup:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4d75397edde8

Comment 10

[Vicamo Yang [:vicamo][:vyang]]

Attachment: patch, no while loop

I think this is a little bit easier.

Comment 11

[Vicamo Yang [:vicamo][:vyang]]

(In reply to Gregor Wagner [:gwagner] from comment #9)
> Followup:
> https://hg.mozilla.org/integration/mozilla-inbound/rev/4d75397edde8

Oops, too late :(

Comment 12

[Gregor Wagner [:gwagner]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/46a6feeaf9c5

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/edbe5d1301fc
https://hg.mozilla.org/mozilla-central/rev/4d75397edde8

Comment 14

[Ryan VanderMeulen [:RyanVM]]

(In reply to Gregor Wagner [:gwagner] from comment #12)
> https://hg.mozilla.org/releases/mozilla-aurora/rev/46a6feeaf9c5

(I'm assuming this was all that was needed for Aurora landing)

Comment 15

[Gregor Wagner [:gwagner]]

(In reply to Ryan VanderMeulen from comment #14)
> (In reply to Gregor Wagner [:gwagner] from comment #12)
> > https://hg.mozilla.org/releases/mozilla-aurora/rev/46a6feeaf9c5
> 
> (I'm assuming this was all that was needed for Aurora landing)

Yes that's all. Thanks for checking!

Comment 17

[Armen [:armenzg]]

I unduped bug 807631 since it happened on the 10-24th stable build and it is about intermittent periods where I miss texts (even though I can still send them).