699256 - Handle RIL parcels in a ChromeWorker

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)]

We need to keep state of which RIL Parcels are waiting for return, as well as how to route them. Create a generalized RIL Manager class that deals with parcel transfer and state.

Comment 1

[Andreas Gal :gal]

Attachment: ril manager in js

This is a sketch of code that runs in a worker and processes input from RIL. There is also code showing how to send, but I think that should be in a separate file since it won't run on the worker. Worker and sender have to synchronize about the outstanding list, so I am less and less convinced that a worker is a good idea here. I think all of this should be on the main thread.

Comment 2

[Philipp von Weitershausen [:philikon]]

Morphing this bug to just implement the initial prototype of RIL parcel handling in a ChromeWorker.

Comment 3

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v1): RIL parcel processing worker

Comment 4

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v1): Experimental navigator.mozTelephony API

Comment 5

[Philipp von Weitershausen [:philikon]]

Attachment: Part 3 (v1): Hook it all up to the buildsystem

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 578723 [details] [diff] [review]
Part 2 (v1): Experimental navigator.mozTelephony API

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

r=me, we need to fix the below things at some point, now or with a followup.

::: dom/telephony/Telephony.js
@@ +88,5 @@
> +    }
> +    if (!this._listeners[type]) {
> +      this._listeners[type] = [];
> +    }
> +    this._listeners[type].push(handler);

This will add duplicates if addEventListener is called multiple times with the same listener, normal behavior is to remove the old one first.

@@ +99,5 @@
> +         (index = list.indexOf(handler) != -1)) {
> +       list.splice(index, 1);
> +       return;
> +     }
> +     throw "XXX TODO some error";

This doesn't match normal behavior. Not error should be thrown.

