974820 - B2G SMS & MMS: Support Error Handling of Sending/Receiving Message while device storage is full.

Status: RESOLVED FIXED

(Firefox OS Graveyard :: RIL, defect)

Description

[Bevis Tseng[:bevis][:btseng](Exited)]

Fire this bug to
1. provide correct error cause after access MobileMessageDB when device storage is full.
2. Notify storage full error to the caller to display correct error dialog.
3. Prevent sending SMS if saving sending SMS is failed and report error accordingly.

Comment 1

[Bevis Tseng[:bevis][:btseng](Exited)]

To replace 1.4+ bug 832140 to this one due to solution change.

Comment 2

[Bevis Tseng[:bevis][:btseng](Exited)]

No permission to set to 1.4+ by myself.

Comment 3

[Ken Chang[:kenkai]]

Let's use the target milestone to track this feature.

Comment 4

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 1: Add new error type STORAGE_FULL_ERROR. r=vyang

Comment 5

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2: Add error handling of sending/receiving SMS/MMS when device storage is full. r=vyang

Summary of the patch:
1. The main purpose is to add error handling of sending/receiving message while device storage is full.
2. Hence, instead of checking error code after digging deeply into indexedDB, the solution is to check DiskSpaceWatcher.isDiskFull at the beginning before any |Write| access to the MobileMessageDatabase and notify STORAGE_FULL_ERROR to the caller.

Verify result:
1. Sending SMS/MMS (OK)
2. Receiving SMS (OK)
3. Auto-Downloading MMS (OK)
4. Manual-Downloading MMS (OK)
5. Delete Message (OK)
6. Mark As Read (OK)
7. Update Delivery/Read report (OK)

Comment 6

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2: Add error handling of sending/receiving SMS/MMS when device storage is full. r=vyang

Update patch part 2 to refine the method of MobileMessageDatabaseService::_ensureWriteAccess(), because, so far, there is only one clear reason that prevents DB writting.

Comment 7

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8379531 [details] [diff] [review]
Patch Part 2: Add error handling of sending/receiving SMS/MMS when device storage is full. r=vyang

Update patch part 2 to refine the method of MobileMessageDatabaseService::_ensureWriteAccess(), because, so far, there is only one clear reason that prevents DB writting.

Comment 8

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

Comment on attachment 8379531 [details] [diff] [review]
Patch Part 2: Add error handling of sending/receiving SMS/MMS when device storage is full. r=vyang

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

::: dom/mobilemessage/src/gonk/MmsService.js
@@ +2369,5 @@
>              // likely due to a full disk.
> +            if (DEBUG) debug("Could not store MMS, error code " + rv);
> +            aRequest.notifyGetMessageFailed((rv == Cr.NS_ERROR_FILE_NO_DEVICE_SPACE) ?
> +                                              Ci.nsIMobileMessageCallback.STORAGE_FULL_ERROR :
> +                                              Ci.nsIMobileMessageCallback.INTERNAL_ERROR);

nit: just have another temporarily variable for that error value.

