899885 - B2G RIL: callback-based worker messaging

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

RadioInterface communicates with worker by |this.worker.postMessage()| and |this.worker.onmessage()|.  The request made and response got cannot be paired because that's a stateless process.  The "sendSMS" request uses so called "envelope" to keep track of the sending transactions and the corresponding responses, while other DOMRequest based ones use |gMessageManager.saveRequestTarget()| and |gMessageManager.sendRequestResults()| for such mapping.  We'd like a common way for all these.

Comment 1

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Sounds an interesting idea!

Comment 2

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

Attachment: patch 1/2

1) Add a WorkerMessenger to handle communication between underlying worker instance.  Transfer worker ownership, instantiation to it as well.
2) Add a convenient function |sendWithIPCMessage| to replace |gMessageManager.saveRequestTarget| and |gMessageManager.sendRequestResults|.
3) Rename |RadioInterface.onmessage| to |handleUnsolicitedWorkerMessage| because we will only have to take special care of unsolicited messages.

Comment 3

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

Attachment: patch 2.1/2

This is the first break-down part of original part 2 patch.

This patch converts send transactions that don't need callbacks.  Also remove |options| argument from |Buf.newParcel()| in |stkHandleCallSetup()| in ril_worker for it never callbacks.

Comment 4

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

Attachment: patch 2.2/2

This is the second break-down part of original part 2 patch.

This patch converts simple IPC transactions by replacing them with |WorkerMessenger.sendWithIPCMessage|.

From now on, we should not manually assign |options.rilMessageType| in ril_worker because it's never referenced again in RadioInterface.  The thing really weights, is |options.rilMessageToken|.  Remove redundant assignments accordingly.

Comment 5

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

Attachment: patch 2.3/2

This is the third break-down part of original part 2 patch.

This patch converts complex "setPreferredNetworkType" transaction.  Use |this.sendChromeMessage(options)| in ril_worker so that we carry |options.rilMessageToken| back.

Comment 6

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

Attachment: patch 2.4/2

This is the fourth break-down part of original part 2 patch.  This patch converts complex "setCellBroadcastSearchList" transaction.

Comment 7

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

Attachment: patch 2.5/2

This is the fifth break-down part of original part 2 patch.  This patch converts complex "enumerateCalls" transaction.

Comment 8

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

Attachment: patch 2.6/2

This is the sixth break-down part of original part 2 patch.

This patch converts complex "sendMMI", "setCLIR", "setCallForward" transactions.  All callback will be processed by the original initiator of the transaction, so there is no more things like setting |options.rilMessageType| during the process and hand over control to another transaction initiator.  That is, transaction invoked by |RadioInterface.sendMMI| will be handled inside |RadioInterface.sendMMI|, and we don't bother checking whether the response in handleSetCallForward or handleSetCLIR is from a previous "sendMMI" transaction.

Comment 9

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

Attachment: patch 2.7/2

This is the seventh break-down part of original part 2 patch.  This patch converts complex "cancelUSSD" transaction.

Maybe we should just merge "RIL:CancelMMI:Return:OK" and "RIL:CancelMMI:Return:KO" into "RIL:CancelMMI" for simplicity.

Comment 10

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

Attachment: patch 2.8/2

This is the eighth break-down part of original part 2 patch.  This patch converts complex "queryCallForwardStatus" transaction.

Comment 11

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

Attachment: patch 2.9/2

This is the last break-down part of original part 2 patch.

This patch converts complex "sendSMS" transaction.  We don't separate sendSMS response into "sms-sent", "sms-delivery", and "sms-send-failed" any more.  Instead, we reuse the transaction callback by returning true.

Comment 12

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

Some day, bug 815526, we should remove |WorkerMessenger.sendWithIPCMessage| as well.  Before that, we need an new interface API in nsIRadioInterface that allows direct communication between RIL services and the underlying ChromeWorker.  Something like:

  void sendWorkerMessage(in DOMString      type,
                         in jsval          message,
              [optional] in nsIFooCallback callback);