@@ +109,5 @@
> +    if (!list) {
> +      return;
> +    }
> +    event.target = this;
> +    list.forEach(function (handler) {

This doesn't handle mutations during event firing. Correct behavior is to not call any listeners that were added during firing and to not call any listeners that were removed during firing. Not much way to do this except with stack guards to track mutation from when you start dispatching. Oh, and you have to worry about recursion.

Comment 7

[Philipp von Weitershausen [:philikon]]

Attachment: Part 2 (v2): Experimental navigator.mozTelephony API

Address bent's review comments, except for the recursion stuff. This needs to be scrutinized and tested for edge cases before it goes into production code anyway.

Comment 8

[Andreas Gal :gal]

Comment on attachment 578720 [details] [diff] [review]
Part 1 (v1): RIL parcel processing worker

>+const DEBUG = true; // set to false to suppress debug messages

DEBUG code should be removed once this is more stable, or make it less verbose and add a pref.

>+  //TODO review these values
>+  INCOMING_BUFFER_LENGTH: 4096,
>+  OUTGOING_BUFFER_LENGTH: 4096,

Make this smaller and make sure you grow the buffer when tail catches head.

>+    // Track where incoming data is read from and written to.
>+    //XXX I think we could fold this into one index just like we do it
>+    // with outgoingIndex.

Yeah, file a follow-up, this looks wonky.

>+    // Maps tokens we send out with requests to the request type, so that
>+    // when we get a response parcel back, we know what request it was for.
>+    this.tokenRequestMap = {};

Object.create(null) is safer

>+  readUint8: function readUint8() {
>+    let value = this.incomingBytes[this.incomingReadIndex];
>+    this.incomingReadIndex = (this.incomingReadIndex + 1) %
>+                             this.INCOMING_BUFFER_LENGTH;

Some sort of & would be nicer and faster I think.

>+  readUint32List: function readUint32List() {
>+    let length = this.readUint32();
>+    let ints = [];
>+    for (let i = 0; i < length; i++) {
>+      ints.push(this.readUint32());
>+    }
>+    return ints;
>+  },

This is a bit allocation happy.

>+    if (!(string_len % 2)) {
>+      delimiter += this.readUint16();
>+    }

& ...

>+  writeUint8: function writeUint8(value) {
>+    this.outgoingBytes[this.outgoingIndex] = value;
>+    this.outgoingIndex += 1;
>+  },

++ !

>+    if (!(value.length % 2)) {

...

>+  writeToIncoming: function writeToIncoming(incoming) {
>+    // We don't have to worry about the head catching the tail since
>+    // we process any backlog in parcels immediately, before writing
>+    // new data to the buffer. So the only edge case we need to handle
>+    // is when the incoming data is larger than the buffer size.
>+    if (incoming.length > this.INCOMING_BUFFER_LENGTH) {
>+      if (DEBUG) {
>+        debug("Current buffer of " + this.INCOMING_BUFFER_LENGTH +
>+              " can't handle incoming " + incoming.length + " bytes ");
>+      }
>+      let oldBytes = this.incomingBytes;
>+      while (this.INCOMING_BUFFER_LENGTH < incoming.length) {
>+        this.INCOMING_BUFFER_LENGTH *= 2;
>+      }

There is probably some math with log you can do here. Make sure an overflow doesn't make this an endless loop (been there, done that).

>+    //TODO XXX this assumes that postRILMessage can eat a ArrayBufferView!
>+    // It also assumes that it will make a copy of the ArrayBufferView right
>+    // away.

Ugly as sin.

Good starting point. Lets land it so we can unblock parallel work.

Comment 9

[Philipp von Weitershausen [:philikon]]

(In reply to Andreas Gal :gal from comment #8)
> >+const DEBUG = true; // set to false to suppress debug messages
> 
> DEBUG code should be removed once this is more stable, or make it less
> verbose and add a pref.

Yup. For now it's incredibly useful, though.

> >+  //TODO review these values
> >+  INCOMING_BUFFER_LENGTH: 4096,
> >+  OUTGOING_BUFFER_LENGTH: 4096,
> 
> Make this smaller and make sure you grow the buffer when tail catches head.

Ok, will decrease the values. The tail-catching-head scenario is already taken care of in Buf.writeToIncoming()!

> >+    // Track where incoming data is read from and written to.
> >+    //XXX I think we could fold this into one index just like we do it
> >+    // with outgoingIndex.
> 
> Yeah, file a follow-up, this looks wonky.

Actually, that comment is outdated, I will remove it. We will battle-test this code with RIL packets all week. I definitely have some suspicions that the indexing counting has a few bugs, so we will file follow-ups as we see them.

> >+    // Maps tokens we send out with requests to the request type, so that
> >+    // when we get a response parcel back, we know what request it was for.
> >+    this.tokenRequestMap = {};
> 
> Object.create(null) is safer

Huh? That throws a TypeError for me. Also, I thought we preferred literals over explicit constructors wherever they're possible...

> >+  readUint32List: function readUint32List() {
> >+    let length = this.readUint32();
> >+    let ints = [];
> >+    for (let i = 0; i < length; i++) {
> >+      ints.push(this.readUint32());
> >+    }
> >+    return ints;
> >+  },
> 
> This is a bit allocation happy.

Well, yes. Both read{String|List}List() functions have that problem, but unfortunately I see no way around the one array and N string/number allocations.

> >+    //TODO XXX this assumes that postRILMessage can eat a ArrayBufferView!
> >+    // It also assumes that it will make a copy of the ArrayBufferView right
> >+    // away.
> 
> Ugly as sin.

Yes, though if it's well documented, I see no problem. Any alternative that has us deal with this problem in JS before we hand the data off to postRILMessage would mean more object allocations, and I know how you feel about those ;). A memcpy in C land is cheaper, no?

Comment 10

[Philipp von Weitershausen [:philikon]]

Attachment: Part 1 (v2): RIL parcel processing worker

Address gal's review comments (and also fix a hideous off-by-one error.)

Comment 11

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a95b61c88f65
https://hg.mozilla.org/integration/mozilla-inbound/rev/4452fc1a3cad
https://hg.mozilla.org/integration/mozilla-inbound/rev/431962a6ac34

Comment 12

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/a95b61c88f65
https://hg.mozilla.org/mozilla-central/rev/4452fc1a3cad
https://hg.mozilla.org/mozilla-central/rev/431962a6ac34