Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Description • 13 years ago

The latest plan for talking to the radio on android goes a little something like this:

1. Have a js component that gets used as a service on the main thread. This service will basically just take care of startup and shutdown as well as talk to the DOM.
2. The service will create a worker that will talk to the radio. Maybe the worker script url will live in a pref.
3. The worker will use ctypes to read from and write to the radio file descriptor. It will block occasionally.
4. The main thread will somehow "bump" the file descriptor after posting a message to the worker to ensure that the worker wakes up. qDot will look into this and make sure that it can't block the main thread.

If we can't get the main thread to wake the worker manually then we'll have to have a third thread. Let's hope we don't have to go there, but if we do, we're going to see about reusing roc's WorkerCrossThreadDispatcher thing from https://hg.mozilla.org/users/rocallahan_mozilla.com/media-patches/file/tip/media-graph if we can.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 1 • 13 years ago

As we discussed on IRC, I'm not a big fan of writing a poll- and task-processing loop from scratch, through jsctypes.  All that code already exists in C++, I'd recommend using it.  Also, using blocking IO is going to increase processing latency, so I'd rather we didn't do that.  Don't feel as strongly about that though.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 2 • 13 years ago

This is the start of the approach we discussed at the end of last week.  It needs to have some socket-IO bits filled in, and needs to be hookup up to the JS worker thread.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 3 • 13 years ago

Socket work is happening at

https://github.com/kmachulis-mozilla/mozilla-central/tree/ipc-ril

I'll create a patch on top of cjones' work once I've confirmed that this works.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 4 • 13 years ago

Comment on attachment 570880 [details] [diff] [review]
WIP patch that does RIL socket IO on the chromium IO thread

I'm marking this obsolete and stealing this bug for the worker portion of the RIL stuff. I think this patch belongs in bug 699222. Note that we have a more filled-in version on github.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 5 • 13 years ago

I've been working on this.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 6 • 13 years ago

So, with this patch (and a related patch to ipc/ril) my Firefox starts and I can have a conversation with a socket, with the worker controlling what we send. There are some printfs and other obvious TODO things here (such as replacing my testing worker with the real thing) but this is ready to start being reviewed at the very least.

Ben, let me know how you want to review this. I don't know if it's better, but I have incremental commits that work up to this giant patch. If you want, I can point you at the github repo. The only problem is that the incremental commits are pretty sketchy, so it might be more work reviewing them and ignoring stuff that was fixed later than going through the final patch.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 7 • 13 years ago

This patch still uses the network based communication from the desktop. I just tried it on the phone and the worker isn't starting up. I get the following error:

E/Gecko *** Console Service *** ( 2690): While creating services from category 'profile-after-change', could not create service for entry 'Telephony Radio', contract ID '@mozilla.org/telephony/radio;1'

It seems to be dying on Radio.cpp around line 258 (will be different in patch, I have a bit of extra android logging code in mine):

  nsCOMPtr<nsITelephonyWorker> worker(do_CreateInstance(kTelephonyWorkerCID));

The worker is getting set to zero I guess.

Things still seem to work fine on the desktop, this only seems to be a problem on the gonk platform.

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 8 • 13 years ago

Is this something to do with an .xpt missing from package-manifest.in?

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 9 • 13 years ago

The line:

@BINPATH@/components/dom_telephony.xpt

was in browser/installer/package-manifest.in but was missing from b2g/installer/package-manifest.in, but adding that doesn't seem to fix it. I still get the same error.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 10 • 13 years ago

The problem was that I forgot to add the new component to the manifest. The latest commits at https://github.com/mrbkap/mozilla-central/commits/ipc-ril-cjones work on the phone as well.

	Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Comment 11 • 13 years ago

Comment on attachment 577301 [details] [diff] [review]
Mostly-working WIP

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

