Closed Bug 876614 Opened 7 years ago Closed 7 years ago

[SMS] The thread_list is not updating automatically after receiving 2 class-0 messages

Categories

(Firefox OS Graveyard :: General, defect, P1)

ARM
Gonk (Firefox OS)

Tracking

(blocking-b2g:leo+, firefox22 wontfix, firefox23 wontfix, firefox24 fixed, b2g18 fixed, b2g18-v1.0.0 wontfix, b2g18-v1.0.1 wontfix, b2g-v1.1hd fixed)

RESOLVED FIXED
1.1 QE4 (15jul)
blocking-b2g leo+
Tracking Status
firefox22 --- wontfix
firefox23 --- wontfix
firefox24 --- fixed
b2g18 --- fixed
b2g18-v1.0.0 --- wontfix
b2g18-v1.0.1 --- wontfix
b2g-v1.1hd --- fixed

People

(Reporter: leo.bugzilla.gaia, Assigned: airpingu)

References

Details

(Whiteboard: [fixed-in-birch])

Attachments

(3 files, 3 obsolete files)

1. Title : The thread_list is not updating automatically after receiving 2 class-0 messages.
2. Precondition : SMS app should be working
3. Tester's Action : 1)Receive 2 class-0 messages 
                     2)Once ok button of the class-0 messages is clicked,receive   normal SMS.Verify the thread_list
                     3)After receiving two class-0 messages,try sending the message from the mobile.
4. Detailed Symptom (ENG.) : The thread_list is not updated for the 2) and 3) steps.After restarting the device the thread_list is updating.
5. Expected : The thread_list should be updated automatically without restarting the device in all the scenarios
6. Reproducibility: Y
	1) Frequency Rate : 100%
7.Gaia Master/v1-train : Reproduced on v1-train
8.Gaia Revision: 61d7ab244db3e2174b22bfa6de3e3d69136b4904
9.Personal email id:  sasikala.paruchuri@gmail.com
about 4> restarting the Sms app should be enough.

I think we currently have several problems with class-0 messages.

Could you tell us if you're using the commercial ril ? (I suspect you are)
blocking-b2g: --- → leo?
Hi Julienw,

1.We have to restart the SMS app to show the list of messages.
2.We are using commercial ril.

Discussing about this issue in Bug:858908.Asked to file a new bug.

Thanks,
Leo
blocking-b2g: leo? → leo+
Hi Leo,

The way of handling class-0 messages in SMS App is that
SMS App just shows the messages without saving the messages.
And the messages wouldn't be in the thread list.

After you clicked OK button,
what page did you stay to sent the message in SMS App?

Could you attach a screenshot for this?
And the better way is recording a video for the STR.

Thanks. :)
Flags: needinfo?(leo.bugzilla.gaia)
Assignee: nobody → evanxd
Attached video Video attached for STR
Hi,
Please find the attached video for STR
Precondition:The thread_list should be empty(No messages)
1.Receive 2 class-0 messages.
2.Press ok button of class-0 messages.
3.Send normal SMS.
4.Once the notification is received the message list should be shown in the thread_list.
5.The thread_list is shown as empty.
6.After restarting the device we can see the thread_list.
Flags: needinfo?(leo.bugzilla.gaia)
Evan, you may run following commands to reproduce this bug with emulator:

$ ./run-emulator.sh &
$ telnet localhost 5554
> sms pdu 00000191F10010001010000000000141
< OK
> sms pdu 00000191F10010001010000000000141
< OK
(click OK twice on Messages App)
> sms send 1234213 adsf
< OK
Hi Vicamo,

We could also use the Android App to send the class 0 message.
https://github.com/virtualabs/ZeroSMS
Hi all,

I'm working on other leo+ bugs.
Please take this bug if you're interesting in that.
Assignee: evanxd → nobody
Assignee: nobody → rexboy
Assignee: rexboy → johu
Sorry Rex, 

