713451 - B2G RIL: Handle request errors

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Thinker Li [:sinker]]

We need to report errors for B2G RIL requests rather than silent faults.

Comment 1

[Thinker Li [:sinker]]

Attachment: Handle request B2G RIL request error

Comment 2

[Thinker Li [:sinker]]

Attachment: Handle request B2G RIL request error, v2

--
Attachment #584270 [details] [diff] - Attachment is obsolete: true

Comment 3

[Philipp von Weitershausen [:philikon]]

Comment on attachment 584272 [details] [diff] [review]
Handle request B2G RIL request error, v2

I don't really understand why you're sending the RIL request constant to the main thread. As you mention in your comment, this will be meaningless to Gecko. Instead, I think we want to associate the request type with the corresponding request (if there was one), and send a more meaningful message to main thread, containing contextual information about the request that failed.

Comment 4

[Philipp von Weitershausen [:philikon]]

(Also, in the future, if you would like a patch reviewed, please request review for it. Otherwise it's likely nobody will look at it.)

Comment 5

[Thinker Li [:sinker]]

Comment on attachment 584272 [details] [diff] [review]
Handle request B2G RIL request error, v2

This patch is out-of-date since we have made a lot of change.

Comment 7

[Philipp von Weitershausen [:philikon]]

For meaningful error handling, the mainthread should be notified about what operation actually failed. To that end, I think we should hold on to the `options` object that we receive via postMessage from the mainthread. `Buf.newParcel()` is already set up to do this. Then when we process parcels and find errors, we can simply attach the error to `options` and send the entire object back to the mainthread which can then deduce the error and notify callbacks or whatever (the mainthread now has complete flexibility over how to manage the round trip)

Comment 8

[Thinker Li [:sinker]]

There is no unique identification for every request from main thread.  Since ril_worker.js works in async semantically, we need a unique idenitfication that main thread or other threads can identify messages from ril_worker.js for requests uniquely.  We can do it by attaching a serial number maintained by RadioInterfaceLayer.js on every message; the options object at ril_worker.js side.

For unsolicited responses, ril_worker.js need to attach an SN without conflicting with ones generated by RadioInterfaceLayer.js, or to assign a special number (-1 for example).

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to Thinker Li [:sinker] from comment #8)
> There is no unique identification for every request from main thread.

Not yet. My point was that we can easily add it if we pass the whole `options` argument around, for instance in the way you suggest.

Comment 10

[Price Tseng]

Hi Philipp:

According to the former comments, I did some impelementations as below:

In Buf::newParcel(), I added the rilRequestErr member variable for the options object, and set to 0 as default (No error).

  newParcel: function newParcel(type, options) {
    ...
    if (!options) {
      options = {};
    }
    options.rilRequestType = type;
    options.rilRequestErr = 0;
    this.tokenRequestMap[token] = options;
    ...
  },


When HAL responses, Buf::processParcel() is invoked and trying to parse the parcel. When error occurs, the options::rilRequestError variable 
keeps track of the error. 

  processParcel: function processParcel() {
    ...
    options = this.tokenRequestMap[token];
    request_type = options.rilRequestType;
    if (error) {
      options.rilRequestError = error;
      RIL.handleReqError(options);
      return;
    }
    ... 
  },


Finally, RIL::handleReqErr is called, which sends the "options" object to RadioInterfaceLayer::onmessage() through Phone::sendDOMMessage()

  handleReqErr: function handleReqErr(options) {
    Phone.sendDOMMessage({type: "error", options: options});
  },

How do you think? Is the implementation similar to your thoughts?

I got two more questions here:
1. What is the defition of RIL request error? Are the conditions such as "No SIM card", "Wrong phone number" defined as errors, too?
2. How much information should the "options" object take when error occurs. Now the object just contains rilRequestType and rilRequestErr variables. Is it enough for the application in upper layer?

Thanks

Comment 11

[Philipp von Weitershausen [:philikon]]

(In reply to Price Tseng from comment #10)
> According to the former comments, I did some impelementations as below:

Cool! Can you please upload it in patch form next time? Thanks!

> In Buf::newParcel(), I added the rilRequestErr member variable for the
> options object, and set to 0 as default (No error).

Let's call it `rilRequestError`. We can afford the extra 2 characters to make it a complete English word :)

>   newParcel: function newParcel(type, options) {
>     ...
>     if (!options) {
>       options = {};
>     }
>     options.rilRequestType = type;
>     options.rilRequestErr = 0;

Let's initialize this to `null` because we don't know yet whether there was an error or not.

> When HAL responses,

s/HAL/RIL/

> Buf::processParcel() is invoked and trying to parse the
> parcel. When error occurs, the options::rilRequestError variable 
> keeps track of the error. 
> 
>   processParcel: function processParcel() {
>     ...
>     options = this.tokenRequestMap[token];
>     request_type = options.rilRequestType;
>     if (error) {
>       options.rilRequestError = error;

You can unconditionally assign options.rilRequestError = error. The RIL will set it to 0 (no error).

Speaking of numbers, it's probably a good idea to define the different RIL request error values in ril_consts.js as ERROR_*. They're in ril.h as RIL_E_*.

> Finally, RIL::handleReqErr is called, which sends the "options" object to
> RadioInterfaceLayer::onmessage() through Phone::sendDOMMessage()
> 
>   handleReqErr: function handleReqErr(options) {

Again, we can spare the extra characters and make it proper words. Let's call this handleRequestError.

>     Phone.sendDOMMessage({type: "error", options: options});

No need to make the `options` object a subobject. It's leaner if we just do

  options.type = "error";
  Phone.sendDOMMessage(options);

> I got two more questions here:
> 1. What is the defition of RIL request error? Are the conditions such as "No
> SIM card", "Wrong phone number" defined as errors, too?

Yes, when you try to place a phone call and it's an invalid number / there's no SIM card / etc., you will indeed get a RIL request error.

> 2. How much information should the "options" object take when error occurs.
> Now the object just contains rilRequestType and rilRequestErr variables. Is
> it enough for the application in upper layer?

That's the beauty of it. It contains *all* the information that the "upper layer" (RadioInterfaceLayer.js on the main thread) gave it. It's essentially a black box of information that we loop around.

Comment 12

[Price Tseng]

Attachment: Keep track of the error type

Associate the error type with the original message when errors occur

Comment 13

[Philipp von Weitershausen [:philikon]]

Comment on attachment 603647 [details] [diff] [review]
Keep track of the error type

You introduced a bunch of trailing whitespace. Also, please always indent by 2 spaces. r=me with those addressed.

A note about patch generation: please generate them with 8 lines of context and the username and log message already included. See https://developer.mozilla.org/en/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F for instructions.

Comment 14

[Philipp von Weitershausen [:philikon]]

(In reply to Philipp von Weitershausen [:philikon] from comment #13)
> You introduced a bunch of trailing whitespace. Also, please always indent by
> 2 spaces. r=me with those addressed.

Since these were so small, I addressed these and checked the patch in:

https://hg.mozilla.org/mozilla-central/rev/96c36ac5ad2b