Comment 13

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

Block bug 864485 because we need above API for newly created TelephonyService.

Comment 14

[Hsin-Yi Tsai (she/her) [:hsinyi]]

Vicamo, sorry to let you know that I might have no much bandwidth for the review, which is in lower priority of my list this week. If you feel you would like to land it quickly, feel free to ask for other peers' review. Thank you.

Comment 15

[Gene Lian (I already quit Mozilla)]

Will take my review tomorrow.

Comment 16

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 784236 [details] [diff] [review]
patch 1/2

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

::: dom/system/gonk/RadioInterfaceLayer.js
@@ -534,5 @@
>          data: data
>        });
> -    },
> -
> -    saveRequestTarget: function saveRequestTarget(msg) {

This function is called by many functions,
but it seems you didn't remove the caller part?

@@ +655,5 @@
> +      return;
> +    }
> +
> +    let keep = false;
> +    try { keep = callback(message); } catch(e) {}

What's the try-catch for?

@@ +664,5 @@
> +
> +  send: function send(type, message, callback) {
> +    if (!message) {
> +      message = {};
> +    }

message = message || {};

@@ +668,5 @@
> +    }
> +
> +    if (callback) {
> +      message.rilMessageToken = this.token;
> +      this.tokenCallbackMap[message.rilMessageToken] = callback;

Do you think we should add a check for typeof function here?

@@ +2567,5 @@
>        byType: {},
>        byAPN: {},
>      };
>    },
>  

we don't plan to add wm into prototype?

Comment 17

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 784236 [details] [diff] [review]
patch 1/2

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

The patch looks okay to me,
but there are some questions and it would helpful to get your response here.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +655,5 @@
> +      return;
> +    }
> +
> +    let keep = false;
> +    try { keep = callback(message); } catch(e) {}

is it okay we didn't do anything in catch clause?

@@ +661,5 @@
> +      delete this.tokenCallbackMap[message.rilMessageToken];
> +    }
> +  },
> +
> +  send: function send(type, message, callback) {

Can we rename send to some better understanding name? like 
sendWorkerMessage, ..etc.

s/type/rilMessageType/

Also it would be helpful to add comments.

@@ +677,5 @@
> +    this.worker.postMessage(message);
> +  },
> +
> +  // TODO: Bug 815526 - deprecate RILContentHelper.
> +  sendWithIPCMessage: function sendWithIPCMessage(msg, workerType, ipcType) {

s/msg/message/, as in the previous function you call it message.
s/workerType/rilMessageType/, I think this helps for understanding what this parameter stands for.

@@ +680,5 @@
> +  // TODO: Bug 815526 - deprecate RILContentHelper.
> +  sendWithIPCMessage: function sendWithIPCMessage(msg, workerType, ipcType) {
> +    this.send(workerType, msg.json.data, function(response) {
> +      msg.target.sendAsyncMessage(ipcType, response);
> +      return true;

Can you explain why returns true here?

Comment 18

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

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #16)
> Review of attachment 784236 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ -534,5 @@
> >          data: data
> >        });
> > -    },
> > -
> > -    saveRequestTarget: function saveRequestTarget(msg) {
> 
> This function is called by many functions,
> but it seems you didn't remove the caller part?

Yes, they're removed in following parts.

> @@ +655,5 @@
> > +      return;
> > +    }
> > +
> > +    let keep = false;
> > +    try { keep = callback(message); } catch(e) {}
> 
> What's the try-catch for?

To prevent unexpected errors thrown in that callback function.  Since we have no information about the failure and don't know what to do to recover, just ignore it in WorkerMessenger.

> @@ +668,5 @@
> > +    }
> > +
> > +    if (callback) {
> > +      message.rilMessageToken = this.token;
> > +      this.tokenCallbackMap[message.rilMessageToken] = callback;
> 
> Do you think we should add a check for typeof function here?

No.  If somebody writes that line, he can't really have a callback called and it's easy enough to debug that error.  Such code should even never ever be committed.

> @@ +2567,5 @@
> >        byType: {},
> >        byAPN: {},
> >      };
> >    },
> >  
> 
> we don't plan to add wm into prototype?

It's a private member and never exposed outside.

Comment 19

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

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #17)
> Review of attachment 784236 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/system/gonk/RadioInterfaceLayer.js
> @@ +661,5 @@
> > +      delete this.tokenCallbackMap[message.rilMessageToken];
> > +    }
> > +  },
> > +
> > +  send: function send(type, message, callback) {
> 
> Can we rename send to some better understanding name? like 
> sendWorkerMessage, ..etc.

The class is named "WorkerMessenger" already.

> s/type/rilMessageType/
> 
> Also it would be helpful to add comments.

Sure.

> @@ +680,5 @@
> > +  // TODO: Bug 815526 - deprecate RILContentHelper.
> > +  sendWithIPCMessage: function sendWithIPCMessage(msg, workerType, ipcType) {
> > +    this.send(workerType, msg.json.data, function(response) {
> > +      msg.target.sendAsyncMessage(ipcType, response);
> > +      return true;
> 
> Can you explain why returns true here?

Nice catch!  That should be false.  Returning true here means WorkerMessenger will keep that entry and wait for next worker message with same rilMessageToken.  However, there is no such need in |sendWithIPCMessage()|.

Comment 20

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

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

> > 
> > ::: dom/system/gonk/RadioInterfaceLayer.js
> > @@ +661,5 @@
> > > +      delete this.tokenCallbackMap[message.rilMessageToken];
> > > +    }
> > > +  },
> > > +
> > > +  send: function send(type, message, callback) {
> > 
> > Can we rename send to some better understanding name? like 
> > sendWorkerMessage, ..etc.
> 
> The class is named "WorkerMessenger" already.
> 

The problem I saw here is,
before, the code is 
this.worker.postMessage

I could know it tries to send a message to worker.

now it's becoming 
this.wm.send

It's not so obvious to know 'wm' means, and 'send' didn't expose too much information here.

When I first saw your 2~6 patches, I need to read part 1 to know what does wm.send means.
I think others might have the some problem I get.
So I would suggest a better naming here.
Or you could ask Hsinyi or Gene for the opinion here.

Beside this, I am okay with your patch.
Can you update the comments and send r? to me again.
And update your part2~part6 patches if needed.

I have reviewed part 2 ~ part 6 patches and I think they are okay. But they depend on Part 1 so I would like to wait for Part 1 is ready.

Comment 21

[Hsin-Yi Tsai (she/her) [:hsinyi]]

I would say I am usually not a big fan of abbreviation. In this case, how about we just use this.workerMessenger instead of this.wm?

Comment 22

[Gene Lian (I already quit Mozilla)]

Comment on attachment 784260 [details] [diff] [review]
patch 2.9/2

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

Looks good!

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +2839,5 @@
> +          gMobileMessageDatabaseService
> +            .setMessageDelivery(context.sms.id,
> +                                null,
> +                                DOM_MOBILE_MESSAGE_DELIVERY_ERROR,
> +                                RIL.GECKO_SMS_DELIVERY_STATUS_ERROR,

Oops, you need to rebase this patch due to the envelopId stuff. Directly give review+ but I don't mind to take a look again if you want. :)

@@ +2844,5 @@
> +                                function notifyResult(rv, domMessage) {
> +            // TODO bug 832140 handle !Components.isSuccessCode(rv)
> +            context.request.notifySendMessageFailed(error);
> +            Services.obs.notifyObservers(domMessage, kSmsFailedObserverTopic, null);
> +          }.bind(this));

Do we need this bind?

@@ +2848,5 @@
> +          }.bind(this));
> +          return false;
> +        } // End of send failure.
> +
> +        if (response.deliveryStatus) {

Hope to make sure the slient SMS won't enter into this if-block. We shouldn't update DB for silent SMS.

OK. I understand it now. You didn't request delivery report for a silent SMS.

@@ +2854,5 @@
> +          gMobileMessageDatabaseService
> +            .setMessageDelivery(context.sms.id,
> +                                null,
> +                                context.sms.delivery,
> +                                message.deliveryStatus,

Ditto. Need to rebase.

@@ +2861,5 @@
> +            let topic = (message.deliveryStatus == RIL.GECKO_SMS_DELIVERY_STATUS_SUCCESS)
> +                        ? kSmsDeliverySuccessObserverTopic
> +                        : kSmsDeliveryErrorObserverTopic;
> +            Services.obs.notifyObservers(domMessage, topic, null);
> +          }.bind(this));

