Closed Bug 795209 Opened 7 years ago Closed 7 years ago

System Message API: Fail to fire 'icc-stkcommand' system messages

Categories

(Core :: DOM: Device Interfaces, defect)

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla18
blocking-basecamp +

People

(Reporter: allstars.chh, Assigned: airpingu)

References

Details

Attachments

(2 files, 3 obsolete files)

The Patch for Bug 787175 added a system message 'icc-stkcommand',
but I found that sometimes this system message is sent too early before gaia is able to handle that.
So we need to tell ICC to send this proactive command later.
This is blocker because sometimes Gaia STK app cannot receive stk commands.
Assignee: nobody → allstars.chh
Blocks: b2g-stk
blocking-basecamp: --- → ?
(In reply to Yoshi Huang[:yoshi][:allstars.chh] from comment #0)
> The Patch for Bug 787175 added a system message 'icc-stkcommand',
> but I found that sometimes this system message is sent too early before gaia
> is able to handle that.

ORLY? We don't even turn on the radio until we receive 'system-message-listener-ready'. So how come we get proactive STK commands from the ICC already?

Please see also bug 783149.
Attached patch logSplinter Review
Hi, philikon
I attached the log.
Yes, the radio is not on yet,
but it's already in SIM-Ready state.

line 517: UNSOL_RIL_CONNECTED

line 565: Got first proactive command 

line 928: proactive command sent to content

line 942: Got system-message-listener-ready.
Oof. Maybe we need to re-think the 'system-message-listener-ready' approach then.

The buffering in bug 783149 should help us, though, no?
(In reply to Philipp von Weitershausen [:philikon] from comment #4)
> Oof. Maybe we need to re-think the 'system-message-listener-ready' approach
> then.
> 
> The buffering in bug 783149 should help us, though, no?
Hi, philikon

After discussed with Gene, when SystemMessageInternal received this message, the handlers for this message are still empty, might be this message is arrived too early. He will take a look into system message part.

Thanks
After some tests I found SIM app won't send the proactive command anymore, 
I think we should try to see if SystemMessageManager can broadcast this early message or not.

Reset assignee to default to let someone who knows system message better to handle this.

Thanks
Assignee: allstars.chh → nobody
Summary: B2G STK: Send icc-stkcommand after receiving system-message-listener-ready → B2G: Sometimes SystemMessageManager cannot receive 'icc-stkcommand' system message.
Gene says he will take this bug.
Assignee: nobody → clian
Need STK for v1.
blocking-basecamp: ? → +
This issue is basically affected by multiple causes. I'll upload a patch to address this later. ;)
Summary: B2G: Sometimes SystemMessageManager cannot receive 'icc-stkcommand' system message. → System Message API: Fail to fire 'icc-stkcommand' system messages
Attached patch Patch (obsolete) — Splinter Review
Hi Fabrice :)

This issue is due to the following causes:

1. The "icc-stkcommand" system message is fired super early when booting up, even before the webapps' registration process is done, thus having incorrect pages to be broadcasted. We need to buffer such "early-fired" messages and let SystemMessageInternal listen to the "webapps-registry-ready" observer event to know the right time to fire them.

2. Even if the #1 is fixed, the "icc-stkcommand" is still fired early than the homescreen activity message, thus occupying the home and disable the homescreen app. To fix this, we need to delay the 10 seconds to 20 seconds. Well, I know this is dirty but is fine because these messages don't care about the exact time they are going to fire (just "near" the booting-up time, though).
Attachment #666175 - Flags: review?(fabrice)
Comment on attachment 666175 [details] [diff] [review]
Patch

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

I'm not a big fan of buffering for 20s on the shell.js side. It looks like only the settings app is listening for the 'icc-stkcommand' message, so why do we need to wait that long?
What I don't understand in your comment is the "thus occupying the home and disable the homescreen app" part.

::: dom/messages/SystemMessageInternal.js
@@ +61,5 @@
> +                                   msg: aMessage,
> +                                   pageURI: aPageURI,
> +                                   manifestURI: aManifestURI });
> +      return;
> +    }

Nit: add a blank line

@@ +103,5 @@
> +      this._bufferedSysMsgs.push({ how: "broadcast",
> +                                   type: aType,
> +                                   msg: aMessage });
> +      return;
> +    }

Nit: add a blank line

@@ +223,5 @@
> +              this.broadcastMessage(aSysMsg.type, aSysMsg.msg);
> +              break;
> +          }
> +        }).bind(this));
> +        break;    

nit: whitespace
Attachment #666175 - Flags: review?(fabrice) → feedback+
Attached patch Patch, V1.1 (obsolete) — Splinter Review
Hi Fabrice,

I just did more tests and cannot reproduce the issue #2 at comment #10 anymore. I did at least 10 times and it always works well. However, it's very close to the homescreen app loaded (at 9 seconds). Do you think we need to make 10 seconds longer?

Btw, what I mentioned: "thus occupying the home and disable the homescreen app" means if an app is asked to be opened *earlier* then the homescreen app, then such the first application opened would be considered as the homescreen app [1]. This would mess up the homescreen.

[1] Please see https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/window_manager.js#L1078
Attachment #666175 - Attachment is obsolete: true
Attachment #666839 - Flags: review?(fabrice)
Attachment #666839 - Flags: feedback+
Attachment #666839 - Flags: review?(fabrice)
Attachment #666839 - Flags: review+
Attachment #666839 - Flags: feedback+
Attached patch Patch 1.2 (obsolete) — Splinter Review
Rebase. r=fabrice.
Attachment #666839 - Attachment is obsolete: true
Attachment #666844 - Flags: review+
Attached patch Patch 1.3Splinter Review
Clean up the buffer after sending the buffered system messages.

Rebase. r=fabrice.
Attachment #666844 - Attachment is obsolete: true
Attachment #666851 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/97362b531a38
Status: NEW → RESOLVED
Closed: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Depends on: 801573
You need to log in before you can comment on or make changes to this bug.