I didn't reload before assigning to myself. I already reassign to you.
Assignee: johu → rexboy
Attached file Test case
I think this may be a Gecko issue.
The problem is that if we call alert() inside a system message, and then system message is triggered twice (that means alert() is called again and |mozbrowsershowmodalprompt| event raised) during alert window is opening, things goes wrong:

1. If there are any queued setTimeout(), they are not executed.
2. Even after closing the two alert() dialogs, the first thread blocked by alert() doesn't continue executing. Only the second one continued.

What we encountered here is the first case. ThreadListUI wants to use setTimeout() to update the sms list[1], but the second alert() opened and the code inside setTimeout() never executed.

The attachment is a test case for this issue:
STR:
1. Apply the patch, then do $ make reset-gaia.
2. Open template App.
3. Wait one second, then close the two alert dialog shown.

Excepted:
"1, 2, timeout test," is shown

Actual:
"2," is shown


Although we have queued alert in system/js/modal_dialog.js so the two alert() shows correctly, but what we need here is the code after alert() being executed correctly. I think this need to be fixed from Gecko side.
I would put this bug to general and see if anyone can help.

[1]: https://github.com/mozilla-b2g/gaia/blob/master/apps/sms/js/thread_list_ui.js#L319
Assignee: rexboy → nobody
Component: Gaia::SMS → General
Blocks: b2g-sms
It seems the system message mechanism works well since it does run the message handler twice, which is expected.


Hi Rex,

What's happening if you run your .openAlert() twice consecutively from the content site without relying on the System Message to run it from the platform?
Flags: needinfo?(rexboy)
Hi Gene:
As we discussed earlier, if we don't use system message to execute .openAlert(), the main thread will be blocked by alert() so the second openAlert() won't be executed until user close alert dialog. Everything goes well then. This is different with cases sending 2 system messages, where the second alert() will be executed while the first alert dialog is still opening.
Flags: needinfo?(rexboy)
Maybe that means we should move away from using alert to using an internal modal dialog system ?

Although it seems wrong that timeouts are forgotten. How does gecko behave on Firefox Desktop in this situation ?
Attached patch WIP Patch (obsolete) — Splinter Review
Hi Fabrice,

Please see comment #11. Javascript has its own special event loop for handling alert(). To deal with that case in a more general way, I try to make the system message mechanism adapt to that.

This is just a WIP and not yet tested.
Assignee: nobody → gene.lian
Attachment #762590 - Flags: feedback?(fabrice)
Attached patch WIP Patch (obsolete) — Splinter Review
Hi Justin and Fabrice,

(Fabrice is taking PTOs and not available until 6/29, so maybe Justin is the right person to take this review)

This is a very tricky case and hard to explain. Please see bug 876614, comment #9. If the system message handler is calling alert() in it, the process won't be blocked and the SystemMessageManager can keep receiving and handling the next coming message. The root cause is the task of alert() will be queued by its own special event loop in Javascript.

To tackle this issue, I queue up all the coming messages if the handler for the last message is not yet returned. To do so, we will have some corresponding changes for "SMM:HandleMessagesDone" and "handle-system-messages-done", which you have been familiar with.

Note that "handle-system-messages-done" is going to be deprecated anyway.
Attachment #762590 - Attachment is obsolete: true
Attachment #762590 - Flags: feedback?(fabrice)
Attachment #764077 - Flags: feedback?(justin.lebar+bug)
Attachment #764077 - Flags: feedback?(fabrice)
jeez, I wish Gaia wouldn't abuse alert() for this sort of thing.  It's good enough for webpages, but it's clearly problematic if you call alert() from inside an alert().

Anyway, that's not your fault; I'll see if I can make sense of this patch.
># HG changeset patch
># User Gene Lian <clian@mozilla.com>
># Date 1371553912 -28800
># Node ID ea897cc43c041c7329279390868555942b841add
># Parent  d52191850fc61c132cbb13c64e1ff4e36d1e770d
>Bug 876614 - [SMS] The thread_list is not updating automatically after receiving 2 class-0 messages. r=jlebar,fabrice a=leo+