::: dom/telephony/Radio.cpp
@@ +183,5 @@
> +  NS_ASSERTION(!JS_IsRunning(aCx), "Are we being called somehow?");
> +  JSObject *workerGlobal = JS_GetGlobalObject(aCx);
> +
> +  JSAutoRequest ar(aCx);
> +  JSAutoEnterCompartment ac;

This should be unnecessary, the worker code makes sure that the context is set up correctly.

@@ +201,5 @@
> +    : mDispatcher(aDispatcher)
> +  { }
> +
> +  virtual void MessageReceived(RilMessage *aMessage) {
> +    mDispatcher->DispatchRILEvent(aMessage->mData, aMessage->mSize);

Hm. This should be encapsulated in a WorkerTask and not tacked on to WorkerCrossThreadDispatcher.

I thought your ConnectWorkerToRIL task would just define a new kind of event object and hook up its prototype chain properly... There's no need to muck with Events.cpp, is there? You could even just reuse MessageEvent and set its type to "RILMessageEvent".

@@ +253,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  NS_ASSERTION(!JSVAL_IS_PRIMITIVE(workerval), "bad worker value");
> +  JSContext *cx;
> +  rv = nsContentUtils::ThreadJSContextStack()->GetSafeJSContext(&cx);

Weren't you going to try to reuse the worker RuntimeService::AutoSafeJSContext thing? Otherwise I think you need to push your cx on the contextstack too right?

@@ +281,5 @@
> +  // worker will try to call the error reporter on the context it was created
> +  // on. However, That doesn't work for component contexts and can result in
> +  // crashes. This onerror handler has to make sure that it calls preventDefault
> +  // on the incoming event.
> +  rv = SetHandler(cx, workerobj, HandleError, "onerror");

I think we should avoid this altogether and just do the message and error handlers in the JS file. I don't see why this needs to be native, and it may make the implementation harder/slower.

@@ +292,5 @@
> +    return NS_ERROR_UNEXPECTED;
> +  }
> +
> +  // Now that we're set up, connect ourselves to the RIL thread.
> +  mozilla::RefPtr<RILReceiver> receiver = new RILReceiver(wctd);

Hm... nsRefPtr followed by mozilla::RefPtr... Sounds confusing.

