ubi::Node should have a |JSCompartment *ubi::Node::compartment()| method that returns nullptr for things that aren't associated with a compartment. From irc: 10:54 < jimb> fitzgen: Okay. Well, then, I think we want to put 'JSCompartment *compartment() const { return nullptr; }' in TracerConcrete, and then override it in the specializations of ubi::Concrete for the things that do have compartments.

https://tbpl.mozilla.org/?tree=Try&rev=ba7d0d1ecf9c This isn't as nice as I'd hoped it would be. It turns out that I had to specialize a constructor and the construct method in addition to just the compartment method for each Concrete<type> with a compartment, or else we were just calling TracerConcrete's construct method which creates a TracerConcrete instance and we end up calling TracerConcrete::compartment. Perhaps there is a better way?

Rebased on top of the new revision of ubi::Node. Clearing review for now, until I reimplement this with the curiously recurring template pattern.

This patch manages to DRY it up a bunch more. https://tbpl.mozilla.org/?tree=Try&rev=1f171c215ab3

Seems like I'm hitting some SPS profiler assertion failures?! ------------------------------------------------------------------------ Assertion failure: js(), at ../../dist/include/js/ProfilingStack.h:64 Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000 js::ProfileEntry::script (this=0x1018fbf30) at ProfilingStack.h:64 (gdb) bt #0 js::ProfileEntry::script (this=0x1018fbf30) at ProfilingStack.h:64 #1 0x00000001008a3ba6 in js::SPSProfiler::updatePC (this=0x103008a48, script=0x102e46380, pc=0x10280cc93 ":") at SPSProfiler.h:183 #2 0x000000010026c147 in js::jit::BailoutIonToBaseline (cx=0x102a01150, activation=0x7fff5fbfdb30, iter=@0x7fff5fbfd720, invalidate=true, bailoutInfo=0x7fff5fbfd8c8, excInfo=0x0) at BaselineBailouts.cpp:1435 #3 0x000000010026c4fd in js::jit::InvalidationBailout (sp=0x7fff5fbfd8d8, frameSizeOut=0x7fff5fbfd8d0, bailoutInfo=0x7fff5fbfd8c8) at Bailouts.cpp:128 #4 0x0000000102c06447 in ?? () Previous frame inner to this frame (gdb could not unwind past this frame)

Comment on attachment 8424067 [details] [diff] [review] ubi-node-compartment.patch Canceling review because that is a sad try push :(

From https://tbpl.mozilla.org/php/getParsedLog.php?id=39836635&tree=Try#error0 > TEST-UNEXPECTED-FAIL | check_vanilla_allocations.py | 'operator new(unsigned' present at UbiNode.cpp:202 UbiNode.cpp:202 is: > mozilla::ScopedDeletePtr<SimpleEdgeRange> r(new SimpleEdgeRange(cx)); inside TracerConcrete<Derived, Referent>::edges. I'm not sure what check_vanilla_allocations.py is checking for, or why it started complaining about this line that I didn't change (just changed the template to take a new class parameter that isn't used in this function). Any idea what's going on here, Jim?

Yeah, that should be js_new<SimpleEdgeRange>(cx), and similar for the other allocations. Sorry; should've caught that.

This does the s/new/js_new/ that the original ubi::Node patch will need to do before landing. Just trying to get a new try push, that's hopefully a little greener. https://tbpl.mozilla.org/?tree=Try&rev=93d3c681ba41

Comment on attachment 8424234 [details] [diff] [review] ubi-node-compartment.patch Review of attachment 8424234 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/public/UbiNode.h @@ +12,2 @@ > #include "jspubtd.h" > +#include "jsscript.h" We can't cite these internal headers from a js/public header.

Here's a re-implementation that doesn't use macros. Includes some jsapi-tests. It's tangled up with some other cleanups, but I just wanted to get this out there.

Surely cursed try push: https://tbpl.mozilla.org/?tree=Try&rev=b83d67f2c29b

This seems ready for review.

Comment on attachment 8462950 [details] [diff] [review] Implement ubi::Node::compartment. Review of attachment 8462950 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsapi-tests/testUbiNode.cpp @@ +5,5 @@ > +#include "js/UbiNode.h" > + > +#include "jsapi-tests/tests.h" > + > +using namespace JS; Please remove this, as for the Zone patch. Sorry I didn't catch this the first time through.

(In reply to Terrence Cole [:terrence] from comment #14) > Please remove this, as for the Zone patch. Sorry I didn't catch this the > first time through. Thanks for checking; I'd already taken it out, but didn't update the patch here.