We like for commit messages to indicate what you're changing, which is often (as in this case) not the title of the related bug.

>diff --git a/dom/messages/SystemMessageManager.js b/dom/messages/SystemMessageManager.js
>--- a/dom/messages/SystemMessageManager.js
>+++ b/dom/messages/SystemMessageManager.js

>@@ -48,16 +48,23 @@ function SystemMessageManager() {
> SystemMessageManager.prototype = {
>   __proto__: DOMRequestIpcHelper.prototype,
> 
>   _dispatchMessage: function sysMessMgr_dispatchMessage(aType, aHandler, aMessage) {
>+    if (aHandler.isHandling) {
>+      aHandler.messages.push(aMessage);
>+      return;
>+    }

Please add a comment briefly explaining why we have to do worry about reentrancy here.

>@@ -67,18 +74,32 @@ SystemMessageManager.prototype = {
>       let wrapper = Cc[contractID].createInstance(Ci.nsISystemMessagesWrapper);
>       if (wrapper) {
>         aMessage = wrapper.wrapMessage(aMessage, this._window);
>         wrapped = true;
>         debug("wrapped = " + aMessage);
>       }
>     }
> 
>-    aHandler.handleMessage(wrapped ? aMessage
>+    aHandler.handler
>+            .handleMessage(wrapped ? aMessage
>                                    : ObjectWrapper.wrap(aMessage, this._window));
>+
>+    // We need to notify the parent one of the system messages has been handled,
>+    // so the parent can release the CPU wake lock it took on our behalf.
>+    cpmm.sendAsyncMessage("SystemMessageManager:HandleMessagesDone",
>+                          { type: aType,
>+                            manifest: this._manifest,
>+                            uri: this._uri,
>+                            handledCount: 1 });
>+
>+    aHandler.isHandling = false;
>+    if (aHandler.messages.length > 0) {
>+      this._dispatchMessage(aType, aHandler, aHandler.messages.shift());
>+    }
>   },

This sends one HandlMessageDone for each message.  But previously, we sent one
HandleMessageDone only after we were done sending all messages.  Surely if we
were to change this we'd have to also change something on the parent side?

>@@ -94,17 +115,17 @@ SystemMessageManager.prototype = {
>     if (!aHandler) {
>       // Setting the handler to null means we don't want to receive messages
>       // of this type anymore.
>       delete handlers[aType];
>       return;
>     }
> 
>     // Last registered handler wins.
>-    handlers[aType] = aHandler;
>+    handlers[aType] = { handler: aHandler, messages: [], isHandling: false };

It's pretty confusing that a variable named aHandler sometimes is the actual
handler, and sometimes is the object that has {handler, messages, isHandling}.
Can we rename something here?
Attachment #764077 - Flags: feedback?(justin.lebar+bug)
(In reply to Justin Lebar [:jlebar] from comment #16)
> This sends one HandlMessageDone for each message.  But previously, we sent
> one
> HandleMessageDone only after we were done sending all messages.  Surely if we
> were to change this we'd have to also change something on the parent side?

No, we don't need to change anything on the parent side. I use |handledCount: 1| to specify only one message is handled for each time (supposing we have more than 2 messages to be handled). In this way, the parent side will still keep the CPU wake lock because it only decreases one count when it receives |handledCount: 1|, which means the CPU wake lock won't be released until all the handled counts are collected.

I'd prefer keeping the handledCount mechanism because we can directly cancel all the handledCount at one time when the handler is not registered (i.e. |handledCount: messages.length|).

Does is sound reasonable to you?
Oh, I see!

Yes, that's great.  Sorry I missed that bit.
Blocks: system-message-api
No longer blocks: b2g-sms
Attached patch Patch (obsolete) — Splinter Review
Attachment #764077 - Attachment is obsolete: true
Attachment #764077 - Flags: feedback?(fabrice)
Attachment #765819 - Flags: review?(justin.lebar+bug)
Comment on attachment 765819 [details] [diff] [review]
Patch

Looks great to me; just some nits on the comments.

>diff --git a/dom/messages/SystemMessageManager.js b/dom/messages/SystemMessageManager.js
>--- a/dom/messages/SystemMessageManager.js
>+++ b/dom/messages/SystemMessageManager.js

>@@ -22,19 +22,19 @@ XPCOMUtils.defineLazyServiceGetter(this,
> 
> function debug(aMsg) {
>    // dump("-- SystemMessageManager " + Date.now() + " : " + aMsg + "\n");
> }
> 
> // Implementation of the DOM API for system messages
> 
> function SystemMessageManager() {
>-  // Message handlers for this page.
>-  // We can have only one handler per message type.
>-  this._handlers = {};
>+  // Contains {handler, messages, isHandling} per message type, where the
>+  // |handler| is the message handler registered for this page.
>+  this._dispatchers = {};

Maybe something like

    // If we have a system message handler registered for messages of type
    // |type|, this._dispatchers[type] equals {handler, messages, isHandling},
    // where
    //  - |handler| is the message handler that the page registered,
    //  - |messages| is a list of messages which we've received while
    //    dispatching messages to the handler, but haven't yet sent, and
    //  - |isHandling| indicates whether we're currently dispatching messages
    //    to this handler.

>@@ -47,17 +47,27 @@ function SystemMessageManager() {
> SystemMessageManager.prototype = {
>   __proto__: DOMRequestIpcHelper.prototype,
> 
>-  _dispatchMessage: function sysMessMgr_dispatchMessage(aType, aHandler, aMessage) {
>+  _dispatchMessage: function sysMessMgr_dispatchMessage(aType, aDispatcher, aMessage) {
>+    if (aDispatcher.isHandling) {
>+      // Queue up the coming messages if the handler for the last message isn't
>+      // returned yet to avoid race condition caused by alert() which has its
>+      // own special even loop and won't block the process receiving messages.
>+      aDispatcher.messages.push(aMessage);
>+      return;
>+    }

I wouldn't necessarily call this a race condition.  Can we use some form of the
word "re-entry"?  For example,

         // Queue up the incomming message if we're currently dispatching a
         // message; we'll send the message once we finish with the current one.
         //
         // _dispatchMethod is reentrant because a page can spin up a nested
         // event loop from within a system message handler (e.g. via alert()),
         // and we can then try to send the page another message while it's
         // inside this nested event loop.

>@@ -182,35 +206,38 @@ SystemMessageManager.prototype = {
>+    if (dispatcher) {
>       messages.forEach(function(aMsg) {
>-        this._dispatchMessage(msg.type, handler, aMsg);
>+        this._dispatchMessage(msg.type, dispatcher, aMsg);
>       }, this);
>+    } else {
>+      // We need to notify the parent the system messages have been handled,
>+      // even if there are no handlers registered for them, so the parent can
>+      // release the CPU wake lock it took on our behalf.

I missed the |handledCount: messages.length| part last time, so I think it
would be helpful if this comment said something like

        // We need to notify the parent that all the queued system messages
        // have been handled (notice |handledCount: messages.length|) ...
Attachment #765819 - Flags: review?(justin.lebar+bug) → review+
Attached patch Patch, V2Splinter Review
Update the patch based on Justin's comment. Thanks for the review!
Attachment #765819 - Attachment is obsolete: true
Attachment #766639 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/bd0fdb3585c6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Priority: -- → P1
Target Milestone: 1.1 QE3 (26jun) → 1.1 QE5
Target Milestone: 1.1 QE5 → 1.1 QE4 (15jul)
You need to log in before you can comment on or make changes to this bug.