Ditto. Need this bind?

@@ +2892,5 @@
> +        gMobileMessageDatabaseService
> +          .setMessageDelivery(context.sms.id,
> +                              null,
> +                              DOM_MOBILE_MESSAGE_DELIVERY_SENT,
> +                              context.sms.deliveryStatus,

Ditto.

@@ +2903,5 @@
> +          }
> +
> +          context.request.notifyMessageSent(domMessage);
> +          Services.obs.notifyObservers(domMessage, kSmsSentObserverTopic, null);
> +        }.bind(this));

Ditto.

Comment 23

[Gene Lian (I already quit Mozilla)]

workerMessenger +1.

Comment 24

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

Attachment: patch 1/2 - Add WorkerMessenger : v2

Address review comment 17 and comment 18, rename 'wm' to 'workerMessenger'.

Comment 25

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

Attachment: patch 2.1/2 - simple, blind sends : v2

rename 'wm' to 'workerMessenger'.

Comment 26

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

Attachment: patch 2.2/2 - simple ipc sends : v2

1. rename 'wm' to 'workerMessenger',
2. omit third parameter of |workerMessenger.sendWithIPCMessage| when it's identical to |msg.name|,
3. include 'queryCallForwardStatus' from attachment 784258 [details] [diff] [review]

Comment 27

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

Attachment: patch 2.3/2 - setPreferredNetworkType : v2

rename 'wm' to 'workerMessenger'

Comment 28

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

Attachment: patch 2.4/2 - setCellBroadcastSearchList : v2

rename 'wm' to 'workerMessenger'

Comment 29

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

Attachment: patch 2.5/2 - enumerateCalls : v2

rename 'wm' to 'workerMessenger'

Comment 30

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

Attachment: patch 2.6/2 - sendMMI, cancelUSSD, setCLIR, and setCallForward : v2

1. rename 'wm' to 'workerMessenger'
2. move 'queryCallForwardStatus' to patch 2.2
3. merge patch 2.6, 2.7

Comment 31

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

Attachment: patch 2.7 - sendSMS : v2

1. remove dangling |this._sentSmsEnvelopes|
2. rename 'wm' to 'workerMessenger'
3. address comment 22

Comment 32

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 788817 [details] [diff] [review]
patch 1/2 - Add WorkerMessenger : v2

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

I got some questions about the 'token' handling.

Canceling r? for I'd like to know the answers.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +654,5 @@
> +    }
> +
> +    let token = message.rilMessageToken;
> +    if (token == null) {
> +      // That's an unsolicited message.  Pass to RadioInterface directly.

That comments seems not correct,
token will also be null/undefined when we call send() without callback parameter.

@@ +666,5 @@
> +      return;
> +    }
> +
> +    let keep = false;
> +    try { keep = callback(message); } catch(e) {}

It's better we could print the e if the callback really wants to throw something, right?

