Closed Bug 556846 Opened 14 years ago Closed 14 years ago

Investigate multi-process Jetpack

Categories

(Mozilla Labs :: Jetpack Prototype, defect)

Product:
Mozilla Labs
Component:
Jetpack Prototype
Platform:
x86
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: jdm, Assigned: mozilla+ben)

References

Details

Attachments

(4 files, 16 obsolete files)

Update process/thread goop to m-c latest
43.83 KB, patch
Details | Diff | Splinter Review
Review comments
3.18 KB, text/plain
Details
Addressing feedback and fixing a bug.
70.40 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
Interdiff from last-reviewed patch.
20.82 KB, patch
Details | Diff | Splinter Review 
Using the new electrolysis technologies, a prototype of the prototype should be created to figure out some of the unknowns we'll run into.
I will upload my prototype changes shortly.  It depends on a few open bugs with uncommitted patches.
Depends on: 545237, 546355, 548904
Areas of concern are documented at http://wiki.mozilla.org/Electrolysis/Jetpack
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.
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.
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>?
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.
https://wiki.mozilla.org/Electrolysis/Jetpack
Assignee: nobody → benjamin
Status: NEW → ASSIGNED
Attachment #443187 - Flags: review?(mozilla+ben)
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 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?
Attachment #443187 - Flags: review?(mozilla+ben) → review+
I'm sure we can use macs. We will have to disable IPC plugins on ppc at least, but I think that's possible.
No, PJetpack is the top of the hierarchy, we won't need a PJetpackProcess.
Any suggestions about how to load URIs without using XPCOM?  I don't really want to reimplement nsIURI.
My thought was to open the stream in the chrome process and just ship the actual script data over the wire.
(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)).
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.
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?
Attachment #443187 - Attachment is obsolete: true
Attachment #443340 - Flags: review?(jones.chris.g)
Depends on: 563747
Assignee: benjamin → mozilla+ben
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.
(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
Hrm, then I missed a refresh: there should be an IS_COMPONENT.

        Attachment #443340 -
        Attachment is obsolete: true

        Attachment #443340 -
        Flags: review?(jones.chris.g)

    
    

    
  
(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

    
    

    
  
Still getting this (after rebasing to trunk)

make[4]: *** No rule to make target `../../staticlib/libjetpack_s.a', needed by `libxul.so'.  Stop.

    
    

    
  
Looks like this was the missing secret sauce

ifdef MOZ_IPC
COMPONENT_LIBS +=  jetpack_s
endif

    
    

    
  
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.

        Attachment #443772 -
        Flags: review+

    
    

    
  
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?

    
    

    
  


  
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).

        Attachment #444529 -
        Flags: feedback?(benjamin)

    
    

    
  


  
Forgot to save js/jetpack/bootstrap-impl.js, thus neglecting the [0] after callMessage.

        Attachment #444529 -
        Attachment is obsolete: true

        Attachment #444535 -
        Flags: feedback?(benjamin)

        Attachment #444529 -
        Flags: feedback?(benjamin)

    
    

    
  
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 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?

        Attachment #444535 -
        Flags: feedback?(benjamin) → feedback+

    
  

        Attachment #443772 -
        Attachment is obsolete: true

    
  
Depends on: 565135

    
    

    
  
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.

    
    

    
  
(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.

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  
(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 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.

    
    

    
  
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).

    
    

    
  


  
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 on attachment 444812 [details] [diff] [review]
WIP patch to port mozilla::jsipc::Handle from projects/electrolysis to mozilla-central.

Argh, wrong patch.

        Attachment #444812 -
        Attachment is obsolete: true

    
    

    
  


  
Right (mozilla-central relative) patch this time.

    
    

    
  


  
These tests incidentally serve to validate the Handle implementation.

        Attachment #444813 -
        Attachment is obsolete: true

    
    

    
  
(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.

    
    

    
  
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

    
    

    
  


  
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.

    
  

        Attachment #447643 -
        Attachment description: Support for arbitrary numbers of JSON-serializable variants in sendMessage/callMessage. → Support for arbitrary numbers of JSON-serializable arguments to sendMessage/callMessage.

    
  

        Attachment #447643 -
        Attachment description: Support for arbitrary numbers of JSON-serializable arguments to sendMessage/callMessage. → Support for arbitrary numbers of JSON-serializable arguments to sendMessage/callMessage.

    
    

    
  
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?

    
    

    
  
(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).

    
  
Blocks: 568695

    
  
Blocks: 568698

    
    

    
  
(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

    
    

    
  
Thanks! The patch queue repo was really helpful in particular.

    
    

    
  


  
Forgot to treat Arrays (which have JSTYPE_OBJECT) as arrays (instead of objects).

        Attachment #447643 -
        Attachment is obsolete: true

    
    

    
  


  
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.

    
  
Blocks: 567703

    
    

    
  


  
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.

        Attachment #444535 -
        Attachment is obsolete: true

        Attachment #445015 -
        Attachment is obsolete: true

        Attachment #447904 -
        Attachment is obsolete: true

        Attachment #448180 -
        Attachment is obsolete: true

        Attachment #448651 -
        Flags: review?(benjamin)

    
    

    
  
(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

    
    

    
  

      
      
      
      

        Attached file
          
        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.

    
  

        Attachment #448651 -
        Flags: review?(benjamin) → review+

    
    

    
  
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.

    
    

    
  
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.

    
    

    
  


  
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.

        Attachment #448651 -
        Attachment is obsolete: true

    
    

    
  
(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 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?

        Attachment #449655 -
        Flags: review?(bent.mozilla)

    
    

    
  
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.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Addressing bent's feedback. (obsolete)
        — 
          — Splinter Review
      

    


  
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.

        Attachment #449655 -
        Attachment is obsolete: true

        Attachment #449655 -
        Flags: review?(bent.mozilla)

    
    

    
  


  
(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.

        Attachment #452806 -
        Attachment is obsolete: true

        Attachment #452813 -
        Flags: review?(bent.mozilla)

    
    

    
  
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?

    
    

    
  


  

        Attachment #452813 -
        Attachment is obsolete: true

        Attachment #453189 -
        Flags: review?(bent.mozilla)

        Attachment #452813 -
        Flags: review?(bent.mozilla)

    
    

    
  
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?

        Attachment #453189 -
        Flags: review?(bent.mozilla) → review+

    
    

    
  
Comment on attachment 453189 [details] [diff] [review]
Addressing feedback and fixing a bug.

Looks good!

    
  
Depends on: 560643

    
  
Blocks: 574039

    
    

    
  
Resolving as fixed; will file follow-up bug to re-enable the tests.
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED

    
  
Blocks: 574870

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: