Last Comment Bug 556846 - Investigate multi-process Jetpack
: Investigate multi-process Jetpack
Status: RESOLVED FIXED
:
Product: Mozilla Labs
Classification: Other
Component: Jetpack Prototype (show other bugs)
: unspecified
: x86 Linux
: -- normal with 1 vote (vote)
: ---
Assigned To: Ben Newman (:bnewman) (:benjamn)
:
Mentors:
: 557259 557340 565135 (view as bug list)
Depends on: 545237 546355 548904 560643 563747 564086 565135
Blocks: SM-OOPP 567703 568695 568698 574039 574870
  Show dependency treegraph
 
Reported: 2010-04-02 12:29 PDT by Josh Matthews [:jdm]
Modified: 2012-01-09 12:16 PST (History)
11 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Basic support for PJetpack/JetpackParent/JetpackChild, rev. 1 (27.08 KB, patch)
2010-05-03 14:21 PDT, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
mozilla+ben: review+
Details | Diff | Splinter Review
Basic support with functional XPCOM interface, rev. 2 (43.05 KB, patch)
2010-05-04 06:59 PDT, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
no flags Details | Diff | Splinter Review
Basic support with functional XPCOM interface, rev. 2.1 (124.30 KB, patch)
2010-05-05 17:30 PDT, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
cjones.bugs: review+
Details | Diff | Splinter Review
Unbitrotted and with COMPONENT_LIBS fix. (41.16 KB, patch)
2010-05-05 20:36 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Support for strings-only messaging and script loading. (34.94 KB, patch)
2010-05-10 17:46 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Support for strings-only messaging and script loading (take two). (34.94 KB, patch)
2010-05-10 17:54 PDT, Ben Newman (:bnewman) (:benjamn)
benjamin: feedback+
Details | Diff | Splinter Review
WIP patch to port mozilla::jsipc::Handle from projects/electrolysis to mozilla-central. (38.43 KB, patch)
2010-05-11 19:52 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
WIP patch to port mozilla::jsipc::Handle from projects/electrolysis to mozilla-central (take two). (30.18 KB, patch)
2010-05-11 19:54 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Add support (and tests) for passing Handles through sendMessage. (43.11 KB, patch)
2010-05-12 16:57 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Update process/thread goop to m-c latest (43.83 KB, patch)
2010-05-17 12:46 PDT, Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
no flags Details | Diff | Splinter Review
Support for arbitrary numbers of JSON-serializable arguments to sendMessage/callMessage. (30.58 KB, patch)
2010-05-26 16:38 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
More tests of serializable arguments. (33.37 KB, patch)
2010-05-27 18:34 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Support cyclic object references in sendMessage arguments. (20.48 KB, patch)
2010-05-28 23:37 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Multiprocess Jetpack API, all in one patch, with tests. (72.68 KB, patch)
2010-06-01 17:21 PDT, Ben Newman (:bnewman) (:benjamn)
benjamin: review+
Details | Diff | Splinter Review
Review comments (3.18 KB, text/plain)
2010-06-01 18:56 PDT, Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg]
no flags Details
Catching up with bit-rot, and making Variant serialization more forgiving. (66.68 KB, patch)
2010-06-07 10:37 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Addressing bent's feedback. (67.18 KB, patch)
2010-06-21 12:06 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Addressing all of bent's feedback. (67.33 KB, patch)
2010-06-21 12:18 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review
Addressing feedback and fixing a bug. (70.40 KB, patch)
2010-06-22 15:03 PDT, Ben Newman (:bnewman) (:benjamn)
bent.mozilla: review+
Details | Diff | Splinter Review
Interdiff from last-reviewed patch. (20.82 KB, patch)
2010-06-22 15:04 PDT, Ben Newman (:bnewman) (:benjamn)
no flags Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2010-04-02 12:29:09 PDT
Using the new electrolysis technologies, a prototype of the prototype should be created to figure out some of the unknowns we'll run into.
Comment 1 Josh Matthews [:jdm] 2010-04-02 12:31:42 PDT
I will upload my prototype changes shortly.  It depends on a few open bugs with uncommitted patches.
Comment 2 Josh Matthews [:jdm] 2010-04-02 12:35:51 PDT
Areas of concern are documented at http://wiki.mozilla.org/Electrolysis/Jetpack
Comment 3 Josh Matthews [:jdm] 2010-04-05 18:03:47 PDT
The prototype hg repo is at http://people.mozilla.org/~jmatthews/jetpack-e10s .  It's all a bit slapdash and held together with bits of glue and string, and I'm happy to elaborate on any of the work I did.
Comment 4 Josh Matthews [:jdm] 2010-04-06 16:19:07 PDT
Cripes.  I realized that a few new files were missing from the repo, and accidentally overwrote the meat of the remote JS changes while trying to add it.  That is... unfortunate.  Hopefully it shouldn't be a big deal to rewrite it.
Comment 5 Myk Melez [:myk] [@mykmelez] 2010-04-08 17:16:09 PDT
Josh: it seems like you've already done a bunch of investigation of multi-process Jetpack!  Does that mean this bug can now be resolved fixed?  Or should this bug actually be summarized as something like "implement separate processes for Jetpack-based extensions" as architected on the Multi-process Jetpack page <https://developer.mozilla.org/en/Multi-Process_Architecture/Jetpack>?
Comment 6 Josh Matthews [:jdm] 2010-04-08 18:36:32 PDT
It can probably be resolved in its current state, since the goal was simply to create a basic prototype from the prototype to identify problems we'll encounter in the future.  I'm going to leave this open until I finish rewriting the missing code for the prototype, however.
Comment 7 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-05-03 14:21:38 PDT
Created attachment 443187 [details] [diff] [review]
Basic support for PJetpack/JetpackParent/JetpackChild, rev. 1

https://wiki.mozilla.org/Electrolysis/Jetpack
Comment 8 Atul Varma [:atul] 2010-05-03 14:39:08 PDT
Er, one thing I forgot to mention in the meeting we just had was: what's the current level of e10s support on OS X?  When we move the Jetpack SDK to e10s, will we have to develop on Linux/Windows VMs, or can we use our Macs?
Comment 9 Ben Newman (:bnewman) (:benjamn) 2010-05-03 14:47:32 PDT
Comment on attachment 443187 [details] [diff] [review]
Basic support for PJetpack/JetpackParent/JetpackChild, rev. 1