@@ +691,5 @@
> +  send: function send(rilMessageType, message, callback) {
> +    message = message || {};
> +
> +    if (callback) {
> +      message.rilMessageToken = this.token;

rilMessageToken is assigned only when callback is provided?

If we don't provide a callback,
How do you tell it's an unsolicited response? Since the rilMessageToken will be undefined in both cases.

@@ +693,5 @@
> +
> +    if (callback) {
> +      message.rilMessageToken = this.token;
> +      this.tokenCallbackMap[message.rilMessageToken] = callback;
> +      this.token++;

token++ is only when callback is provided?

@@ +712,5 @@
> +   *        A text string for ipc message type. 'msg.name' if omitted.
> +   *
> +   * @TODO: Bug 815526 - deprecate RILContentHelper.
> +   */
> +  sendWithIPCMessage: function sendWithIPCMessage(msg, rilMessageType, ipcType) {

s/msg/message/
I have explained this in previous comments.

Comment 33

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

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #32)
> Review of attachment 788817 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I got some questions about the 'token' handling.

I'll allocate a token for every request, but only create the mapping when callback is provided.

> @@ +712,5 @@
> > +   *        A text string for ipc message type. 'msg.name' if omitted.
> > +   *
> > +   * @TODO: Bug 815526 - deprecate RILContentHelper.
> > +   */
> > +  sendWithIPCMessage: function sendWithIPCMessage(msg, rilMessageType, ipcType) {
> 
> s/msg/message/
> I have explained this in previous comments.

They're different things.  One is from ppmm, another is the message object to be sent to worker.

Comment 34

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

Attachment: patch 1/2 - Add WorkerMessenger : v3

Address comment 32.

Comment 35

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 789382 [details] [diff] [review]
patch 1/2 - Add WorkerMessenger : v3

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

Add r=me

Comment 36

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 788824 [details] [diff] [review]
patch 2.4/2 - setCellBroadcastSearchList : v2

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

It should be a trivial thing for using rilRequestError or success here.
so r+ for this.

::: dom/system/gonk/RadioInterfaceLayer.js
@@ +1314,5 @@
>  
> +    this.workerMessenger.send("setCellBroadcastSearchList",
> +                              { searchListStr: newSearchListStr },
> +                              (function callback(response) {
> +      if (response.rilRequestError != RIL.ERROR_SUCCESS) {

rilRequestError should be only inside ril_worker,
in RadioInterfaceLayer.js, should we use success for that?

Comment 37

[Yoshi Cheng-Hao Huang [:allstars.chh][:allstarschh][:yoshi]]

Comment on attachment 788826 [details] [diff] [review]
patch 2.6/2 - sendMMI, cancelUSSD, setCLIR, and setCallForward : v2

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

stealing ferjm's review.

Comment 38

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

Attachment: 2.4/2 - setCellBroadcastSearchList : v3

1. Address comment 36, use |response.success| instead.
2. In some cases there will be no reply to the request.  Corrected.  Also checked all other requests to ril_worker have their own responses (if rild always answers our ril request, of course).

Comment 39

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

Attachment: patch 1/2 - Add WorkerMessenger : v4

r=yoshi instead

Comment 40

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

Attachment: patch 2/2 - fix all send transactions

1. rebase after bug 899418
2. merge all 2.x patches

Comment 41

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

https://hg.mozilla.org/integration/b2g-inbound/rev/bcba39c2240a
https://hg.mozilla.org/integration/b2g-inbound/rev/b4653ad0ce9b

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/bcba39c2240a
https://hg.mozilla.org/mozilla-central/rev/b4653ad0ce9b

Comment 43

[Fernando Jiménez Moreno [:ferjm]]

(In reply to Yoshi Huang[:allstars.chh][:yoshi] from comment #37)
> Comment on attachment 788826 [details] [diff] [review]
> patch 2.6/2 - sendMMI, cancelUSSD, setCLIR, and setCallForward : v2
> 
> Review of attachment 788826 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> stealing ferjm's review.

Thanks Yoshi!

Sorry Vicamo, I was away on PTO during August.