Closed Bug 823010 Opened 9 years ago Closed 8 years ago

B2G SMS: We should not ack reception when there's a storage error

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
blocking-b2g tef+
Tracking Status
firefox18 --- wontfix
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- fixed

People

(Reporter: julienw, Assigned: philikon)

References

Details

(Keywords: late-l10n, Whiteboard: [triaged:1/17][uxwanted][eta:1/21])

Attachments

(2 files)

STR:
* fill your /data partition (push a big file (2395140 kbytes) and finish with |adb shell dd if=/dev/urandom of=/data/arandomfile|)

* receive a SMS (send from another device)
=> we get the sms notification
=> there is an error on the logcat [1] which is expected
=> the sms is not shown in the SMS app

* remove the smaller file (/data/arandomfile)
* reboot the phone to get back a sane state
=> the SMS is not received again

I think that most other phones knows when there are not enough storage and reject new receptions, so that the network can retry sending the SMS later.

I don't know if there is an error happening on the gecko side as well (I think we may have one that I don't see because I didn't activate the dump), and imho that's where we should handle this.


[1] E/GeckoConsole(  488): [JavaScript Error: "UnknownError" {file: "app://sms.gaiamobile.org/shared/js/async_storage.js" line: 90}]
Sounds a platform bug for me.
Component: Gaia::SMS → General
QA Contact: mbarone976
Since we poorly handle lack of disk space elsewhere and don't have enough time to fix all areas, let's not block on this one issue.

That being said ...