Looks good.  Will there at some point be a PJetpackProcess, a la PContentProcess?  And a JetpackProcessChild to go along with JetpackProcessParent?
Comment 10 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-05-03 15:06:44 PDT
I'm sure we can use macs. We will have to disable IPC plugins on ppc at least, but I think that's possible.
Comment 11 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-05-03 15:07:06 PDT
No, PJetpack is the top of the hierarchy, we won't need a PJetpackProcess.
Comment 12 Ben Newman (:bnewman) (:benjamn) 2010-05-03 18:41:19 PDT
Any suggestions about how to load URIs without using XPCOM?  I don't really want to reimplement nsIURI.
Comment 13 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-05-03 18:45:51 PDT
My thought was to open the stream in the chrome process and just ship the actual script data over the wire.
Comment 14 Ben Newman (:bnewman) (:benjamn) 2010-05-03 18:53:06 PDT
(In reply to comment #13)
> My thought was to open the stream in the chrome process and just ship the
> actual script data over the wire.

>+async protocol PJetpack
>+{
>+child:
>+  async LoadImplementation(nsCString script);
>+};

Ah, so |script| will end up being JS code, not the script URI (in contrast to ajetpack.loadImplementation(uri)).
Comment 15 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-05-04 06:11:06 PDT
Yeah, that's what I was thinking. We'll probably go through several revisions of the IPDL protocol, since it's likely that we'll want to send fastload data, not unparsed script, and maybe even used sharedmem if startup performance becomes noticeable.
Comment 16 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-05-04 06:59:24 PDT
Created attachment 443340 [details] [diff] [review]
Basic support with functional XPCOM interface, rev. 2

cjones, I borrowed this in general from the plugin stuff, but this uses async launching instead of sync. Can you look it over to make sure I haven't done anything stupid?
Comment 17 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-05-05 16:01:36 PDT
This patch bitrotted slightly from 563747, but with that fixed it doesn't build (debug) on my machine:

make[4]: *** No rule to make target `../../staticlib/libjetpack_s.a', needed by `libxul.so'.  Stop.
Comment 18 Ben Newman (:bnewman) (:benjamn) 2010-05-05 16:42:03 PDT
(In reply to comment #17)
> This patch bitrotted slightly from 563747, but with that fixed it doesn't build
> (debug) on my machine:
> 
> make[4]: *** No rule to make target `../../staticlib/libjetpack_s.a', needed by
> `libxul.so'.  Stop.

I had the same problem.  I fixed it (perhaps not ideally) by adding EXPORT_LIBRARY = 1 to js/jetpack/Makefile.in, and making the following changes:

diff --git a/toolkit/toolkit-makefiles.sh b/toolkit/toolkit-makefiles.sh
--- a/toolkit/toolkit-makefiles.sh
+++ b/toolkit/toolkit-makefiles.sh
@@ -208,8 +208,12 @@ MAKEFILES_xpconnect="
   js/src/xpconnect/tools/Makefile
   js/src/xpconnect/tools/idl/Makefile
 "
 
+MAKEFILES_jetpack="
+  js/jetpack/Makefile
+"
+
 MAKEFILES_jsdebugger="
   js/jsd/Makefile
   js/jsd/idl/Makefile
 "
@@ -818,8 +822,9 @@ add_makefiles "
   $MAKEFILES_gfx
   $MAKEFILES_htmlparser
   $MAKEFILES_intl
   $MAKEFILES_xpconnect
+  $MAKEFILES_jetpack
   $MAKEFILES_jsdebugger
   $MAKEFILES_jsctypes
   $MAKEFILES_content
   $MAKEFILES_layout
Comment 19 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-05-05 17:18:24 PDT
Hrm, then I missed a refresh: there should be an IS_COMPONENT.
Comment 20 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-05-05 17:30:37 PDT
Created attachment 443772 [details] [diff] [review]
Basic support with functional XPCOM interface, rev. 2.1
Comment 21 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-05-05 19:03:33 PDT
(In reply to comment #20)
> Created an attachment (id=443772) [details]
> Basic support with functional XPCOM interface, rev. 2.1

Just to warn you, a crapload of crap snuck into this patch (antivirus gone mad?)

$ hg qimport -n 556846-review bz:556846
Fetching... done
Parsing... done
adding 556846-review to series file
cjones@beast:~/mozilla/mozilla-central[nspr-crud]$ hg qpush
applying 556846-review
patching file toolkit/library/nsStaticXULComponents.cpp
Hunk #2 succeeded at 254 with fuzz 2 (offset -2 lines).
unable to find 'toolkit/mozapps/extensions/test/unit/data/test_bug404024.xml' for patching
1 out of 1 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/test/unit/data/test_bug404024.xml.rej
unable to find 'toolkit/mozapps/extensions/test/unit/data/test_bug417606.xml' for patching
1 out of 1 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/test/unit/data/test_bug417606.xml.rej
unable to find 'toolkit/mozapps/extensions/test/unit/data/test_bug424262.xml' for patching
1 out of 1 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/test/unit/data/test_bug424262.xml.rej
unable to find 'toolkit/mozapps/extensions/test/unit/test_bug404024.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/test/unit/test_bug404024.js.rej
unable to find 'toolkit/mozapps/extensions/test/unit/test_bug417606.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/test/unit/test_bug417606.js.rej
unable to find 'toolkit/mozapps/extensions/test/unit/test_bug424262.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/test/unit/test_bug424262.js.rej
unable to find 'toolkit/mozapps/extensions/test/unit/test_bug449027.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/test/unit/test_bug449027.js.rej
unable to find 'toolkit/mozapps/extensions/test/unit/test_bug455906.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/test/unit/test_bug455906.js.rej
unable to find 'toolkit/mozapps/extensions/test/unit/test_bug514327_3.js' for patching
1 out of 1 hunks FAILED -- saving rejects to file toolkit/mozapps/extensions/test/unit/test_bug514327_3.js.rej
patching file toolkit/toolkit-tiers.mk
Hunk #1 FAILED at 100
1 out of 1 hunks FAILED -- saving rejects to file toolkit/toolkit-tiers.mk.rej
patch failed, unable to continue (try -v)
toolkit/mozapps/extensions/test/unit/data/test_bug404024.xml not tracked!
toolkit/mozapps/extensions/test/unit/data/test_bug417606.xml not tracked!
toolkit/mozapps/extensions/test/unit/data/test_bug424262.xml not tracked!
toolkit/mozapps/extensions/test/unit/test_bug404024.js not tracked!
toolkit/mozapps/extensions/test/unit/test_bug417606.js not tracked!
toolkit/mozapps/extensions/test/unit/test_bug424262.js not tracked!
toolkit/mozapps/extensions/test/unit/test_bug449027.js not tracked!
toolkit/mozapps/extensions/test/unit/test_bug455906.js not tracked!
toolkit/mozapps/extensions/test/unit/test_bug514327_3.js not tracked!
toolkit/mozapps/extensions/test/unit/data/test_bug404024.xml: No such file or directory
toolkit/mozapps/extensions/test/unit/data/test_bug417606.xml: No such file or directory
toolkit/mozapps/extensions/test/unit/data/test_bug424262.xml: No such file or directory
toolkit/mozapps/extensions/test/unit/test_bug404024.js: No such file or directory
toolkit/mozapps/extensions/test/unit/test_bug417606.js: No such file or directory
toolkit/mozapps/extensions/test/unit/test_bug424262.js: No such file or directory
toolkit/mozapps/extensions/test/unit/test_bug449027.js: No such file or directory
toolkit/mozapps/extensions/test/unit/test_bug455906.js: No such file or directory
toolkit/mozapps/extensions/test/unit/test_bug514327_3.js: No such file or directory
Comment 22 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-05-05 19:07:33 PDT
Still getting this (after rebasing to trunk)

make[4]: *** No rule to make target `../../staticlib/libjetpack_s.a', needed by `libxul.so'.  Stop.
Comment 23 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-05-05 19:11:18 PDT
Looks like this was the missing secret sauce

ifdef MOZ_IPC
COMPONENT_LIBS +=  jetpack_s
endif
Comment 24 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-05-05 20:34:57 PDT
Comment on attachment 443772 [details] [diff] [review]
Basic support with functional XPCOM interface, rev. 2.1

My, what a passel o' boilerplate.  But looks fine to me, other than the COMPONENT_LIBS issue; I'll post an updated patch.
Comment 25 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-05-05 20:36:39 PDT
Created attachment 443796 [details] [diff] [review]
Unbitrotted and with COMPONENT_LIBS fix.
Comment 26 Ben Newman (:bnewman) (:benjamn) 2010-05-06 18:25:52 PDT
I'm assuming that .registerReceiver(msgName, fn) can be called multiple times with the same message name but different receiver functions.  If there are multiple receiver functions for .sendMessage(msgName, data), we just run all of them; that's easy.  But what about .callMessage(msgName, data), which is supposed to be synchronous and may return a value?  What should that return value be in case there are 0 or >1 receivers registered for msgName?
Comment 27 Ben Newman (:bnewman) (:benjamn) 2010-05-10 17:46:35 PDT
Created attachment 444529 [details] [diff] [review]
Support for strings-only messaging and script loading.

See js/jetpack/tests/unit/test_jetpack.js and js/jetpack/bootstrap-impl.js for examples of what's supported.

Wherever you see // TODO, I plan to replace an nsString IPDL parameter with something more general (at least allowing Handles), but that requires porting Handles from projects/electrolysis, and a string-only API is pretty useful already for testing. 

I solved the problem in comment 26 by returning an array of strings from callMessage.  This value will probably be ignored in practice.

Next I think I'm going to switch gears to working on bug 563010 so that I can implement implGlobal.wrap(obj) and userGlobal.require(id).
Comment 28 Ben Newman (:bnewman) (:benjamn) 2010-05-10 17:54:27 PDT
Created attachment 444535 [details] [diff] [review]
Support for strings-only messaging and script loading (take two).

Forgot to save js/jetpack/bootstrap-impl.js, thus neglecting the [0] after callMessage.
Comment 29 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-05-11 07:45:28 PDT
Hrm, I wasn't actually imagining that there would normally be multiple receivers for a named message. Can we start out with just one (throw if you set more than one) and see whether anyone complains?
Comment 30 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-05-11 07:51:40 PDT
Comment on attachment 444535 [details] [diff] [review]
Support for strings-only messaging and script loading (take two).

bootstrap-user.js and bootstrap-impl.js are test code, right? Can we put them in js/jetpack/tests?
Comment 31 Atul Varma [:atul] 2010-05-11 13:43:26 PDT
This is awesome!

A few observations:

* I filed bug 565135, as it would be useful to be able to kill/shutdown Jetpack processes from the parent.

* I'm confused as to the rationale for nsIJetpack::loadUserScript() and the wrap(object) global in the implementation-jetpack context.  I had assumed that the jetpack-implementation code started by nsIJetpack::loadImplementation() would be responsible for creating a Components.utils.Sandbox() (or some equivalent) and then evaluating the user-script code in it (which would have been provided to it via a message from the parent).  This would allow the jetpack-implementation code to create multiple Sandbox instances, which is actually how modules are implemented in the current Jetpack platform (each module's scope is actually a Cu.Sandbox()).  It's also entirely possible for modules to not require chrome privileges, e.g. because they're just helper modules for the user's code, so I believe we actually *need* to be able to create new sandboxes/global scopes in the jetpack process.
Comment 32 Atul Varma [:atul] 2010-05-11 13:45:00 PDT
(In reply to comment #29)
> Hrm, I wasn't actually imagining that there would normally be multiple
> receivers for a named message. Can we start out with just one (throw if you set
> more than one) and see whether anyone complains?

I think that this should be okay as long as the developer's expectations are set accordingly.
Comment 33 Atul Varma [:atul] 2010-05-11 13:46:35 PDT
Also, for anyone who's watching this bug and interested in trying out the code, the patches here are to mozilla-central, not the electrolysis branch.
Comment 34 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-05-11 13:51:39 PDT
In jetpack-implementation there is no Components (and therefore no Components.utils.sandbox). The boostrap code basically creates the sandbox for you.

We could, instead of having a loadUserScript function, just have a createSandbox() function and code to eval something within that sandbox. Note that we'd like to move away from eval() if possible, in order to use fastload data and improve jetpack startup time in the future. That's why we had the more explicit load...(uri) methods.
Comment 35 Ben Newman (:bnewman) (:benjamn) 2010-05-11 13:55:37 PDT
(In reply to comment #30)
> (From update of attachment 444535 [details] [diff] [review])
> bootstrap-user.js and bootstrap-impl.js are test code, right? Can we put them
> in js/jetpack/tests?

I created the bootstrap-*.js files initially with the expectation that they would be for setting up things like require() and standard message receivers, but I ended up abusing bootstrap-impl.js to get the initial tests running.  I haven't actually come across anything I'd like to put in them permanently, so, yeah, I should definitely move the testing bits to js/jetpack/tests/unit.
Comment 36 Ben Newman (:bnewman) (:benjamn) 2010-05-11 19:37:02 PDT
Comment on attachment 443796 [details] [diff] [review]
Unbitrotted and with COMPONENT_LIBS fix.

>+JetpackParent::JetpackParent()
>+  : mSubprocess(new JetpackProcessParent())
>+{
>+  mSubprocess->Launch();
>+  Open(mSubprocess->GetChannel(),
>+       mSubprocess->GetChildProcessHandle());
>+}
>+
>+JetpackParent::~JetpackParent()
>+{
>+  XRE_GetIOMessageLoop()
>+    ->PostTask(FROM_HERE, new DeleteTask<JetpackProcessParent>(mSubprocess));
>+}

Doesn't JetpackParent need to add itself as an observer for the xpcom-shutdown event, a la ContentProcessParent?

http://hg.mozilla.org/projects/electrolysis/file/1c7b03e451a0/dom/ipc/ContentProcessParent.cpp#l64

While trying to add Handle support to Jetpack, I've found that ~JetpackParent gets called too late to delete the PJetpack subtree successfully.
Comment 37 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-05-11 19:38:51 PDT
Hrm. I think that the caller should ideally be responsible for cleaning up the jetpack when it is no longer needed or at shutdown (cleanup to be implemented in bug 565135).
Comment 38 Ben Newman (:bnewman) (:benjamn) 2010-05-11 19:52:00 PDT
Created attachment 444812 [details] [diff] [review]
WIP patch to port mozilla::jsipc::Handle from projects/electrolysis to mozilla-central.

So far I'm only exposing createHandle methods via nsIJetpack and JetpackChild::mImplCx.globalObject, so that (sub-)handles can be created by both the parent and the child.  No support yet for passing handles through messages.
Comment 39 Ben Newman (:bnewman) (:benjamn) 2010-05-11 19:53:42 PDT
Comment on attachment 444812 [details] [diff] [review]
WIP patch to port mozilla::jsipc::Handle from projects/electrolysis to mozilla-central.

Argh, wrong patch.
Comment 40 Ben Newman (:bnewman) (:benjamn) 2010-05-11 19:54:36 PDT
Created attachment 444813 [details] [diff] [review]
WIP patch to port mozilla::jsipc::Handle from projects/electrolysis to mozilla-central (take two).

Right (mozilla-central relative) patch this time.
Comment 41 Ben Newman (:bnewman) (:benjamn) 2010-05-12 16:57:36 PDT
Created attachment 445015 [details] [diff] [review]
Add support (and tests) for passing Handles through sendMessage.

These tests incidentally serve to validate the Handle implementation.
Comment 42 Ben Newman (:bnewman) (:benjamn) 2010-05-12 17:01:33 PDT
(In reply to comment #41)
> These tests incidentally serve to validate the Handle implementation.

To run the tests, execute this command from the object directory:

  make SOLO_FILE="test_jetpack.js" -C js/jetpack/tests check-interactive

and then type _execute_test() at the prompt.
Comment 43 Atul Varma [:atul] 2010-05-13 14:31:10 PDT
I may have mis-applied the patches, but so far I'm getting this error on 'make -f client.mk':

  > JetpackActorCommon.cpp
  > nsIJetpackService.idl
  > nsIJetpack.idl
  > ar: no archive members specified
  > usage:  ar -d [-TLsv] archive file ...
  > make[4]: *** [libjsipc_s.a] Error 1
  > make[3]: *** [libs_tier_platform] Error 2
  > make[2]: *** [tier_platform] Error 2
  > make[1]: *** [default] Error 2
  > make: *** [build] Error 2
Comment 44 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-05-17 12:46:58 PDT
Created attachment 445777 [details] [diff] [review]
Update process/thread goop to m-c latest
Comment 45 Ben Newman (:bnewman) (:benjamn) 2010-05-26 16:38:58 PDT
Created attachment 447643 [details] [diff] [review]
Support for arbitrary numbers of JSON-serializable arguments to sendMessage/callMessage.

Still passing the old tests, and a few new tests.  Probably needs more tests.

Error reporting could stand to be a lot better.  I'm going to work on that (and adding tests) before requesting review, but since this theoretically works now I thought I should get it out there.
Comment 46 Atul Varma [:atul] 2010-05-27 15:32:03 PDT
Which of the uploaded patches currently need to be applied to try out the functionality?  Could you please mark as 'obsolete' any of the ones that are superseded by subsequent patches, if there are any?
Comment 47 Ben Newman (:bnewman) (:benjamn) 2010-05-27 15:38:28 PDT
(In reply to comment #46)
> Which of the uploaded patches currently need to be applied to try out the
> functionality?  Could you please mark as 'obsolete' any of the ones that are
> superseded by subsequent patches, if there are any?

My patch queue is as follows:

bug-560643-part-3-support-v2.patch
556846-jetpack-bootstrap.patch
multiprocess-jetpack-api.diff
handle.diff
jetpack-variant.diff

The first patch is from the bug to add a jsval parameter type to IDL, which jorendorff has been working on.  I could work around it if need be, but it makes things a lot nicer on the nsIJetpack/JetpackParent side.  I think it should be landing soon.

The other four patches are the third, first, second, and fourth of the patches currently attached to this bug (in that order).
Comment 48 Ben Newman (:bnewman) (:benjamn) 2010-05-27 17:09:51 PDT
(In reply to comment #47)
> My patch queue is as follows:

Now committed to my patch queue repository (hosted on github):
http://github.com/benjamn/patches/tree/456d35c70e7f99733418f0c6f536ef9a86c54413
Comment 49 Atul Varma [:atul] 2010-05-27 17:19:15 PDT
Thanks! The patch queue repo was really helpful in particular.
Comment 50 Ben Newman (:bnewman) (:benjamn) 2010-05-27 18:34:43 PDT
Created attachment 447904 [details] [diff] [review]
More tests of serializable arguments.

Forgot to treat Arrays (which have JSTYPE_OBJECT) as arrays (instead of objects).
Comment 51 Ben Newman (:bnewman) (:benjamn) 2010-05-28 23:37:05 PDT
Created attachment 448180 [details] [diff] [review]
Support cyclic object references in sendMessage arguments.

Since we never actually have to serialize sendMessage arguments to JSON, the serialization format can be a little more complex.  With this patch, circular references are no longer forbidden.  In fact, the only remaining prohibition prevents serializing objects that have function properties.
Comment 52 Ben Newman (:bnewman) (:benjamn) 2010-06-01 17:21:58 PDT
Created attachment 448651 [details] [diff] [review]
Multiprocess Jetpack API, all in one patch, with tests.

The previous division of patches was more an accident of history than a logical progression of changes, so I've merged everything into one patch for ease of review.  I've flagged bsmedberg provisionally, but I wouldn't mind if anyone else wants to have a look at this.

You can run the tests by issuing the following command in your object directory:

  make -C js/jetpack/tests SOLO_FILE="unit/test_jetpack.js" check-interactive

and entering _execute_test() at the xpcshell prompt.
Comment 53 Ben Newman (:bnewman) (:benjamn) 2010-06-01 17:23:33 PDT
(In reply to comment #52)
> The previous division of patches was more an accident of history than a logical
> progression of changes, so I've merged everything into one patch for ease of
> review.

Note that I only merged my own patches.  There are still four patches that have to be applied for this all to work:

http://github.com/benjamn/patches
Comment 54 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-06-01 18:56:26 PDT
Created attachment 448665 [details]
Review comments

Review comments attached: r=me with everything addressed, or catch me on IRC if there are issues (the return type of callMessage is the only thing I think would be an issue). You'll need somebody to review the JSAPI bits also, I think, because I'm not sure about the rooting.
Comment 55 Ben Newman (:bnewman) (:benjamn) 2010-06-01 20:08:35 PDT
Comment on attachment 448665 [details]
Review comments

>> diff --git a/js/jetpack/PJetpack.ipdl b/js/jetpack/PJetpack.ipdl
>
>> +parent:
>> +  sync CallMessage(nsString messageName,
>> +                   Variant[] data)
>> +    returns (Variant[] results);
>
>It seems strange to me that CallMessage returns an array of variants. Can it return a single Variant, and the client can pass an array if they want to do structured returns?

Optionally passing an array is tricky because callMessage already takes an unlimited number of optional data arguments.

I think the second best option is to unpack the array when there's only one handler registered (the common case) but that seems like it would be surprising in cases when more than one handler is registered, and totally ambiguous if any of the handlers returned an array.

Crazy idea: could we forbid/ignore return values and use generator syntax to allow the parent to 'yield' results instead?

>> diff --git a/js/jetpack/jar.mn b/js/jetpack/jar.mn
>> new file mode 100644
>> --- /dev/null
>> +++ b/js/jetpack/jar.mn
>> @@ -0,0 +1,3 @@
>> +toolkit.jar:
>> +        content/global/jetpack/test-impl.js (tests/unit/impl.js)
>> +        content/global/jetpack/test-user.js (tests/unit/user.js)
>
>This isn't right: we shouldn't be shipping these as part of our default chrome: they should go in a test package instead.

Are there any examples of that elsewhere in the code base?  The toolkit.jar hack was the only way I could find of giving those files URIs that were visible within the xpcshell tests.
Comment 56 Benjamin Smedberg AWAY UNTIL 2-AUG-2016 [:bsmedberg] 2010-06-02 07:24:15 PDT
do_get_file(testdirRelativePath, allowNonexistent) will give you an nsIFile from which you can construct a file URI

As for the return type of CallMessage, did you decide that only allowing one receiver for called messages was a bad idea? (Also, I guess we're not dealing with exceptions thrown during called messages).

If you really want to allow multiple receivers, I guess the array of results is the only good solution: I can't imagine that a generator would be obvious, and people might think it was asynchronous, which is definitely not correct.
Comment 57 Ben Newman (:bnewman) (:benjamn) 2010-06-07 10:37:11 PDT
Created attachment 449655 [details] [diff] [review]
Catching up with bit-rot, and making Variant serialization more forgiving.

Besides accommodating some changes to the discipline for creating global objects in the JS engine, this patch makes mozilla::jetpack::Variant serialization infallible (besides failures in JSAPI functions, like OOM).  Properties and values that can't be serialized are simply dropped.  Function properties, in particular.  This is how things work with IndexedDB, and should make life easier for Jetpack implementers.
Comment 58 Ben Newman (:bnewman) (:benjamn) 2010-06-07 11:45:33 PDT
(In reply to comment #56)
> do_get_file(testdirRelativePath, allowNonexistent) will give you an nsIFile
> from which you can construct a file URI

This patch also uses do_get_file for that part of the test, which works real well.
Comment 59 Ben Newman (:bnewman) (:benjamn) 2010-06-10 14:56:52 PDT
Comment on attachment 449655 [details] [diff] [review]
Catching up with bit-rot, and making Variant serialization more forgiving.

When you get back from your bicycle trip, Ben T., think you could have a look over this?
Comment 60 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-06-17 23:13:42 PDT
Comment on attachment 449655 [details] [diff] [review]
Catching up with bit-rot, and making Variant serialization more forgiving.

>+  case JSTYPE_XML:
>+    // fall through

I'd either explicitly return here or put all your unhandled cases here together (e.g. JSTYPE_FUNCTION).

>+  OpaqueSeenType::IdType id;
>+  if (!seen->add(obj, &id)) {
>+    *to = CompVariant(id);

It looks like this will leave id uninitialized if rmap.AppendElement or map.add fail.

>+  if (JS_IsArrayObject(cx, obj)) {
>+    nsTArray<Variant> elems;
>+    jsuint len;
>+    if (!JS_GetArrayLength(cx, obj, &len))
>+      return false;

You should set the capacity of elems here to avoid lots of memcpy.

>+    for (jsuint i = 0; i < len; ++i) {
>+      jsval val;
>+      Variant* vp = elems.AppendElement();

If you don't set the capacity above then AppendElement could fail.

>+  nsTArray<KeyValue> kvs;

You should set the capacity again, or error check below.

>+    // Silently drop properties that can't be converted.
>+    if (jsval_to_Variant(cx, val, &kv.value(), seen)) {
>+      kv.key() = nsString((PRUnichar*)JS_GetStringChars(idStr),
>+                          JS_GetStringLength(idStr));

You want nsDependentString here.

>+          !JS_SetUCProperty(cx, obj,
>+                            kv.key().BeginReading(),
>+                            kv.key().Length(),
>+                            &val))

If you've got an nsString you can call .get() rather than BeginReading(), fyi.

>+    obj = js_NewArrayObjectWithCapacity(cx, vs.Length(), &elems);

Oh. Red flag. You can't use this API like that. You have to have everything ready to go before calling it because the buffer is left uninitialized. If you trigger a GC (like, in one of your recursive calls) then the collector will walk off into random memory. You certainly can't leave the buffer uninitialized and return early as the GC will surely run later. Just make a new array and eat the reallocation cost, I think. (I recently misused this API and got bitten by it).

>+    jsval rval;
>+    if (!JS_CallFunctionValue(cx, implGlobal, snapshot.ElementAt(i), argc, argv,
>+                              &rval))

I think code usually assumes that rval is rooted... Maybe AutoRoot before calling, not after. But ask the JS guys first to make sure I'm not crazy.

>+JetpackActorCommon::RegisterReceiver(JSContext* cx,

Don't you want to ensure that the jsval is a function?

>+  while (!mReceivers.Get(messageName, &vals))
>+    mReceivers.Put(messageName, new nsTArray<nsAutoJSValHolder>(1));

This seems... bad. One try should be enough!

>+    nsAutoJSValHolder* holder = vals->AppendElement();
>+    holder->Hold(cx);

This can fail iirc

>+  JetpackActorCommon() {
>+    mReceivers.Init();

This can fail.

>+JetpackChild::RecvSendMessage(const nsString& messageName,
>+                              const nsTArray<Variant>& data)
> {
>+  return JetpackActorCommon::RecvMessage(mImplCx, messageName, data, NULL);

You could save yourself the hassle of worrying about requests later if you wrap this in a request here, I think. Just look for all the places you use mImplCx or mUserCx.

>+  jsval v;
>+  JS_EvaluateScript(cx, JS_GetGlobalObject(cx), code.get(),
>+                    code.Length(), EmptyCString().get(), 1, &v);

How about s/EmptyCString().get()/""/

And maybe root the rval again.

>+  JSString* msgNameStr = JS_ValueToString(cx, argv[0]);
>+  if (!msgNameStr) {
>+    JS_ReportOutOfMemory(cx);

Actually I think this is wrong. It will make an uncatchable exception, which you don't want. Just use JS_ReportError("Couldn't convert to string").

>+  result->msgNameChars = (PRUnichar*)JS_GetStringChars(msgNameStr);
>+  result->msgNameLength = JS_GetStringLength(msgNameStr);

I think you might need to copy the string here... Can you guarantee that this jsval won't be collected before you use the string, even with the serialization that's about to happen? Why not just make MessageResult have an nsString that you load here?

>+JetpackChild::SendMessage(JSContext* cx, uintN argc, jsval* vp)
> ...
>+  JS_SET_RVAL(cx, vp, JSVAL_VOID);

I think this happens by default, and is unnecessary. But best to ask JS guys :)

>+    js_NewArrayObjectWithCapacity(cx, results.Length(), &rvals);

Need a new API here again.

>+  if (arity < 1)
>+    return JS_TRUE;

Doesn't look like this is possible?

>+  JSString* str = JS_ValueToString(cx, argv[0]);
>+  if (!str) {
>+    JS_ReportError(cx, "%s expects a string as its first argument", methodName);

Just checking, but JS_ValueToString actually converts to a string if possible. Is that what you want? The error message doesn't match.

>+  result->msgNameChars = (PRUnichar*)JS_GetStringChars(str);

Same worry about copying the string here.

>+  if (arity < 2)

Feels weird that you're checking arity here when argc is what you really care about (even though you ensured they're equal).

>+  result->receiver = argv[1];

This gave me pause, but it's GC safe because it still lives in argv and you don't use it outside of argv. Maybe comment?

>+JetpackChild::Wrap(JSContext* cx, uintN argc, jsval* vp)

NS_NOTYETIMPLEMENTED is noisier...

>+JetpackParent::SendMessage(const nsAString& aMessageName)
> ...
>+  nsTArray<Variant> data;

Set the capacity again. Oh, won't InfallibleTArray be great!

> JetpackService::CreateJetpack(nsIJetpack** aResult)
> ...
>+  nsCOMPtr<nsIJSContextStack>
>+    stack(do_GetService("@mozilla.org/js/xpc/ContextStack;1"));
>+  JSContext* cx;
>+  nsresult rv = stack->Peek(&cx);

Hm. Ask jst if this is right? I would imagine that you want to get the native call context, and get the cx from there. Not sure Peek is guaranteed to always return non-null.
Comment 61 Ben Newman (:bnewman) (:benjamn) 2010-06-21 12:06:59 PDT
Created attachment 452806 [details] [diff] [review]
Addressing bent's feedback.

Interdiff:
http://github.com/benjamn/patches/blob/1f85425a8c/address-bent-feedback.diff

Salient responses (everything else fixed as advised):

> >+  OpaqueSeenType::IdType id;
> >+  if (!seen->add(obj, &id)) {
> >+    *to = CompVariant(id);
> 
> It looks like this will leave id uninitialized if rmap.AppendElement or map.add
> fail.

Nice catch.

> >+  nsTArray<KeyValue> kvs;
> 
> You should set the capacity again, or error check below.

This is an interesting case because the position of elements in the array is unimportant.  If the AppendElement call fails, the property will just be dropped, which seems like a reasonable failure mode for a very rare case (OOM).  I added a comment to that effect.

> >+    obj = js_NewArrayObjectWithCapacity(cx, vs.Length(), &elems);
> 
> Oh. Red flag. You can't use this API like that. You have to have everything
> ready to go before calling it because the buffer is left uninitialized. If you
> trigger a GC (like, in one of your recursive calls) then the collector will
> walk off into random memory. You certainly can't leave the buffer uninitialized
> and return early as the GC will surely run later. Just make a new array and eat
> the reallocation cost, I think. (I recently misused this API and got bitten by
> it).

Great catch!

> >+  jsval v;
> >+  JS_EvaluateScript(cx, JS_GetGlobalObject(cx), code.get(),
> >+                    code.Length(), EmptyCString().get(), 1, &v);
> 
> How about s/EmptyCString().get()/""/
> 
> And maybe root the rval again.

That rval is completely ignored, so we don't care, do we?  In my updated patch, I changed "v" to "ignored", for what it's worth.

> >+  if (arity < 2)
> 
> Feels weird that you're checking arity here when argc is what you really care
> about (even though you ensured they're equal).

The point is that the arity is known for each of those API methods, and we want to catch inconsistencies between known arity and observed argc.

> > JetpackService::CreateJetpack(nsIJetpack** aResult)
> > ...
> >+  nsCOMPtr<nsIJSContextStack>
> >+    stack(do_GetService("@mozilla.org/js/xpc/ContextStack;1"));
> >+  JSContext* cx;
> >+  nsresult rv = stack->Peek(&cx);
> 
> Hm. Ask jst if this is right? I would imagine that you want to get the native
> call context, and get the cx from there. Not sure Peek is guaranteed to always
> return non-null.

Still need to ask jst if this is right.
Comment 62 Ben Newman (:bnewman) (:benjamn) 2010-06-21 12:18:08 PDT
Created attachment 452813 [details] [diff] [review]
Addressing all of bent's feedback.

(In reply to comment #61)
> > Hm. Ask jst if this is right? I would imagine that you want to get the native
> > call context, and get the cx from there. Not sure Peek is guaranteed to always
> > return non-null.
> 
> Still need to ask jst if this is right.

Now getting the native call context.
Comment 63 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-06-21 13:57:42 PDT
Comment on attachment 452813 [details] [diff] [review]
Addressing all of bent's feedback.

>+class Handle
> ...
>+  JSObject* ToJSObject(JSContext* cx) const {

By the way, why do this as a const and then force yourself to have mutable members? Did this fix something somewhere?

>+      // Nulling out mObj effectively unroots the object, but we still
>+      // need to remove the root for good hygiene's sake.

Not just for hygiene, the jseng will yell at you if you don't.

>+  Finalize(JSContext* cx, JSObject* obj) {
>+    Handle* self = Unwrap(cx, obj);
>+    self && Send__delete__(self);

Add a comment here saying what you're doing and why.

>+  bool add(KeyType obj, IdType* id) {
>+    MapType::AddPtr ap = map.lookupForAdd(obj);
>+    if (!ap) {
>+      *id = rmap.Length();
>+      if (!rmap.AppendElement(obj) ||
>+          !map.add(ap, obj, rmap.Length() - 1))
>+        *id = kInvalidId;

Looks like you test id below to be non-null... Are you requiring id now? One or the other needs to be changed.

>+  bool reverseLookup(IdType id, KeyType* objp) {
>+    return !!(*objp = rmap.SafeElementAt(id, NULL));
>+  }

You could just make this return KeyType* instead of the weird bool return + KeyType out param.

.get() here instead of BeginReading()

>+    const nsTArray<KeyValue>& kvs = from.get_ArrayOfKeyValue();
>+    for (PRUint32 i = 0; i < kvs.Length(); ++i) {
>+      const KeyValue& kv = kvs.ElementAt(i);
>+      jsval val;

Hm. I think you need to root val here. If GC is triggered by JS_SetUCProperty after jsval_from_Variant creates something new then this could be GCd.

>+  case CompVariant::TArrayOfVariant: {
>+    const nsTArray<Variant>& vs = from.get_ArrayOfVariant();
>+    nsAutoTArray<jsval, 8> jsvals;
>+    jsval* elems = jsvals.AppendElements(vs.Length());
>+    if (!elems)
>+      return false;
>+    js::AutoArrayRooter root(cx, vs.Length(), elems);

Uh oh, you can't do this. AppendElements will create undefined jsvals and then rooting the array will tell the GC to walk them. You either need to specialize nsTArrayElementTraits for jsval or you need to manually initialize the elements before you do anything that could trigger GC.

>+  js::AutoValueRooter rval(cx, JSVAL_VOID);

AutoValueRooter will do JSVAL_VOID automatically for you, fyi.

>+  for (PRUint32 i = 0; i < snapshot.Length(); ++i) {
>+    if (!JS_CallFunctionValue(cx, implGlobal, snapshot.ElementAt(i), argc, argv,
>+                              rval.addr()))
>+      break;

Hm. Is there a TODO somewhere for what to do if receivers throw exceptions? You could clear the exception and keep calling the others, or send a message about the exception, or something. But just stopping seems weird.

Also, you should probably reset rval to JSVAL_VOID before each call to JS_CallFunctionValue or else you could leak stuff from one receiver to the other.

>+Evaluate(JSContext* cx, const nsCString& code)
>+{
>+  JSAutoRequest request(cx);
>+  jsval ignored = JSVAL_VOID;
>+  JS_EvaluateScript(cx, JS_GetGlobalObject(cx), code.get(),
>+                    code.Length(), "", 1, &ignored);

Even though you don't use it you need to root the rval. JSEng assumes it is rooted. Just use the autorooter trick.

>+  result->msgName =
>+    nsDependentString((PRUnichar*)JS_GetStringChars(msgNameStr),
>+                      JS_GetStringLength(msgNameStr));

This doesn't copy either. I think you need to do |result->msgName.Assign(JS_GetStringChars, JS_GetStringLength)|

>+    Variant* vp = result->data.AppendElement();

That can fail.

>+  JSObject* arrObj = JS_NewArrayObject(cx, results.Length(), rvals);

This could return NULL, in which case I think you want to return JS_FALSE.

>+  result->msgName =
>+    nsDependentString((PRUnichar*)JS_GetStringChars(str),
>+                      JS_GetStringLength(str));

Same problem here re:copying

>+  JSObject* hobj = handle->ToJSObject(cx);
>+  JS_SET_RVAL(cx, vp, OBJECT_TO_JSVAL(hobj));

Do you want to throw (i.e. return JS_FALSE) if hobj is null here?

>+ReadFromURI(const nsAString& aURI,
> ...
>+  channel->Open(getter_AddRefs(input));
>+  if (input) {

I would think you'd want to return an error here if Open fails (rather than NS_OK). What do you think?

>+JetpackParent::SendMessage(const nsAString& aMessageName)

I think you need a JSAutoRequest here before you call jsval_to_Variant.

>+JetpackParent::CreateHandle(nsIVariant** aResult)
> ...
>+  JSObject* hobj = handle->ToJSObject(mContext);

Again, throw if null?
Comment 64 Ben Newman (:bnewman) (:benjamn) 2010-06-22 15:03:46 PDT
Created attachment 453189 [details] [diff] [review]
Addressing feedback and fixing a bug.
Comment 65 Ben Newman (:bnewman) (:benjamn) 2010-06-22 15:04:21 PDT
Created attachment 453190 [details] [diff] [review]
Interdiff from last-reviewed patch.

JFYI
Comment 66 Ben Newman (:bnewman) (:benjamn) 2010-06-22 15:10:05 PDT
Comment on attachment 453190 [details] [diff] [review]
Interdiff from last-reviewed patch.

(In reply to comment #64)
> Created an attachment (id=453189) [details]
> Addressing feedback and fixing a bug.

>-  nsClassHashtable<nsStringHashKey,
>-                   nsTArray<nsAutoJSValHolder> > mReceivers;
>+  nsClassHashtable<nsStringHashKey, RecList> mReceivers;

This was the crux of the bug that I fixed in addition to addressing Ben T.'s feedback.  nsAutoJSValHolders don't survive memcpy'ing when nsTArray reallocates.


(In reply to comment #63)
> By the way, why do this as a const and then force yourself to have mutable
> members? Did this fix something somewhere?

To my way of thinking, ToJSObject is conceptually const.  More importantly, it's a pure function, and the mObj and mRuntime members are just used to cache the result.  That's a reasonable use for the mutable keyword, I think.

> Looks like you test id below to be non-null... Are you requiring id now? One or
> the other needs to be changed.

Oversight, no need for that check.  I'm requiring id now.

> Uh oh, you can't do this. AppendElements will create undefined jsvals and then
> rooting the array will tell the GC to walk them. You either need to specialize
> nsTArrayElementTraits for jsval or you need to manually initialize the elements
> before you do anything that could trigger GC.

Manually initializing.

> >+  js::AutoValueRooter rval(cx, JSVAL_VOID);
> 
> AutoValueRooter will do JSVAL_VOID automatically for you, fyi.
> 
> >+  for (PRUint32 i = 0; i < snapshot.Length(); ++i) {
> >+    if (!JS_CallFunctionValue(cx, implGlobal, snapshot.ElementAt(i), argc, argv,
> >+                              rval.addr()))
> >+      break;
> 
> Hm. Is there a TODO somewhere for what to do if receivers throw exceptions? You
> could clear the exception and keep calling the others, or send a message about
> the exception, or something. But just stopping seems weird.

Clearing the exception.  This was a little tricky (needed to set JSOPTION_DONT_REPORT_UNCAUGHT in addition to clearing the exception).

> >+ReadFromURI(const nsAString& aURI,
> > ...
> >+  channel->Open(getter_AddRefs(input));
> >+  if (input) {
> 
> I would think you'd want to return an error here if Open fails (rather than
> NS_OK). What do you think?
> 
> >+JetpackParent::SendMessage(const nsAString& aMessageName)
> 
> I think you need a JSAutoRequest here before you call jsval_to_Variant.

It's an IDL method that can only be called from script, so a request must already have been started.  I think?
Comment 67 Ben Turner (not reading bugmail, use the needinfo flag!) 2010-06-22 15:33:27 PDT
Comment on attachment 453189 [details] [diff] [review]
Addressing feedback and fixing a bug.

Looks good!
Comment 69 Ben Newman (:bnewman) (:benjamn) 2010-06-25 17:07:40 PDT
Resolving as fixed; will file follow-up bug to re-enable the tests.
Comment 70 Atul Varma [:atul] 2010-07-26 15:46:51 PDT
*** Bug 557340 has been marked as a duplicate of this bug. ***
Comment 71 Atul Varma [:atul] 2010-07-26 15:47:36 PDT
*** Bug 557259 has been marked as a duplicate of this bug. ***
Comment 72 Atul Varma [:atul] 2010-07-26 15:49:03 PDT
*** Bug 565135 has been marked as a duplicate of this bug. ***
Comment 73 Atul Varma [:atul] 2010-09-20 15:04:46 PDT
I've documented these additions to the platform on MDC here:

  https://developer.mozilla.org/en/nsIJetpack
  https://developer.mozilla.org/en/nsIJetpackService
  https://developer.mozilla.org/en/Jetpack_Processes

Note You need to log in before you can comment on or make changes to this bug.