::: dom/telephony/worker-component/nsTelephonyWorker.js
@@ +44,5 @@
> +const nsITelephonyWorker                = Components.interfaces.nsITelephonyWorker;
> +
> +function nsTelephonyWorker()
> +{
> +    this.worker = new ChromeWorker("resource://gre/modules/nsRILDecoder.js");

We could add the error/message handlers here. Simple.

::: dom/workers/Events.cpp
@@ +1235,5 @@
>  
> +  if (!aMainRuntime) {
> +    WorkerPrivate* worker = GetWorkerPrivateFromContext(aCx);
> +    if (worker->IsChromeWorker() &&
> +        !RILMessageEvent::InitClass(aCx, aGlobal, eventProto)) {

Hm, I really don't like these being inited in every chrome worker. They should be restricted to just the RIL worker.

::: dom/workers/RuntimeService.cpp
@@ +467,5 @@
>  
> +WorkerCrossThreadDispatcher*
> +GetWorkerCrossThreadDispatcher(JSContext* aCx, jsval aWorker)
> +{
> +  if (!JSVAL_IS_OBJECT(aWorker))

JSVAL_IS_PRIMITIVE! I don't think GetInstancePrivate is nullsafe.

@@ +470,5 @@
> +{
> +  if (!JSVAL_IS_OBJECT(aWorker))
> +    return NULL;
> +  WorkerPrivate* w = worker::GetInstancePrivate(aCx, JSVAL_TO_OBJECT(aWorker));
> +  if (!w)

Nit: braces even on single lines

@@ +477,5 @@
> +}
> +
> +namespace {
> +
> +class WorkerTaskRunnable : public WorkerRunnable

I think you should make WorkerTaskRunnable have empty PreDispatch and PostDispatch methods since the point is to be dispatched from some non-parent thread.

@@ +482,5 @@
> +{
> +public:
> +  WorkerTaskRunnable(WorkerPrivate* aPrivate, WorkerTask* aTask)
> +    // XXX Is ModifyBusyCount correct?
> +    : WorkerRunnable(aPrivate, WorkerThread, ModifyBusyCount),

No, this isn't correct. ModifyBusyCount only works when run from the parent thread, and these are most likely going to run from some other thread. The busy count is only used to keep the worker alive while a task is running, and the whole WorkerCrossThreadDispatcher API is based on strongly rooting the worker in some other way so it's not a big deal.

@@ +502,5 @@
> +      mSize(aSize)
> +  { }
> +
> +  bool
> +  PreDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate)

These should be unnecessary now.

@@ +535,5 @@
> +bool
> +WorkerTaskRunnable::WorkerRun(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
> +{
> +  mTask->RunTask(aCx);
> +  return true;

Hm, I think you should make RunTask return a bool, otherwise there's no way for a task to throw exceptions.

@@ +544,5 @@
> +bool
> +WorkerCrossThreadDispatcher::PostTask(WorkerTask* aTask)
> +{
> +  mozilla::MutexAutoLock lock(mMutex);
> +  if (!mPrivate)

Nit: braces

::: dom/workers/Worker.cpp
@@ +435,4 @@
>      }
>    }
>  
> +  if (aFunctionName) {

I don't know why this change was made but you can't return NULL here without setting an exception.

@@ +470,5 @@
>    }
>  }
>  
> +WorkerPrivate*
> +GetInstancePrivate(JSContext* aCx, JSObject* aObj)

Hm, I don't like exposing this really... Can we just make the impl for GetWorkerCrossThreadDispatcher live in Worker.cpp?

::: dom/workers/Worker.h
@@ +54,5 @@
>  void
>  ClearPrivateSlot(JSContext* aCx, JSObject* aObj, bool aSaveEventHandlers);
>  
> +WorkerPrivate*
> +GetInstancePrivate(JSContext* aCx, JSObject* aObj);

Boo. See above ;)

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 12 • 13 years ago

(In reply to ben turner [:bent] from comment #11)
> @@ +201,5 @@
> > +    : mDispatcher(aDispatcher)
> > +  { }
> > +
> > +  virtual void MessageReceived(RilMessage *aMessage) {
> > +    mDispatcher->DispatchRILEvent(aMessage->mData, aMessage->mSize);
> 
> Hm. This should be encapsulated in a WorkerTask and not tacked on to
> WorkerCrossThreadDispatcher.
> 
> I thought your ConnectWorkerToRIL task would just define a new kind of event
> object and hook up its prototype chain properly... There's no need to muck
> with Events.cpp, is there? You could even just reuse MessageEvent and set
> its type to "RILMessageEvent".

I don't think I understand what you mean here. Doing anything with events outside of dom/threads is impossible (outside of calling postMessage on the worker, which I can't do off the main thread). MessageEvent does crazy stuff with structured clones, which I don't want to do (all the RILMessageEvent wants to do is expose a buffer as a typed array). In order to dispatch any sort of event, you have to have some code in dom/workers any way, since Events.h isn't exported anywhere. Even with that, you can't dispatch an event that hasn't been added to Events::GetPrivate.

What am I missing?

> Weren't you going to try to reuse the worker
> RuntimeService::AutoSafeJSContext thing? Otherwise I think you need to push
> your cx on the contextstack too right?

Oops, yeah. Rather than try to figure out how to expose AutoSafeJSContext, I'm going to just push the context on the stack.

> I think we should avoid this altogether and just do the message and error
> handlers in the JS file. I don't see why this needs to be native, and it may
> make the implementation harder/slower.

I thought we talked about this and you told me not to do it this way. This basically means communicating with the worker through the nsITelephonyWorker interface (i.e. XPCOM). I don't care either way. Though I agree that doing more of this in JS is cleaner.

> Hm... nsRefPtr followed by mozilla::RefPtr... Sounds confusing.

Yeah, I'll change it. The only reason I used mozilla::RefPtr here was that we use that version in Ril.cpp and I wanted to be consistent with that.

> ::: dom/workers/Events.cpp
> > +        !RILMessageEvent::InitClass(aCx, aGlobal, eventProto)) {
> 
> Hm, I really don't like these being inited in every chrome worker. They
> should be restricted to just the RIL worker.

There's no clean way to do this currently. Events are initialized before we know anything about the worker that we're about to load. I could not do this here and try to init the class later, but that requires some code in dom/workers since none of the required classes are visible from dom/telephony.

> > +  if (!JSVAL_IS_OBJECT(aWorker))
> 
> JSVAL_IS_PRIMITIVE! I don't think GetInstancePrivate is nullsafe.

FWIW, this wasn't my code :-) (and neither were the missing braces).

> @@ +502,5 @@
> > +  PreDispatch(JSContext* aCx, WorkerPrivate* aWorkerPrivate)
> 
> These should be unnecessary now.

Ah, I'd copied them from roc's patch. Taking them out causes assertions. The problem is that we're using the outermost WorkerPrivate (the one that we get from the worker on the content side). I'm tempted to leave the hooks in to silence the warnings, but I don't know if that might cause real problems down the line.
 
> ::: dom/workers/Worker.cpp
> > +  if (aFunctionName) {
> 
> I don't know why this change was made but you can't return NULL here without
> setting an exception.

Yeah we talked about this. The reason is that in the case of GetCrossThreadDispatcher we call this function and don't have a function name to pass in. I've taken out the if and simply pass in a function name. Of course, in this case, the function won't ever reach here.

	Blake Kaplan (:mrbkap) (inactive) Assignee
	Comment 13 • 13 years ago

The latest code at the repo takes care of ben's requests here. He's going to re-integrate his telephony APIs into the repo (he has push access). Once that's taken care of and hooked up, all we need to do are figure out the appropriate ifdefs (I assume that we don't want to build this stuff if we're not on a phone) and get it checked in.

	Kyle Machulis [:qdot] [:kmachulis] (INACTIVE)
	Comment 14 • 13 years ago

Note: We took care of the ifdefs in the patch on 699235. We've actually got everything integrated and dialing now, though there's lots of cleanup left. Check out my github mozilla-central for merging what we've done.

	Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Comment 15 • 13 years ago

Comment on attachment 577301 [details] [diff] [review]
Mostly-working WIP

r=me on all the stuff we checked in to your repo yesterday.

	Philipp von Weitershausen [:philikon]
	Comment 16 • 13 years ago

I'm attaching Blake's code from GitHub on his behalf. Hope you won't mind, Blake, we'd like to get this landed.

	Ben Turner (not reading bugmail, use the needinfo flag!) Reporter
	Comment 18 • 13 years ago

Comment on attachment 578718 [details] [diff] [review]
Part 1 (v1): cross-thread dispatching for web workers

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

::: dom/workers/Events.cpp
@@ +43,5 @@
>  
>  #include "jsapi.h"
>  #include "jscntxt.h"
>  #include "jsfriendapi.h"
> +#include "jstypedarray.h"

This needs to be removed.

	Philipp von Weitershausen [:philikon]
	Comment 19 • 13 years ago

Addressed bent's review comments.

	Philipp von Weitershausen [:philikon]
	Comment 20 • 13 years ago

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d7db29d9362
https://hg.mozilla.org/integration/mozilla-inbound/rev/a1f89c23bbca

	Matt Brubeck (:mbrubeck)
	Comment 21 • 13 years ago

https://hg.mozilla.org/mozilla-central/rev/6d7db29d9362
https://hg.mozilla.org/mozilla-central/rev/a1f89c23bbca