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

Status: RESOLVED FIXED

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

Description

[Leo]

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:  [e-mail]

Comment 1

[Julien Wajsberg [:julienw]]

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)

Comment 2

[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

Comment 3

[Evan Tseng [:evanxd]]

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

Comment 4

[Leo]

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

Comment 5

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

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

Comment 6

[Evan Tseng [:evanxd]]

Hi Vicamo,

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

Comment 7

[Evan Tseng [:evanxd]]

Hi all,

I'm working on other leo+ bugs.
Please take this bug if you're interesting in that.

Comment 8

[John Hu [:johnhu][:johu][:醬糊小弟]]

Sorry Rex, 

I didn't reload before assigning to myself. I already reassign to you.

Comment 9

[KM Lee [:rexboy] (inactivate after Sep. 22)]

Attachment: 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

Comment 10

[Gene Lian (I already quit Mozilla)]

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?

Comment 11

[KM Lee [:rexboy] (inactivate after Sep. 22)]

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.

Comment 12

[Julien Wajsberg [:julienw]]

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 ?

Comment 13

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch

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.

Comment 14

[Gene Lian (I already quit Mozilla)]

Attachment: WIP Patch

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.

Comment 15

[Justin Lebar (not reading bugmail)]

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.

Comment 16

[Justin Lebar (not reading bugmail)]

># HG changeset patch
># User Gene Lian <[e-mail]>
># 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?

Comment 17

[Gene Lian (I already quit Mozilla)]

(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?

Comment 18

[Justin Lebar (not reading bugmail)]

Oh, I see!

Yes, that's great.  Sorry I missed that bit.

Comment 19

[Gene Lian (I already quit Mozilla)]

Attachment: Patch

Comment 20

[Justin Lebar (not reading bugmail)]

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

Comment 21

[Gene Lian (I already quit Mozilla)]

Attachment: Patch, V2

Update the patch based on Justin's comment. Thanks for the review!

Comment 22

[Gene Lian (I already quit Mozilla)]

https://hg.mozilla.org/projects/birch/rev/bd0fdb3585c6
https://hg.mozilla.org/releases/mozilla-b2g18/rev/62049a1c5d36

Comment 23

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bd0fdb3585c6

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-b2g18_v1_1_0_hd/rev/62049a1c5d36