::: dom/mobilemessage/src/gonk/MobileMessageDatabaseService.js
@@ +58,5 @@
>  
>    saveReceivedMessage: function(aMessage, aCallback) {
> +    if (this._ensureWriteAccess({callback: aCallback})) {
> +      this.mmdb.saveReceivedMessage(aMessage, aCallback);
> +    }

Don't really like this.  See below.

@@ +132,5 @@
> +
> +  /**
> +   * Internal methods
> +   */
> +  _ensureWriteAccess: function(aResponses) {

We can't test MobileMessageDatabaseService, so this has to be moved into MobileMessageDB.jsm.

  MobileMessageDB.prototype = {
    // |isDiskFull| is supposed to be optionally hooked up with a function
    // that returns a boolean value.
    isDiskFull: null,

    newTxn: function(...) {
      ...
      this.ensureDB(function(...) {
        ...
        if (txn_type === READ_WRITE &&
            this.isDiskFull && !this.isDiskFull()) {
          callback(<error>);
          return;
        }

        let txn = ...
      });
    },
  };

Then we can have test cases for disk full events.  So, you know that ...

Comment 9

[Bevis Tseng[:bevis][:btseng](Exited)]

Thanks for better suggestion.
I'll revise it and add the corresponding test case.

Comment 10

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2_v2: Add error handling of sending/receiving SMS/MMS when device storage is full. r=vyang

NOTE: Please see https://wiki.mozilla.org/Release_Management/B2G_Landing to better understand the B2G approval process and landings.

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #):
[User impact] if declined:
[Testing completed]:
[Risk to taking this patch] (and alternatives if risky):
[String changes made]:

Add Error handling in MobileMessageDB.jsm instead of MobileMessageDatabaseService.js suggested in comment#8.

Comment 11

[Bevis Tseng[:bevis][:btseng](Exited)]

Comment on attachment 8381962 [details] [diff] [review]
Patch Part 2_v2: Add error handling of sending/receiving SMS/MMS when device storage is full. r=vyang

Add Error handling in MobileMessageDB.jsm instead of MobileMessageDatabaseService.js suggested in comment#8.

Comment 12

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3: Test case to verify Storage Full Error Code. r=vyang

Add Test case to verify Storage Full Error Code.

Comment 13

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

Comment on attachment 8381962 [details] [diff] [review]
Patch Part 2_v2: Add error handling of sending/receiving SMS/MMS when device storage is full. r=vyang

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

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +63,5 @@
>  
> +const DB_ON_UNEXPECTED_DB_VERSION_ERROR = -1;
> +const DB_ON_OPENING_ERROR = -2;
> +const DB_ON_BLOCK_OPENING_ERROR = -3;
> +const DB_ON_STORAGE_FULL_ERROR = -4;

The first three are not really referenced somewhere other than the lines throw them.  Please just use |Cr.NS_ERROR_FAILURE|.  For the last one, just use |Cr.NS_ERROR_FILE_NO_DEVICE_SPACE|.

@@ +75,5 @@
>                                     "nsIMmsService");
>  
> +XPCOMUtils.defineLazyServiceGetter(this, "gDiskSpaceWatcher",
> +                                   "@mozilla.org/toolkit/disk-space-watcher;1",
> +                                   "nsIDiskSpaceWatcher");

Please don't install more service getter here.  Instead, move it to MobileMessageDatabaseService.  The more service getters in this jsm, the higher reliance on a complete runtime environment.  That completely disobey the original purpose of this jsm file -- extract core parts of the mmdb logic to an independent jsm to enable us to manipulate the database at will.

@@ +108,5 @@
> +   * @return true if full.
> +   */
> +  isDiskFull: function() {
> +    return gDiskSpaceWatcher.isDiskFull;
> +  },

Just leave a null pointer here will do.

@@ +347,5 @@
>      let self = this;
>      this.newTxn(READ_ONLY, function(error, txn, messageStore){
>        if (error) {
>          if (aCallback) {
> +          aCallback(self.translateDBErrorToComponentResult(error));

Don't need this.

@@ +472,5 @@
>    /**
> +   * Helper to translate internally defined DB errors to
> +   * the values defined in Components.results.
> +   */
> +  translateDBErrorToComponentResult: function(aError) {

ditto.

@@ +1695,5 @@
>          aCallback.notify(aRv, domMessage);
>        };
>  
>        if (aError) {
> +        notifyResult(self.translateDBErrorToComponentResult(aError), null);

ditto

@@ +1727,5 @@
>          aCallback.notify(aRv, domMessage);
>        };
>  
>        if (error) {
> +        notifyResult(self.translateDBErrorToComponentResult(error), null);

ditto

@@ +2371,5 @@
>      let self = this;
>      this.newTxn(READ_ONLY, function(error, txn, messageStore) {
>        if (error) {
>          if (DEBUG) debug(error);
> +        aCallback.notify(self.translateDBErrorToMessageCallbackError(error), null, null);

aCallback.notify(error, null, null);

@@ +2407,5 @@
>      let self = this;
>      this.newTxn(READ_ONLY, function(error, txn, messageStore) {
>        if (error) {
>          if (DEBUG) debug(error);
> +        aCallback.notify(self.translateDBErrorToMessageCallbackError(error), null, null);

All the |aCallback.notify| calls in this function should be |aCallback.notify(Cr.BLAH_BLAH_BLAH, null, null)|.

Comment 14

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

Comment on attachment 8381964 [details] [diff] [review]
Patch Part 3: Test case to verify Storage Full Error Code. r=vyang

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

::: dom/mobilemessage/tests/marionette/test_mmdb_full_storage.js
@@ +12,5 @@
> +    receiver: "[phone]",
> +    body: "quick fox jump over the lazy dog",
> +    deliveryStatusRequested: false,
> +    timestamp: Date.now(),
> +    iccId: "1029384756"

2 SP indentation.

@@ +20,5 @@
> +  log("validateDiskSpaceWatcher()");
> +  // We expect to get a boolean value of 'false' if DiskspaceWatcher is available.
> +  ok(gMmdb.isDiskFull() === false, "validating DiskspaceWatcher.isDiskFull!");
> +
> +  return Promise.resolve();

That's not some async process.  You don't need a promise here.  Actually, we don't have initial |gMmdb.isDiskFull|.  It must be hooked manually.

@@ +31,5 @@
> +  };
> +
> +  ok(gMmdb.isDiskFull(), "replace gMmdb.isDiskFull");
> +
> +  return Promise.resolve();

ditto.

@@ +92,5 @@
> +  return deferred.promise;
> +}
> +
> +startTestBase(function testCaseMain() {
> +  gMmdb = newMobileMessageDB();

gMmdb.isDiskFull = function() {
  return gIsDiskFull;
};

@@ +95,5 @@
> +startTestBase(function testCaseMain() {
> +  gMmdb = newMobileMessageDB();
> +  return initMobileMessageDB(gMmdb, "test_gMmdb_full_storage", 0)
> +         .then(validateDiskSpaceWatcher)
> +         .then(replaceIsDiskFull)

.then(testGetMessage)
.then(function() {
  gIsDiskFull = true;
})
.then(testGetMessage)

Comment 15

[Bevis Tseng[:bevis][:btseng](Exited)]

Thanks! I'll revice it again to meet the design.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #13)

Comment 16

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2_v3: Add error handling of sending/receiving SMS/MMS when device storage is full. r=vyang

Refine ErrorCode Mapping of Component.results to the error code in nsIMobileMessageCallback.

Comment 17

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 3_v2: Test case to verify Storage Full Error Code. r=vyang

Update test case.

Comment 18

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

Comment on attachment 8382908 [details] [diff] [review]
Patch Part 2_v3: Add error handling of sending/receiving SMS/MMS when device storage is full. r=vyang

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

Thank you!

::: dom/mobilemessage/src/gonk/MobileMessageDB.jsm
@@ +93,5 @@
>     */
>    lastMessageId: 0,
>  
>    /**
> +   * A Hook to check if device storage is full.

nit: it's an optional hook.

@@ +2356,1 @@
>                           messageRecord, null);

nit: please also remove that extra line break. We no longer have to wrap the line.

@@ +2404,1 @@
>                           messageRecord, domMessage);

ditto.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +3938,5 @@
>  
>      let notifyResult = (function notifyResult(rv, domMessage) {
> +      if (!Components.isSuccessCode(rv)) {
> +        if (DEBUG) this.debug("Error! Fail to save sending message! rv = " + rv);
> +        request.notifySendMessageFailed(gMobileMessageDatabaseService.translateCrErrorToMessageCallbackError(rv));

nit: line wrap

Comment 19

[Bevis Tseng[:bevis][:btseng](Exited)]

Thanks! I'll revise this nits again before landing it.

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #18)

Comment 20

[Bevis Tseng[:bevis][:btseng](Exited)]

Attachment: Patch Part 2_v4: Add error handling of sending/receiving SMS/MMS when device storage is full. r=vyang

Revise the nits mentioned in comment 18.

Comment 21

[Bevis Tseng[:bevis][:btseng](Exited)]

Try server result is green:
https://tbpl.mozilla.org/?tree=Try&rev=7bb125589223

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/784079020455
https://hg.mozilla.org/integration/b2g-inbound/rev/514dac4cd4ce
https://hg.mozilla.org/integration/b2g-inbound/rev/abb348bb281d

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/784079020455
https://hg.mozilla.org/mozilla-central/rev/514dac4cd4ce
https://hg.mozilla.org/mozilla-central/rev/abb348bb281d