Vicamo, could this affect certification?
blocking-basecamp: ? → -
Flags: needinfo?(vyang)
(In reply to Andrew Overholt [:overholt] from comment #2)
> Since we poorly handle lack of disk space elsewhere and don't have enough
> time to fix all areas, let's not block on this one issue.
> 
> That being said ...
> 
> Vicamo, could this affect certification?

I remember we had some short discuss on this issue. Storage full event is usually for feature phones, not for smart phones. I guess each SMS message may only take ~1KB or slightly more, I don't have real number for this but I think that's a reasonable estimation. So, you have to send/receive at least 239514 messages to blow up /data. Even you have 1% space left for SMS, you'll still have to send/receive 2400 messages before having the problem.
Depends on: 739143
Flags: needinfo?(vyang)
A user can fill the storage in another way... he can put a lot of audio/video files on its device for example.

I don't know how Android handles this, but instinctively I'd say that they're not using the same partition for this so maybe they don't have this problem.

But we do.
When I put audio files on my music player, I usually fill it to a point where there is 0 bytes left; the last file is truncated.

Maybe the real problem is that we shouldn't let the user do this.
Component: General → DOM: Device Interfaces
Product: Boot2Gecko → Core
blocking-b2g: --- → tef?
Duplicate of this bug: 831079
OS: Linux → Gonk (Firefox OS)
(In reply to Julien Wajsberg [:julienw] from comment #4)
> A user can fill the storage in another way... he can put a lot of
> audio/video files on its device for example.

devicestorage uses a separate partition AFAIK, but built-in databases like SMS and Contacts will always fight with app packages, appcache, and app's indexedDB storage for space.
OS: Gonk (Firefox OS) → Linux
When the storage fails for due to QuotaExceededError / NS_ERROR_DOM_INDEXEDDB_QUOTA_ERR, or perhaps for *whatever* reason, we should acknowledge the SMS with PDU_FCS_MEMORY_CAPACITY_EXCEEDED instead of PDU_FCS_OK. So far so good.

But then what? Do we still want the SMS to propagate to content? I think we don't. Should we perhaps instead show a notification that we're out of space? I think so, but I don't know where we stand in terms of the deadlines and string freeze. The work required for adding such a notification would be small compared to the work required to acknowledge SMS differently depending on whether it was saved or not. Andrew, who can make this call?
Flags: needinfo?(overholt)
Assignee: nobody → philipp
Summary: We should not ack the SMS reception when we have an error (eg we can't save it because we have no storage left) → B2G SMS: We should not ack reception when there's a storage error
Attached patch v1Splinter Review
This introduces an optional callback attribute to the methods in nsIRilSmsDatabaseService so that the consumer (RadioInterfaceLayer) can find out about the success or failure of the I/O operations, notify the RIL worker, and then acknowledge the SMS accordingly. For now this is very simplistic, in that it uses the same error constants everywhere, does no other error handling, and does not (yet?) notify the user of the message.

However, it does fix the problem originally described in description 0: when you run out of disk space in the /data partition, we ACK the SMS in the right way, so that when space becomes available again, the SMS is eventually resent by the carrier and stored properly.
Attachment #703142 - Flags: review?(vyang)
(In reply to Philipp von Weitershausen [:philikon] from comment #8)
> When the storage fails for due to QuotaExceededError /
> NS_ERROR_DOM_INDEXEDDB_QUOTA_ERR, or perhaps for *whatever* reason, we
> should acknowledge the SMS with PDU_FCS_MEMORY_CAPACITY_EXCEEDED instead of
> PDU_FCS_OK. So far so good.
> 
> But then what? Do we still want the SMS to propagate to content? I think we
> don't. Should we perhaps instead show a notification that we're out of
> space? I think so, but I don't know where we stand in terms of the deadlines
> and string freeze. The work required for adding such a notification would be
> small compared to the work required to acknowledge SMS differently depending
> on whether it was saved or not. Andrew, who can make this call?

Maybe Josh?
Flags: needinfo?(overholt) → needinfo?(jcarpenter)
blocking-b2g: tef? → tef+
blocking-basecamp: - → ---
Whiteboard: [triaged:1/17]
Keywords: late-l10n
Whiteboard: [triaged:1/17] → [triaged:1/17][uxwanted]
Does bug 739143 still block this?
Flags: needinfo?(vyang)
(In reply to Andrew Overholt [:overholt] from comment #11)
> Does bug 739143 still block this?

Not really. AFAICT, bug 739143 is about SIM storage
No longer depends on: 739143
Flags: needinfo?(vyang)
Comment on attachment 703142 [details] [diff] [review]
v1

Let's have a review race!
Attachment #703142 - Flags: review?(ferjmoreno)
Attachment #703142 - Flags: review?(allstars.chh)
Whiteboard: [triaged:1/17][uxwanted] → [triaged:1/17][uxwanted][eta:1/21]
(In reply to Philipp von Weitershausen [:philikon] from comment #8)
> When the storage fails for due to QuotaExceededError /
> NS_ERROR_DOM_INDEXEDDB_QUOTA_ERR, or perhaps for *whatever* reason, we
> should acknowledge the SMS with PDU_FCS_MEMORY_CAPACITY_EXCEEDED instead of
> PDU_FCS_OK. So far so good.
> 
> But then what? Do we still want the SMS to propagate to content? I think we
> don't. Should we perhaps instead show a notification that we're out of
> space? I think so, but I don't know where we stand in terms of the deadlines
> and string freeze. The work required for adding such a notification would be
> small compared to the work required to acknowledge SMS differently depending
> on whether it was saved or not. Andrew, who can make this call?

Do we still need an answer to this or are we going to silently reject them?
Comment on attachment 703142 [details] [diff] [review]
v1

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

r- because of aDeliveryStatus in saveSendingMessage().

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +653,5 @@
> +        notifyResult(Cr.NS_OK);
> +      };
> +      txn.onabort = function onabort(event) {
> +        // TODO check event.target.errorCode and use appropriate
> +        //      error code.

Several places in this script have this. I've filed bug 832140 for it, so let's add the bug id in the comments.

@@ +771,5 @@
>        numberIndex:    [[sender, aDate], [receiver, aDate]],
>        readIndex:      [FILTER_READ_READ, aDate],
>  
>        delivery:       DELIVERY_SENDING,
> +      deliveryStatus: aDeliveryStatus,

there is no aDeliveryStatus defined in this function.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1364,5 @@
> +      this.worker.postMessage(message);
> +
> +      if (!success) {
> +        //XXX TODO return here and notify the message to content?
> +        // fire off a "could not store incoming SMS" notification?

Just don't. Network is supposed to re-send the message again.

@@ +1378,5 @@
> +          receiver: message.receiver || null,
> +          body: message.fullBody || null,
> +          messageClass: message.messageClass,
> +          timestamp: message.timestamp,
> +          read: false

Can we simply use the 'sms' arg here?

@@ +1380,5 @@
> +          messageClass: message.messageClass,
> +          timestamp: message.timestamp,
> +          read: false
> +      });
> +      Services.obs.notifyObservers(sms, kSmsReceivedObserverTopic, null);      

trailing ws

@@ +1442,5 @@
> +                                         receiver: options.sms.receiver,
> +                                         body: options.sms.body,
> +                                         messageClass: options.sms.messageClass,
> +                                         timestamp: options.sms.timestamp,
> +                                         read: true});

Same here.

@@ +2399,5 @@
>                         : RIL.GECKO_SMS_DELIVERY_STATUS_NOT_APPLICABLE;
> +    let id = gSmsDatabaseService.saveSendingMessage(number, message, deliveryStatus, timestamp,
> +                                                    function notifyResult(rv, sms) {
> +      //TODO bug NNN handle !Components.isSuccessCode(rv)
> +      let messageClass = RIL.GECKO_SMS_MESSAGE_CLASSES[RIL.PDU_DCS_MSG_CLASS_NORMAL];

You don't need messageClass any more.

::: dom/system/gonk/ril_worker.js
@@ +4592,5 @@
>    },
>  
>    /**
>     * Helper for processing SMS-DELIVER PDUs.
> +   * 

trailing ws

@@ +4600,5 @@
>     *
>     * @param length
>     *        Length of SMS string in the incoming parcel.
>     *
>     * @return A failure cause defined in 3GPP 23.040 clause 9.2.3.22.

Then the return value here is meaningless, isn't it?

@@ +4687,5 @@
>        this.sendDOMMessage(message);
> +
> +      // We will acknowledge receipt of the SMS after we try to store it
> +      // in the database.
> +      return message.result;

Actually, after scanning all the changes made in _processSmsDeliver(), all that you need is return PDU_FCS_RESERVED here. Then you don't have to modify RIL[UNSOLICITED_RESPONSE_NEW_SMS] neither.
Attachment #703142 - Flags: review?(vyang) → review-
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> > + //XXX TODO return here and notify the message to content?
> > + // fire off a "could not store incoming SMS" notification?
> 
> Just don't. Network is supposed to re-send the message again.

And how does network know we've got available space? That's why we need bug 739143. Without it, modem won't report space available back and we'll never receive SMS messages again.
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> > > + //XXX TODO return here and notify the message to content?
> > > + // fire off a "could not store incoming SMS" notification?
> > 
> > Just don't. Network is supposed to re-send the message again.
> 
> And how does network know we've got available space? That's why we need bug
> 739143. Without it, modem won't report space available back and we'll never
> receive SMS messages again.

Are you sure the network keeps such stateful information ?
I'd say it will eventually retry even without this, but with this it may try earlier. (but I'm just guessing here)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> Comment on attachment 703142 [details] [diff] [review]
-----------------------------------------------------------------
> ::: dom/sms/src/ril/SmsDatabaseService.js
> @@ +771,5 @@
> >        numberIndex:    [[sender, aDate], [receiver, aDate]],
> >        readIndex:      [FILTER_READ_READ, aDate],
> >  
> >        delivery:       DELIVERY_SENDING,
> > +      deliveryStatus: aDeliveryStatus,
> 
> there is no aDeliveryStatus defined in this function.

Just realized you want to fix possible inconsistent delivery statuses of message saved and message carried in broadcast event. But I still think we can save all the changes in _processSmsDeliver().
(In reply to Julien Wajsberg [:julienw] from comment #17)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> > (In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> > > > + //XXX TODO return here and notify the message to content?
> > > > + // fire off a "could not store incoming SMS" notification?
> > > 
> > > Just don't. Network is supposed to re-send the message again.
> > 
> > And how does network know we've got available space? That's why we need bug
> > 739143. Without it, modem won't report space available back and we'll never
> > receive SMS messages again.
> 
> Are you sure the network keeps such stateful information ?
> I'd say it will eventually retry even without this, but with this it may try
> earlier. (but I'm just guessing here)

6.6 of http://www.quintillion.co.jp/3GPP/Specs/23204-850.pdf

We indeed need to report space available.
Comment on attachment 703142 [details] [diff] [review]
v1

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

Just a few nits added to Vicamo's review

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +700,5 @@
>    /**
>     * nsIRilSmsDatabaseService API
>     */
>  
> +  saveReceivedMessage: function saveReceivedMessage(aSender, aBody, aMessageClass, aDate, aCallback) {

nit: lines shorter than 80 chars, please :)

@@ +743,3 @@
>    },
>  
> +  saveSendingMessage: function saveSendingMessage(aReceiver, aBody, aDeliveryStatus, aDate, aCallback) {

Ditto.

@@ +771,5 @@
>        numberIndex:    [[sender, aDate], [receiver, aDate]],
>        readIndex:      [FILTER_READ_READ, aDate],
>  
>        delivery:       DELIVERY_SENDING,
> +      deliveryStatus: aDeliveryStatus,

It's defined in line 745

@@ +784,3 @@
>    },
>  
> +  setMessageDelivery: function setMessageDelivery(messageId, delivery, deliveryStatus, aCallback) {

nit: Let's be consistent in the whole script: aMessageId, aDelivery...

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1365,5 @@
> +
> +      if (!success) {
> +        //XXX TODO return here and notify the message to content?
> +        // fire off a "could not store incoming SMS" notification?
> +        debug("Could store SMS " + message.id + ", error code " + rv);

s/Could/Could not/
Attachment #703142 - Flags: review?(ferjmoreno)
Attachment #703142 - Flags: review?(allstars.chh)
Attachment #703142 - Flags: review+
Attachment #703142 - Flags: review?(allstars.chh)
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #16)
> (In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> > > + //XXX TODO return here and notify the message to content?
> > > + // fire off a "could not store incoming SMS" notification?
> > 
> > Just don't. Network is supposed to re-send the message again.
> 
> And how does network know we've got available space? That's why we need bug
> 739143. Without it, modem won't report space available back and we'll never
> receive SMS messages again.

Yes we will. With the patch in this bug, I can fill up my phone's storage, send some text messages to it (they are not "delivered" to the SMS app); once I free some space again on the device and reboot, the texts are delivered again by the network (sometimes it takes a few minutes).
(In reply to Vicamo Yang [:vicamo][:vyang] from comment #15)
> > +        notifyResult(Cr.NS_OK);
> > +      };
> > +      txn.onabort = function onabort(event) {
> > +        // TODO check event.target.errorCode and use appropriate
> > +        //      error code.
> 
> Several places in this script have this. I've filed bug 832140 for it, so
> let's add the bug id in the comments.

Thanks. Will do.

> > +      this.worker.postMessage(message);
> > +
> > +      if (!success) {
> > +        //XXX TODO return here and notify the message to content?
> > +        // fire off a "could not store incoming SMS" notification?
> 
> Just don't. Network is supposed to re-send the message again.

Right, but how will the user know that they need to make some space on the device? :)

> > +          receiver: message.receiver || null,
> > +          body: message.fullBody || null,
> > +          messageClass: message.messageClass,
> > +          timestamp: message.timestamp,
> > +          read: false
> 
> Can we simply use the 'sms' arg here?

I'm not sure, actually. `sms` would be an XPCOM object, but the parameter sent through the system message API is presumably JSONified, or structured-cloned. Let's leave this for another patch in another bug.

> >        this.sendDOMMessage(message);
> > +
> > +      // We will acknowledge receipt of the SMS after we try to store it
> > +      // in the database.
> > +      return message.result;
> 
> Actually, after scanning all the changes made in _processSmsDeliver(), all
> that you need is return PDU_FCS_RESERVED here. Then you don't have to modify
> RIL[UNSOLICITED_RESPONSE_NEW_SMS] neither.

Good point! As discussed on IRC, we'll introduce a separate return value that means "don't ACK right away".

(In reply to Vicamo Yang [:vicamo][:vyang] from comment #18)
> > >        delivery:       DELIVERY_SENDING,
> > > +      deliveryStatus: aDeliveryStatus,
> > 
> > there is no aDeliveryStatus defined in this function.

It is, I added it. :)

> Just realized you want to fix possible inconsistent delivery statuses of
> message saved and message carried in broadcast event. But I still think we
> can save all the changes in _processSmsDeliver().

Yes, there was an inconsistency between _PENDING and _NOT_APPLICABLE, but that wasn't actually my concern. I just wanted to have *one* place where nsIDOMMozSmsMessage objects are instantiated.


Thanks for the r+, will address your comments in the morning.
Blocks: 830335
Depends on: 832140
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/c30676933621, perhaps prematurely but that's what you get when you land in a sea of other bustage, for https://tbpl.mozilla.org/php/getParsedLog.php?id=18945878&tree=Mozilla-Inbound
Where by "possibly prematurely" I of course mean "back yourself out of b2g18 for the same failure, it's not my responsibility" :)
Just getting caught up on this thread.

Is it possible to notify the user when lack of storage impedes the ability to receive an incoming SMS? eg:

* State: Winston's device storage is too low to store an incoming SMS message
* Joe sends Winston as SMS
* Winston's device receives ping from the network that there is a message
* Winston's device tell network "nuh uh, I'm full". Delivery fails. It seems like the patches above handle this and associated use cases. 
* Winston's device creates notification. eg: "Title: Message delivery failed. Body: Clear storage space to receive messages." (final copy TBD)

cc'ing Ayman for input here also.
Flags: needinfo?(jcarpenter) → needinfo?(aymanmaat)
(In reply to Josh Carpenter [:jcarpenter] from comment #27)
> Just getting caught up on this thread.
> 
> Is it possible to notify the user when lack of storage impedes the ability
> to receive an incoming SMS?

It is technically possible. It would mean introducing new strings though, which I am not sure if it is an option at this point.
(In reply to Josh Carpenter [:jcarpenter] from comment #27)
> Just getting caught up on this thread.
> 
> Is it possible to notify the user when lack of storage impedes the ability
> to receive an incoming SMS? eg:
> 
> * State: Winston's device storage is too low to store an incoming SMS message
> * Joe sends Winston as SMS
> * Winston's device receives ping from the network that there is a message
> * Winston's device tell network "nuh uh, I'm full". Delivery fails. It seems
> like the patches above handle this and associated use cases. 
> * Winston's device creates notification. eg: "Title: Message delivery
> failed. Body: Clear storage space to receive messages." (final copy TBD)
> 
> cc'ing Ayman for input here also.

I would also like to see an upfront notification when there is something like (e.g.) 5% memory left as a warning before memory actually runs out that would, therefore, occur before this scenario
Flags: needinfo?(aymanmaat)
Anshul, does having a UI like is being discussed in comment 27 and comment 29 affect bug 808607 or is the proposed patch sufficient?
Flags: needinfo?(anshulj)
Flags: needinfo?(alhadp)
(In reply to Andrew Overholt [:overholt] from comment #30)
> Anshul, does having a UI like is being discussed in comment 27 and comment
> 29 affect bug 808607 or is the proposed patch sufficient?

The proposed UI changes are not required for 808607. Our purpose was mainly that a "memory full" error code gets sent back to network when SMS cannot be stored, and that is being addressed by proposed patch.
Flags: needinfo?(anshulj)
Flags: needinfo?(alhadp)
No longer depends on: 832140
Duplicate of this bug: 830335
Attached patch fix testsSplinter Review
The original patch in this bug fixes a bunch of inconsistencies between the SMS object we notify when we send or receive a text, and the SMS objects we retrieve from the database (because we now have the database generate these objects always). This updates the tests accordingly.
Attachment #705115 - Flags: review?(jgriffin)
Comment on attachment 705115 [details] [diff] [review]
fix tests

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

lgtm
Attachment #705115 - Flags: review?(jgriffin) → review+
Comment on attachment 703142 [details] [diff] [review]
v1

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

::: dom/sms/src/ril/SmsDatabaseService.js
@@ +639,5 @@
> +    function notifyResult(rv) {
> +      if (!callback) {
> +        return;
> +      }
> +      let sms = self.createMessageFromRecord(message);

I'm pretty sure I am missing something. But where is the implementation of createMessageFromRecord?
(In reply to Alhad Purnapatre from comment #36)
> ::: dom/sms/src/ril/SmsDatabaseService.js
> @@ +639,5 @@
> > +    function notifyResult(rv) {
> > +      if (!callback) {
> > +        return;
> > +      }
> > +      let sms = self.createMessageFromRecord(message);
> 
> I'm pretty sure I am missing something. But where is the implementation of
> createMessageFromRecord?

https://mxr.mozilla.org/mozilla-central/source/dom/sms/src/ril/SmsDatabaseService.js#416
(In reply to Philipp von Weitershausen [:philikon] from comment #37)
> (In reply to Alhad Purnapatre from comment #36)
> > ::: dom/sms/src/ril/SmsDatabaseService.js
> > @@ +639,5 @@
> > > +    function notifyResult(rv) {
> > > +      if (!callback) {
> > > +        return;
> > > +      }
> > > +      let sms = self.createMessageFromRecord(message);
> > 
> > I'm pretty sure I am missing something. But where is the implementation of
> > createMessageFromRecord?
> 
> https://mxr.mozilla.org/mozilla-central/source/dom/sms/src/ril/
> SmsDatabaseService.js#416

Oh, sorry. I applied the patch on a slightly older version of b2g18, due to which I was seeing my problems. Thanks!
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a0484e3d8791 - I'd recommend getting rwood to review your test changes, so he won't rot you like he did with https://hg.mozilla.org/integration/mozilla-inbound/rev/1aeb49c8f4d9
https://hg.mozilla.org/mozilla-central/rev/eaa2f811232a
https://hg.mozilla.org/mozilla-central/rev/00f75c7fb670
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Duplicate of this bug: 805799
(In reply to ayman maat :maat from comment #29)
> (In reply to Josh Carpenter [:jcarpenter] from comment #27)
> > Just getting caught up on this thread.
> > 
> > Is it possible to notify the user when lack of storage impedes the ability
> > to receive an incoming SMS? eg:
> > 
> > * State: Winston's device storage is too low to store an incoming SMS message
> > * Joe sends Winston as SMS
> > * Winston's device receives ping from the network that there is a message
> > * Winston's device tell network "nuh uh, I'm full". Delivery fails. It seems
> > like the patches above handle this and associated use cases. 
> > * Winston's device creates notification. eg: "Title: Message delivery
> > failed. Body: Clear storage space to receive messages." (final copy TBD)
> > 
> > cc'ing Ayman for input here also.
> 
> I would also like to see an upfront notification when there is something
> like (e.g.) 5% memory left as a warning before memory actually runs out that
> would, therefore, occur before this scenario

Can we get a followup bug filed for these UI changes?
Landed on mozilla-b2g18/gaia master prior to the 1/25 branching to mozilla-b2g18_v1_0_0/v1.0.0, updating status-b2g-v1.0.0 to fixed.
Duplicate of this bug: 822207
You need to log in before you can comment on or make changes to this bug.