Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Description • 11 years ago

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.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 1 • 11 years ago

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

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 2 • 11 years ago

No permission to set to 1.4+ by myself.

	Ken Chang[:kenkai]
	Comment 3 • 11 years ago

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

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 5 • 11 years ago

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)

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 6 • 11 years ago

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

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 7 • 11 years ago

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.

	Vicamo Yang [:vicamo][:vyang]
	Comment 8 • 11 years ago

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 ...

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 9 • 11 years ago

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

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 10 • 11 years ago

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.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 11 • 11 years ago

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.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 12 • 11 years ago

Add Test case to verify Storage Full Error Code.

	Vicamo Yang [:vicamo][:vyang]
	Comment 13 • 11 years ago

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)|.

	Vicamo Yang [:vicamo][:vyang]
	Comment 14 • 11 years ago

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: "+1234567890",
> +    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)

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 15 • 11 years ago

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

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

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 16 • 11 years ago

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

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 17 • 11 years ago

Update test case.

	Vicamo Yang [:vicamo][:vyang]
	Comment 18 • 11 years ago

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

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 19 • 11 years ago

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

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

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 20 • 11 years ago

Revise the nits mentioned in comment 18.

	Bevis Tseng[:bevis][:btseng](Exited) Assignee
	Comment 21 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 11 years ago

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

	Ryan VanderMeulen [:RyanVM]
	Comment 23 • 11 years ago

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