1474383 - JSRuntime needs a hook to construct ubi::Nodes specialized for nsINode instances

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Kris Wright :KrisWright]

Some JSObjects are actually reflector objects to DOM elements, but ubi::Node cannot recognize their outgoing edges within Spidermonkey. We want to create a JSRuntime hook that exposes nsINode edges to ubi::Node by delegating JSObject construction through the hook to a Concrete<JSObject> subclass that can recognize its nsISupports and query the interface for an additional nsINode edge.

Comment 1

[Kris Wright :KrisWright]

Attachment: JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Created a runtime hook as well as a new CoarseType value to handle DOM nodes. Specialized ubi::Concrete for several nsINode types. Displayed outgoing nsISupports* edges on reflector JSObjects. Generated outgoing child edges from nsINodes by examining their children. Updated files to take census with the new CoarseType.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8990855 [details] [diff] [review]
JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

This is looking really good!  There are a bunch of nits and comments and suggestions below, but overall this is very solid.

>+++ b/dom/base/moz.build
>+    'nsINodeUbiReporting.h',

Generally, files (and classes) with names starting with "nsI" are assumed to be interfaces implemented by objects.

It's probably better to name this file "NodeUbiReporting.h" and similarly for the .cpp file.

>+++ b/dom/base/nsIContent.h
>+  void constructUbiNode(void* storage) override;

Local style is that methods are in InitialCaps.  Please name this ConstructUbiNode, same elsewhere.

>+++ b/dom/base/nsINodeUbiReporting.cpp
>+typedef JS::ubi::SimpleEdgeRange SimpleEdgeRange;
>+typedef JS::ubi::EdgeRange EdgeRange;

Might be better to do:

  using JS::ubi::SimpleEdgeRange;

and similar for EdgeRange.

>+JS::ubi::Concrete<nsINode>::construct(void* storage, nsINode* ptr) {
>+  ptr->constructUbiNode(storage);
>+}

I guess we need this extra level of virtual dispatch because we want to be able to call the ubi::Node constructor to construct-as-value in XPCJSRuntime and in our edge construction below?  Worth documenting.

Opening curly should be on its own line, not at end of previous line.

>+js::UniquePtr<EdgeRange>
>+JS::ubi::Concrete<nsINode>::edges (JSContext* cx, bool wantNames) const {

Please drop the space after "edges" and move the opening curly to its own line.

>+  char16_t* edgeName;

Please declare as close to use as you can.  In this case, inside the loop.

>+  auto range = js::MakeUnique<SimpleEdgeRange>();
>+  if (!range || !ptr) {

Can ptr actually be null here?  How would that happen?  If it can happen, please document whatever non-obvious circumstances are involved.

>+  if (!get().GetFirstChild())
>+    return range;

Why do we need this check up front, or the loop index bit?  Seems like we could basically do:

  for (nsIContent* curr = get().GetFirstChild();
       curr;
       curr = curr->GetNextSibling()) {
    // Add stuff to the range here.
  }

>+    edgeName = NS_strdup(u"nsINode");

This is pretty unfortunate.  Technically, things allocated via NS_strdup should not be freed via js_free, which is what ubi::Edge will try to do with this pointer.  But the JS engine doesn't seem to expose a function that will do the right thing here (e.g. js::DuplicateString).  For now this is OK, because Gecko and Spidermonkey happen to share an allocator, but long-term it's a problem.  Please file a followup to get this fixed and make it block bug 1243367.

As far as the edge name itself goes, I think "child node" or "Child Node" or "child Node" or something might be better.

Nodes keep their parents alive.  We include an edge to the parent here as well.

Should we be paying attention to the wantNames bit somewhere here?

>+JS::ubi::Node::Size
>+JS::ubi::Concrete<nsINode>::size (mozilla::MallocSizeOf mallocSizeOf) const {

Drop space before '(', curly on own line.

>+  get().AddSizeOfExcludingThis(wn, &n);

Why excluding this?  I'd think we want to include this.

>+  if (n > 0)
>+    return n;
>+  return 1;

And then we can just "return n;" without worrying about it being 0.

>+const char16_t*
>+JS::ubi::Concrete<nsINode>::getTypeName () const{

Formatting.

>+  const char16_t* chars = get().NodeName().get();

As discussed, this doesn't follow the contract for getTypeName(), which promises to return the static concreteTypeName in all cases.

>+js::UniquePtr<EdgeRange>
>+JS::ubi::Concrete<nsIContent>::edges (JSContext* cx, bool wantNames) const

As discussed, it might be best for these Concrete structs to inherit from each other so that we can just implement "edges" for nsINode and have it get picked up automatically here instead of needing to keep them in sync.

Similar for documents.

>+JS::ubi::Node::Size
>+JS::ubi::Concrete<nsIContent>::size (mozilla::MallocSizeOf mallocSizeOf) const {

This could be inherited too.

>+JS::ubi::Concrete<nsIDocument>::edges (JSContext* cx, bool wantNames) const {

Could be inherited.

>+JS::ubi::Concrete<nsIDocument>::size (mozilla::MallocSizeOf mallocSizeOf) const {

Formatting (spacing, curly).

>+  get().DocAddSizeOfExcludingThis(wn);

DocAddSizeOfIncludingThis()

>+nsINode::constructUbiNode (void* storage) {

Formatting.

Further, I would expect the concrete ConstructUbiNode functions to appear in the .cpp files for the relevant classes (nsDocument.cpp for nsIDocument, FragmentOrElement.cpp for nsIContent, nsINode.cpp for nsINode, but see more on that part below).

>+++ b/dom/base/nsINodeUbiReporting.h

Right after the include guards, there should be a comment like so:

  /**
   * Describe what the point of this file is.
   */

>+  static void actuallyConstruct(void *storage, nsINode *ptr) { new (storage) Concrete (ptr); }

I think it might make sense to have the Concrete<nsINode> be an abstract class, with three subclasses: Concrete<nsIDocument>, Concrete<nsIContent>, Concrete<Attr>.  Those are the only subclasses of nsINode, and any concrete nsINode instance is actually an instance of one of those subclasses.

Shared bits like edges() and size() could go on Concrete<nsINode>.  Each subclass would have its own typeName().  Concrete<nsINode>::construct would call the virtual nsINode method; the three subclasses would just have construct() methods that do the "new (storage) Concrete(ptr);" thing.

>+++ b/js/public/UbiNode.h
>+struct ConstructUbiNodeForWrappedNativeCallback {
>+  virtual void Construct (void* storage, JSObject* ptr) = 0;
>+};
>+
>+JS_PUBLIC_API(bool) SetConstructUbiNodeForWrappedNativeCallback (JSContext* cx, ConstructUbiNodeForWrappedNativeCallback* cb);

Those bits need more documentation.  In particular, that this callback will be invoked for objects which have an isDOMClass() JSClass.  And then perhaps the callback should be called ConstructUbiNodeForDOMObjectCallback.

It's also not clear to me why we're using a pointer to struct with a virtual method here.  All we really need is a function pointer.

>+++ b/js/src/vm/UbiNode.cpp
>+Concrete<JSObject>::construct (void* storage, JSObject* ptr)

Why the space before '('?

>+        new (storage) Concrete (ptr);

Why the space after "Concrete"?

>+    } else {

Might be better to early-return in the "if" and outdent here.

>+        //JSObjects may hold hidden pointers to nsINodes. If that is the case we want to expose those.

Well, JSObjects may hold hidden pointers to DOM objects of all sorts.  As far as this code is concerned, that's all that matters.

>+SetConstructUbiNodeForWrappedNativeCallback (JSContext* cx, ConstructUbiNodeForWrappedNativeCallback* cb){

Formatting?

>+SetConstructUbiNodeForWrappedNativeCallback (JSContext* cx, ConstructUbiNodeForWrappedNativeCallback* cb){

Formatting.

>+    if(!cb || !cx){

Space after "if", please.

>+        cx->runtime()->constructUbiNodeForWrappedNativeCallback = nullptr;


This seems like an odd thing to do if !cx.

In general, I would expect this method to assume cx is non-null, return void, and just set the callback to the provided function pointer, like all the other callback bits.

I skipped over the census bits; I assume Jim will review those.

>+++ b/js/xpconnect/src/XPCJSRuntime.cpp
>+  static void construct(void *storage, JSObject *ptr) { new (storage) ReflectorNode (ptr); }

Please nix the space after "ReflectorNode"

>+typedef JS::ubi::SimpleEdgeRange SimpleEdgeRange;
>+typedef JS::ubi::EdgeRange EdgeRange;

Why not keep the "namespace JS { namespace ubi {" block open through until after you define the edges function?  Would make more sense to me than adding these typedefs and writing out the namespace in "JS::ubi::ReflectorNode::edges".

>+JS::ubi::ReflectorNode::edges (JSContext* cx, bool wantNames) const {

Formatting (space before '(', curly location).


>+    auto range = js::UniquePtr<SimpleEdgeRange>(static_cast<SimpleEdgeRange*>(
>+                                                Concrete<JSObject>::edges(cx, wantNames).release()));

This seems like a somewhat gratuitous use of auto.  Might be simpler as:

  js::UniquePtr<SimpleEdgeRange> range(
    static_cast<SimpleEdgeRange*>(Concrete<JSObject>::edges(cx, wantNames).release()));

And maybe file a followup bug to get a downcast() of some sort added to UniquePtr.

The other concern here is that this is assuming that Concrete<JSObject>::edges will keep returning a SimpleEdgeRange.  What guarantees that?  We either need to document in TracerConcrete<Referent>::edges and Concrete<JSObject> that we rely on this invariant,  or we should add a method on TracerConcrete<Referent> that returns a UniquePtr<SimpleEdgeRange> and call it both here and from TracerConcrete<Referent>::edges.  I would prefer the latter solution, I think, but check with Jim.

>+    //after adding tracer edges from JSObject we will worry about the other edges.

How about:

  // Now add the edges that the JS engine does not already know about.

?

>+    nsISupports* stuff = UnwrapDOMObjectToISupports(&get());

"supp" might be a more common name for this, or "supports".

>+    if(stuff) {

Probably nicer to do:

  if (!stuff) {
      return ranges;
  }

and then outdent the rest.

>+        char16_t* edgeName = NS_strdup(u"Reflected nsINode");
>+        nsCOMPtr<nsINode> node = do_QueryInterface(stuff);
>+        if (!node)
>+            return range;

That leaks "edgeName".  Which is best solved by moving the declaration and creation of edgeName to where it's actually needed.

Also, curlies around the if body, please.

>+        if (!range->addEdge(JS::ubi::Edge(edgeName, JS::ubi::Node(node.get()))))

Do you need the node.get() here?  I wouldn't think you do; Node's constructor takes a pointer, so the conversion operator for nsCOMPtr to pointer should do the right thing.

Alternately, you could leave the .get() but nix the Node constructor, since it's implicit:

        if (!range->addEdge(JS::ubi::Edge(edgeName, node.get())))

but I think I prefer the explicit Node constructor without the .get().

Also, this whole file is "using namespace JS" so no need for "JS::" prefixes here.  This applies throughout this file.

Should we be paying any attention to the wantNames arg?

>+/* Callback for creating ubi::Nodes reporting from DOM node objects */

"representing DOM node objects"?

>+struct ConstructUbiNodeForWrappedNativeCallback : JS::ubi::ConstructUbiNodeForWrappedNativeCallback {

Curly on next line.  Though per comments above this will probably change anyway.

Comment 3

[Kris Wright :KrisWright]

Attachment: JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Created a runtime hook as well as a new CoarseType value to handle DOM nodes. Specialized ubi::Concrete for several nsINode types. Displayed outgoing nsISupports* edges on reflector JSObjects. Generated outgoing child edges from nsINodes by examining their children. Updated files to take census with the new CoarseType.

Comment 4

[Kris Wright :KrisWright]

Attachment: JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Created a runtime hook as well as a new CoarseType value to handle DOM nodes. Specialized ubi::Concrete for several nsINode types. Displayed outgoing nsISupports* edges on reflector JSObjects. Generated outgoing child edges from nsINodes by examining their children.

Comment 5

[Kris Wright :KrisWright]

Attachment: JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Created a runtime hook as well as a new CoarseType value to handle DOM nodes. Specialized ubi::Concrete for several nsINode types. Displayed outgoing nsISupports* edges on reflector JSObjects. Generated outgoing child edges from nsINodes by examining their children.

Comment 6

[Kris Wright :KrisWright]

Comment on attachment 8992740 [details] [diff] [review]
JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

To note: the changes to CoarseType and type names have been separated into their own bug (Bug 1476141) so for this patch the nsINode specializations have been sorted as CoarseType::Other.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8992740 [details] [diff] [review]
JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

>+++ b/dom/base/NodeUbiReporting.cpp
>+const char16_t JS::ubi::Concrete<nsINode>::concreteTypeName[] = u"nsINode";

Don't we need a concreteTypeName for every Concrete specialization?  My understanding of how the typename thing works is that the ubinode machinery basically depends on that in some way...  Please double-check this with Jim.

>+JS::ubi::Concrete<nsINode>::edges (JSContext* cx, bool wantNames) const

Please remove space before '('.

>+  char16_t* edgeName;

Please declare this closer to where it's used.  Really, just having two separate declarations for the two blocks that need an edge name would be best.

>+  if (!range || !ptr) {

As before, can ptr be null here?  See comment 2.  I know there were a lot of comments in there, but please double-check whether you addressed them all.  If there are some you are explicitly not addressing, please explain why.

As before, should we be paying attention to the wantNames bit somewhere here?  If we're ignoring it, we should have a comment explaining why, at least.

>+    range->addEdge(JS::ubi::Edge(edgeName, get().GetParent()));

So failure here means we press on....

>+    if(!range->addEdge(JS::ubi::Edge(edgeName, curr))) {

But failure here means we bail out?  Why the difference

If this line stays, please add a space after "if".

>+++ b/dom/base/NodeUbiReporting.h
>+* We want to measure parts of DOM structure from the JS memory tool.
>+* To do this we specialize DOM objects for JS::ubi::Base.

How about:

 * This file defines specializations of JS::ubi::Concrete for DOM nodes
 * so that the JS memory tools, which operate on the UbiNode graph, can
 * add nodes representing DOM nodes to that graph and surface ownership
 * relationships involving DOM nodes.

or something?  I'm not happy about the overloading of "node" in my phrasing....

>+  static void actuallyConstruct(void *storage, nsINode *ptr) { new (storage) Concrete(ptr); }

Again, I think we should have an Attr specialization instead.

>+++ b/js/public/UbiNode.h
>+struct ConstructUbiNodeForDOMObjectCallback {
>+  virtual void Construct (void* storage, JSObject* ptr) = 0;
>+};

Please see comment 2.

>+++ b/js/xpconnect/src/XPCJSRuntime.cpp

Watch 80 char line length.

>+    if(!supp) {

Please fix the whitespace (space after "if").

Comment 8

[Kris Wright :KrisWright]

Attachment: JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Created a runtime hook as well as to handle DOM nodes. Specialized ubi::Concrete for nsINode-inheriting objects. Displayed outgoing nsISupports* edges on reflector JSObjects. Generated outgoing child edges from nsINodes by examining their children. Updated the UbiNodeCensus to ignore zone checks if there is no zone to be found in a node.

Comment 9

[Kris Wright :KrisWright]

Attachment: JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Created a runtime hook as well as to handle DOM nodes. Specialized ubi::Concrete for nsINode-inheriting objects. Displayed outgoing nsISupports* edges on reflector JSObjects. Generated outgoing child edges from nsINodes by examining their children. Updated the UbiNodeCensus to ignore zone checks if there is no zone to be found in a node.

Comment 10

[Kris Wright :KrisWright]

Attachment: JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Created a runtime hook as well as to handle DOM nodes. Specialized ubi::Concrete for nsINode-inheriting objects. Displayed outgoing nsISupports* edges on reflector JSObjects. Generated outgoing child edges from nsINodes by examining their children. Updated the UbiNodeCensus to ignore zone checks if there is no zone to be found in a node.

Comment 11

[Kris Wright :KrisWright]

Attachment: JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Created a runtime hook as well as to handle DOM nodes. Specialized ubi::Concrete for nsINode-inheriting objects. Displayed outgoing nsISupports* edges on reflector JSObjects. Generated outgoing child edges from nsINodes by examining their children. Updated the UbiNodeCensus to ignore zone checks if there is no zone to be found in a node.

Comment 12

[Kris Wright :KrisWright]

Comment on attachment 8993188 [details] [diff] [review]
JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

In addressing the wantNames bit in Concrete<nsINode>::edges I moved the OOM checks from SimpleEdgeRange::addEdge to immediately after they happen in Concrete<nsINode>::edges (as it should have been all along). SimpleEdgeRange::addEdge is only used in this patch right now.

Comment 13

[Eric Rahm [:erahm]]

Attachment: Interdiff of the patch reviewed in comment 7 and the latest patch in comment 18

Boris, FYI this is the interdiff of the last version you reviewed (rebased on inbound) if that makes things easier.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8993188 [details] [diff] [review]
JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

>+++ b/dom/base/NodeUbiReporting.cpp
>+    if (wantNames){

Need space between ')' and '{'.  This applies to both places (the parent and child blocks).

>+        if (!edgeName) {

Can't happen. NS_strdup never returns null.  Yes, I know it has internal null-checks; they're bogus because moz_xmalloc never returns null.  Please remove  this block.  Again, applies to both places.

>+      }

This is misindented (it's the closing curly of the "if (wantNames)" block in the "parent" block).

>+++ b/dom/base/nsINode.h
>+  virtual void ConstructUbiNode(void* storage) = 0;

Could use a comment about how this is a hook for constructing one of the structs defined in NodeUbiReporting.h.  Sorry, should have caught this before...

>+++ b/js/xpconnect/src/XPCJSRuntime.cpp
>+ReflectorNode::edges (JSContext* cx, bool wantNames) const

Please remove the space after "edges".

>+        if (!edgeName) {

Can't happen; remove this block.

r=me with those issues fixed.

Comment 15

[Kris Wright :KrisWright]

Attachment: JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Created a runtime hook to handle DOM nodes. Specialized ubi::Concrete for nsINode-inheriting objects. Displayed outgoing nsISupports* edges on reflector JSObjects. Generated outgoing child edges from nsINodes by examining their children. Updated the UbiNodeCensus to ignore zone checks if there is no zone to be found in a node.

Comment 16

[Kris Wright :KrisWright]

Comment on attachment 8993522 [details] [diff] [review]
JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Carrying over Boris's r+.

Comment 17

[Jim Blandy :jimb]

Comment on attachment 8993522 [details] [diff] [review]
JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

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

Still reviewing, but here are my comments thus far:

::: js/public/UbiNode.h
@@ +1166,5 @@
>  };
> +
> +// When creating ubi::Nodes for reflector objects, we want to populate the
> +// runtime of the object with a ptr to its instructions to construct an instance
> +// of the reflector ubi::Node.

The comment for a function that is called for its side effects, like this, should start with an imperative sentence. If it's not too awkward, the sentence should try to use the argument names to spell out the effect:

   Set |cx|'s runtime hook for constructing ubi::Nodes for DOM classes to |cb|.

Since this header is the documentation for the ubi::Node API, we should spell out how to use this hook. If they've written ubi::Base subclasses before, we should be able to tell them everything they need to know to use the hook, without having to drill down into the code:

    The |cb| callback is much like the |Concrete<T>::construct| method: a call to
    |cb| should construct an instance of the most appropriate JS::ubi::Base subclass
    for |obj| in |storage|. The callback may assume that
    |obj->getClass()->isDOMClass()|, and that |storage| refers to the
    sizeof(JS::ubi::Base) bytes of space that all ubi::Base implementations should
    require.

Then we should actually name the arguments in the signature, as we refer to them in the comment:

    void SetConstructUbiNodeForDOMObjectCallback(JSContext* cx, void (*cb)(void* storage, JSObject* obj));

::: js/src/vm/Runtime.h
@@ +358,4 @@
>      /* Compartment memory reporting callback. */
>      js::MainThreadData<JSSizeOfIncludingThisCompartmentCallback> sizeOfIncludingThisCompartmentCallback;
>  
> +    /* Callback for creating ubi::Nodes representing DOM node objects */

This should probably refer people to the doc comment for SetConstructUbiNodeForDOMObjectCallback in UbiNode.h's for details.

::: js/src/vm/UbiNode.cpp
@@ +538,5 @@
>      return tracer.okay;
>  }
>  
> +void
> +Concrete<JSObject>::construct(void* storage, JSObject* ptr) {

Shouldn't this patch include a matching change to UbiNode.h that deletes the inline definition of this method from the Concrete<JSObject> specialization defined there? Otherwise this wouldn't compile...

@@ +545,5 @@
> +    } else {
> +        auto src = ptr->getClass();
> +        auto cb = ptr->compartment()->
> +                  runtimeFromMainThread()->constructUbiNodeForDOMObjectCallback;
> +        if (!src->isDOMClass() || !cb) {

SpiderMonkey typically names js::Class pointers 'clasp'; that's better than 'src'.
Also, since the code will be read more often than it is written, 'callback' is better than 'cb'.

I think this `if` nest would be neater if we let `new (storage) Concrete(ptr)` be the fallback at the bottom of the function, and then just had positive conditions for the more specialized construction:

if (ptr) {
    ... get clasp and callback ...
    if (clasp->isDOMClass() && callback) {
        ... call callback
        return;
    }
}

new (storage) Concrete(ptr);

Comment 18

[Jim Blandy :jimb]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #2)
> This is pretty unfortunate.  Technically, things allocated via NS_strdup
> should not be freed via js_free, which is what ubi::Edge will try to do with
> this pointer.  But the JS engine doesn't seem to expose a function that will
> do the right thing here (e.g. js::DuplicateString).  For now this is OK,
> because Gecko and Spidermonkey happen to share an allocator, but long-term
> it's a problem.  Please file a followup to get this fixed and make it block
> bug 1243367.

Yes, this is an ugly problem with the ubi::Node interface. Effectively, JS::ubi::Edge wants to apply a SpiderMonkey de-allocator to objects that the embedding is expected to construct. Maybe the right fix is to delete Edge::name as a public member, whose type determines the allocator, and instead make the Edge name accessed via a virtual accessor method. Then individual Edge subclasses could decide how to free the name. Some subclasses might not even need heap-allocated names at all.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> effectively, JS::ubi::Edge wants to apply a SpiderMonkey de-allocator to objects that the embedding is
> expected to construct.

That's not a problem per se, since JS_malloc does exist.  It's just pretty ugly to construct/duplicate a string via determining a length, then JS_malloc, then copying...

This could be solved by allowing deallocation by the embedding, or by exposing a string-copying thing that allocates via the SpiderMonkey allocator, either way.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> Some subclasses might not even need heap-allocated names at all.

Oh, and that is a _very_ good point.  All the edge names we have in the nsINode bits so far are static-lifetime, so wouldn't need allocation at all.

Comment 21

[Eric Rahm [:erahm]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #20)
> > Some subclasses might not even need heap-allocated names at all.
> 
> Oh, and that is a _very_ good point.  All the edge names we have in the
> nsINode bits so far are static-lifetime, so wouldn't need allocation at all.

Just to be clear, these changes can be follow ups, right?

Comment 22

[Boris Zbarsky [:bzbarsky]]

> Just to be clear, these changes can be follow ups, right?

Yes.

Comment 23

[Jim Blandy :jimb]

Comment on attachment 8993522 [details] [diff] [review]
JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

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

This looks great. I just have a few revisions I think need to happen before we land, so I'm marking it r-. If you address these comments and post a revised patch, I promise to re-review promptly.

::: dom/base/NodeUbiReporting.cpp
@@ +24,5 @@
> +  ptr->ConstructUbiNode(storage);
> +}
> +
> +js::UniquePtr<EdgeRange>
> +JS::ubi::Concrete<nsINode>::edges(JSContext* cx, bool wantNames) const

This is really nice. Just what's needed, no extra fuss, no surprises.

@@ +35,5 @@
> +    char16_t* edgeName = nullptr;
> +    if (wantNames) {
> +      edgeName = NS_strdup(u"Parent Node");
> +    }
> +    range->addEdge(JS::ubi::Edge(edgeName, get().GetParent()));

SimpleEdgeRange::addEdge is fallible; don't forget to check for OOM.

@@ +42,5 @@
> +    char16_t* edgeName = nullptr;
> +    if (wantNames) {
> +      edgeName = NS_strdup(u"Child Node");
> +    }
> +    range->addEdge(JS::ubi::Edge(edgeName, curr));

Same.

@@ +64,5 @@
> +  mozilla::SizeOfState sz(mallocSizeOf);
> +  nsWindowSizes wn(sz);
> +  getDoc().DocAddSizeOfIncludingThis(wn);
> +  size_t n = wn.getTotalSize();
> +  return n;

nit: I'd be just as happy with:

return wn.getTotalSize();

::: dom/base/NodeUbiReporting.h
@@ +12,5 @@
> +
> +/*
> + * This file defines specializations of JS::ubi::Concrete for DOM nodes
> + * so that the JS memory tools, which operate on the UbiNode graph, can
> + * add specializations to JS::ubi::Base that represent DOM nodes and

Instead of "add specializations to JS::ubi::Base", why not "define subclasses of JS::ubi::Base"? We shouldn't use the wrong terminology, especially when we're using 'specializations' correctly just above.

@@ +24,5 @@
> +template<>
> +class Concrete<nsINode> : public Base
> +{
> +protected:
> +  explicit Concrete(nsINode *ptr) : Base(ptr) { }

I could use a comment here reminding me that this is only used as a base class, and thus needs no concreteTypeName.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2817,5 @@
>  }
>  
> +// In the event that we want to report ubi::Nodes from DOM reflectors, we want
> +// to delegate their JSObject construction to this special reflector class.
> +// XPCJSRuntime knows enough about JS::ubi::Node /and/ nsINode to do this.

The comments on a definition of a class like ReflectorNode should ideally answer three questions:
- "What the hell is this?"
- "How does it get used?"
- "How does it work?"

This explanation has to serve three very different audiences:

- People trying to figure out if they can ignore it
- People who have found themselves stuck fixing it just because their real work happens to touch it tangentially
- People who care about it and are interested in understanding how it works

So you want to start out the comment for ReflectorNode with a noun clause that says what it is:

// Subclass of JS::ubi::Base for DOM reflector JS objects,

and then people are going to say "What in the world is JS::ubi::Concrete?", so you add

// for the JS::ubi::Node memory analysis framework; see js/public/UbiNode.h.

This should be enough to reassure people who don't care that they can ignore it.

For the others, now that we've pointed them at UbiNode.h, we can assume the reader understands the basic idea of ubi::Node. If the explanation there isn't clear, then we should fix that, but either way that's where background explanation belongs; there's no point in recapitulating it here. 

Now for "How does it get used?"

// In XPCJSRuntime::Initialize, we register the ConstructUbiNode function as a hook with the SpiderMonkey runtime for
// it to use use to construct ubi::Nodes of this class for JSObjects whose class has the JSCLASS_IS_DOMJSCLASS flag set.

Note that we've now tied this code to the specific feature that activates it, Firefox's use of JSCLASS_IS_DOMJSCLASS. That's something they can searchfox for.

Now for "How does it work?" This text should help a reader who is familiar with ubi::Node, and wondering why we need more code about it here:

// ReflectorNode specializes Concrete<JSObject> for DOM reflector nodes, reporting the edge from the JSObject to
// the nsINode it represents, in addition to the usual edges departing any normal JSObject.

@@ +2837,5 @@
> +js::UniquePtr<EdgeRange>
> +ReflectorNode::edges(JSContext* cx, bool wantNames) const
> +{
> +    js::UniquePtr<SimpleEdgeRange> range(
> +        static_cast<SimpleEdgeRange*>(Concrete<JSObject>::edges(cx, wantNames).release()));

Downcasts like this make me uncomfortable, but we can fix it later. Please file a bug blocking this one: "JS::ubi::ReflectorNode::edges should not assume a SimpleEdgeRange"

@@ +2841,5 @@
> +        static_cast<SimpleEdgeRange*>(Concrete<JSObject>::edges(cx, wantNames).release()));
> +    if (!range) {
> +        return nullptr;
> +    }
> +    // Add the edges that the JS engine doesn't know about

This should end with a period.

@@ +2854,5 @@
> +    char16_t* edgeName = nullptr;
> +    if (wantNames) {
> +        edgeName = NS_strdup(u"Reflected Node");
> +    }
> +    range->addEdge(Edge(edgeName, node.get()));

As before, I think this `if` nest would work better phrased as positive conditions for adding the additional edge, not early returns:

  nsISupports* supp = UnwrapDOM...;
  if (supp) {
       nsCOMPtr<NSINode> node = do_Query...;
       if (node) {
           ... allocate name if requested ...
           ... addEdge ...
           ... return nullptr on oom ...
       }
  }

  return range;

I think that makes it easier to see that we're always returning range (absent any failures); we're just adding an edge under certain conditions.

Comment 24

[Kris Wright :KrisWright]

Attachment: JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Created a runtime hook to handle DOM nodes. Specialized ubi::Concrete for nsINode-inheriting objects. Displayed outgoing nsISupports* edges on reflector JSObjects. Generated outgoing child edges from nsINodes by examining their children. Updated the UbiNodeCensus to ignore zone checks if there is no zone to be found in a node.

Comment 25

[Kris Wright :KrisWright]

(In reply to Jim Blandy :jimb from comment #23)
> Downcasts like this make me uncomfortable, but we can fix it later. Please
> file a bug blocking this one: "JS::ubi::ReflectorNode::edges should not
> assume a SimpleEdgeRange"

Shouldn't a new followup bug for this issue depend on this bug rather than block it? Maybe my understanding of the system is wrong but from what I understood, we need to resolve this bug first since the fix must first know about the new JS::ubi::ReflectorNode class.

Comment 26

[Jim Blandy :jimb]

(In reply to Eric Rahm [:erahm] from comment #21)
> (In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from
> comment #20)
> > > Some subclasses might not even need heap-allocated names at all.
> > 
> > Oh, and that is a _very_ good point.  All the edge names we have in the
> > nsINode bits so far are static-lifetime, so wouldn't need allocation at all.
> 
> Just to be clear, these changes can be follow ups, right?

Yes, definitely.

Comment 27

[Jim Blandy :jimb]

(In reply to Kristen Wright :KrisWright from comment #25)
> (In reply to Jim Blandy :jimb from comment #23)
> > Downcasts like this make me uncomfortable, but we can fix it later. Please
> > file a bug blocking this one: "JS::ubi::ReflectorNode::edges should not
> > assume a SimpleEdgeRange"
> 
> Shouldn't a new followup bug for this issue depend on this bug rather than
> block it? Maybe my understanding of the system is wrong but from what I
> understood, we need to resolve this bug first since the fix must first know
> about the new JS::ubi::ReflectorNode class.

You're saying: that bug's patch would be something that cannot land *until* this bug lands, so logically this bug would be a prerequisite for it, not the other way around? That makes sense.

I was thinking at a more project-oriented level: a follow-up bug represents a little piece of work that really belongs as part of this bug, but we're putting it off for later. So although we'll mark this bug RESOLVED FIXED, we'll still have open blockers for it, because there's work to be done yet.

I'm not actually positive that my approach is what everyone else does on Bugzilla. I think what's essential is that we be able to find the bug when we need it, and we can do that no matter which direction the "X blocks Y" arrow points. So I think things will be fine if you do whichever makes the most sense to you.

Comment 28

[Jim Blandy :jimb]

Comment on attachment 8994321 [details] [diff] [review]
JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

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

Looks great!

::: dom/base/NodeUbiReporting.cpp
@@ +37,5 @@
> +      edgeName = NS_strdup(u"Parent Node");
> +    }
> +    if (!range->addEdge(JS::ubi::Edge(edgeName, get().GetParent()))) {
> +      return nullptr;
> +    }

Great, thanks.

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +2846,5 @@
> +        static_cast<SimpleEdgeRange*>(Concrete<JSObject>::edges(cx, wantNames).release()));
> +    if (!range) {
> +        return nullptr;
> +    }
> +    // Add the edges that the JS engine doesn't know about.

Great comment.

Comment 29

[Raul Gurzau (:RaulG)]

Patches failed to apply:

applying interdiff.patch
patching file dom/base/nsINode.cpp
Hunk #1 FAILED at 98
Hunk #2 FAILED at 2884
2 out of 2 hunks FAILED -- saving rejects to file dom/base/nsINode.cpp.rej
patching file dom/base/nsINode.h
Hunk #1 FAILED at 448
1 out of 1 hunks FAILED -- saving rejects to file dom/base/nsINode.h.rej
patching file js/public/UbiNode.h
Hunk #1 FAILED at 1157
1 out of 1 hunks FAILED -- saving rejects to file js/public/UbiNode.h.rej
patching file js/src/vm/Runtime.h
Hunk #1 FAILED at 352
1 out of 1 hunks FAILED -- saving rejects to file js/src/vm/Runtime.h.rej
patching file js/src/vm/UbiNode.cpp
Hunk #1 FAILED at 545
1 out of 1 hunks FAILED -- saving rejects to file js/src/vm/UbiNode.cpp.rej
patching file js/xpconnect/src/XPCJSRuntime.cpp
Hunk #1 FAILED at 2821
Hunk #2 FAILED at 2919
2 out of 2 hunks FAILED -- saving rejects to file js/xpconnect/src/XPCJSRuntime.cpp.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working directory
errors during apply, please fix and qrefresh interdiff.patch

Comment 30

[Boris Zbarsky [:bzbarsky]]

Why are you applying the interdiff?

Comment 31

[Raul Gurzau (:RaulG)]

Now that it's obsolete it should't apply.

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec8c33de4e10
JSRuntime hook to construct ubi::Nodes specialized for nsINode instances r=jimb

Comment 33

[Natalia Csoregi [:nataliaCs]]

Backed out for build bustage.

backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/b3c8d1cdee5009b46bb769f5386a9b58993dd787

push: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ec8c33de4e10e6bc4311ecfcdca4828c36329df5

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=189833173&repo=mozilla-inbound&lineNumber=22992

[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -  In file included from /builds/worker/workspace/build/src/dom/base/ScreenLuminance.cpp:7:0,
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -                   from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp:2:
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -  /builds/worker/workspace/build/src/dom/base/ScreenLuminance.h:19:1: error: expected class-name before '{' token
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -   {
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -   ^
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -  /builds/worker/workspace/build/src/dom/base/ScreenLuminance.h:43:13: error: 'JSObject* mozilla::dom::ScreenLuminance::WrapObject(JSContext*, JS::Handle<JSObject*>)' marked 'override', but does not override
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -     JSObject* WrapObject(JSContext* aCx,
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -               ^~~~~~~~~~
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -  In file included from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/EventTarget.h:13:0,
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -                   from /builds/worker/workspace/build/src/dom/base/nsPIDOMWindow.h:15,
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -                   from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/DOMEventTargetHelper.h:13,
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -                   from /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/ScreenOrientation.h:10,
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -                   from /builds/worker/workspace/build/src/dom/base/nsScreen.h:12,
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -                   from /builds/worker/workspace/build/src/dom/base/ScreenLuminance.cpp:8,
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -                   from /builds/worker/workspace/build/src/obj-firefox/dom/base/Unified_cpp_dom_base4.cpp:2:
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -  /builds/worker/workspace/build/src/dom/base/ScreenLuminance.cpp: In member function 'virtual void mozilla::dom::ScreenLuminance::cycleCollection::Unlink(void*)':
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -  /builds/worker/workspace/build/src/dom/base/nsWrapperCache.h:437:8: error: 'class mozilla::dom::ScreenLuminance' has no member named 'ReleaseWrapper'; did you mean 'Release'?
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -     tmp->ReleaseWrapper(p);
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -          ^
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -  /builds/worker/workspace/build/src/dom/base/nsWrapperCache.h:457:5: note: in expansion of macro 'NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER'
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -       NS_IMPL_CYCLE_COLLECTION_UNLINK_PRESERVED_WRAPPER      \
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -  /builds/worker/workspace/build/src/dom/base/ScreenLuminance.cpp:15:1: note: in expansion of macro 'NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE'
[task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -   NS_IMPL_CYCLE_COLLECTION_WRAPPERCACHE(ScreenLuminance, mScreen)
[task 2018-07-24T16:01:40.853Z] 16:01:40     INFO -   ^
[task 2018-07-24T16:01:40.853Z] 16:01:40     INFO -  /builds/worker/workspace/build/src/dom/base/ScreenLuminance.cpp: In member function 'virtual void mozilla::dom::ScreenLuminance::cycleCollection::Trace(void*, const TraceCallbacks&, void*)':
[task 2018-07-24T16:01:40.853Z] 16:01:40     INFO -  /builds/worker/workspace/build/src/dom/base/nsWrapperCache.h:434:8: error: 'class mozilla::dom::ScreenLuminance' has no member named 'TraceWrapper'

Comment 34

[Boris Zbarsky [:bzbarsky]]

> [task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -  /builds/worker/workspace/build/src/dom/base/ScreenLuminance.h:19:1: error: expected class-name before '{' token

This is the key part, I expect.  ScreenLuminance.h has:

  class ScreenLuminance final : public nsWrapperCache

but does not include nsWrapperCache.h.  Looks like the new .cpp file added in this patch changed unification boundaries and this started failing, like it should have all along.  I thought we had non-unified build jobs that were supposed to catch that...

Kristen, you should probably do a try run before requesting checkin on the fixed version, in case there are more landmines here. :(

Comment 35

[Kris Wright :KrisWright]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #34)
> This is the key part, I expect.  ScreenLuminance.h has:
> 
>   class ScreenLuminance final : public nsWrapperCache
> 
> but does not include nsWrapperCache.h.  Looks like the new .cpp file added
> in this patch changed unification boundaries and this started failing, like
> it should have all along.  I thought we had non-unified build jobs that were
> supposed to catch that...
> 
> Kristen, you should probably do a try run before requesting checkin on the
> fixed version, in case there are more landmines here. :(

That's really interesting. I've never had problems with files I haven't touched before, but then again I've never added an entirely new .cpp to DOM either. Should I be including nsWrapperCache.h in ScreenLuminance.h or is this a problem with something else entirely?

Comment 36

[Boris Zbarsky [:bzbarsky]]

> I've never had problems with files I haven't touched before

Happy to explain to you on IRC why this was a problem, if you want.  Let me know.

> Should I be including nsWrapperCache.h in ScreenLuminance.h

My guess is that this is the issue and that adding that will fix the problem.  But you definitely want to do that try run to verify.

Comment 37

[Eric Rahm [:erahm]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #34)
> > [task 2018-07-24T16:01:40.852Z] 16:01:40     INFO -  /builds/worker/workspace/build/src/dom/base/ScreenLuminance.h:19:1: error: expected class-name before '{' token
> 
> This is the key part, I expect.  ScreenLuminance.h has:
> 
>   class ScreenLuminance final : public nsWrapperCache
> 
> but does not include nsWrapperCache.h.  Looks like the new .cpp file added
> in this patch changed unification boundaries and this started failing, like
> it should have all along.  I thought we had non-unified build jobs that were
> supposed to catch that...

We only support unified builds these days.

> Kristen, you should probably do a try run before requesting checkin on the
> fixed version, in case there are more landmines here. :(

Those are only fuzzing build failures -- it's unclear whether we can even build them on try. I'm surprised they're tier 1. There was a red hazard build [1] that we should probably check out. I pinged sfink about it.

[1] https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=ec8c33de4e10e6bc4311ecfcdca4828c36329df5&selectedJob=189833186

Comment 38

[Boris Zbarsky [:bzbarsky]]

> We only support unified builds these days.

See bug 1447458 tracking fixing that.

> Those are only fuzzing build failures 

Funtimes.

> There was a red hazard build [1] that we should probably check out

I am _guessing_ that now we think that ubi::Node::construct can GC because we have a virtual call in the nsINode specialization and that we need to edit js/src/devtools/rootAnalysis/annotations.js to suppress that false positive.

Comment 39

[Jim Blandy :jimb]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #38)
> I am _guessing_ that now we think that ubi::Node::construct can GC because
> we have a virtual call in the nsINode specialization and that we need to
> edit js/src/devtools/rootAnalysis/annotations.js to suppress that false
> positive.

Yes, obviously ubi::Node construction must never, ever GC. If it's concerned about the new virtual method added in the patch, then the hazard build is indeed reporting a false positive; that doesn't GC. (If it does, that's a bug.)

Comment 40

[Kris Wright :KrisWright]

(In reply to Jim Blandy :jimb from comment #39)
> Yes, obviously ubi::Node construction must never, ever GC. If it's concerned
> about the new virtual method added in the patch, then the hazard build is
> indeed reporting a false positive; that doesn't GC. (If it does, that's a
> bug.)

I've had a chance to play with it and read through the hazards and yes, it's concerned that pretty much all of the above may GC. sfink suggested putting an AutoSuppressGCAnalysis into the Concrete<JSObject>::construct, but it generates 19 other errors that encompass nearly all of the new Concrete specializations' methods and the ReflectorNode::edges method. Again, fixable by adding AutoSuppressGCAnalysis but it seems redundant to go in and tell nearly every method that it does not GC.

Maybe the best solution would be to do as Boris said and edit js/src/devtools/rootAnalysis/annotations.js.

Comment 41

[Kris Wright :KrisWright]

Attachment: JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Created a runtime hook to handle DOM nodes. Specialized ubi::Concrete for nsINode-inheriting objects. Displayed outgoing nsISupports* edges on reflector JSObjects. Generated outgoing child edges from nsINodes by examining their children. Updated the UbiNodeCensus to ignore zone checks if there is no zone to be found in a node.

Comment 42

[Eric Rahm [:erahm]]

Comment on attachment 8994984 [details] [diff] [review]
JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Steve, can you give the |AutoSuppressGCAnalysis| changes a once over here?

Comment 43

[Jim Blandy :jimb]

FWIW, the interdiff looks okay to me. But yes, I'd like sfink's opinion too

Comment 44

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8994984 [details] [diff] [review]
JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

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

I'm fine with this landing as-is (well, with one minor change; see below), but filing a followup bug to fix the hazards in a slightly cleaner way: (1) in isOverridableField in annotations.js, add in an exception for DocAddSizeOfIncludingThis; and (2) do *something* better for the QueryInterface hazard. I'm actually unsure how safe that one is, I seem to recall that some QI calls *can* GC. I wish I kept better notes.

(1) https://searchfox.org/mozilla-central/source/js/src/devtools/rootAnalysis/annotations.js#385

(2) Example hazard:

Function '_ZN29cls_test_JS_ubi_DominatorTree3runEN2JS6HandleIP8JSObjectEE$uint8 cls_test_JS_ubi_DominatorTree::run(class JS::Handle<JSObject*>)' has unrooted 'noGC' of type 'JS::AutoCheckCannotGC' live across GC call '_ZN2JS3ubi13DominatorTree6CreateEP9JSContextRNS_17AutoCheckCannotGCERKNS0_4NodeE$static mozilla::Maybe<JS::ubi::DominatorTree> JS::ubi::DominatorTree::Create(JSContext*, JS::AutoCheckCannotGC&, const JS::ubi::Node&)' at js/src/jsapi-tests/testUbiNode.cpp:545
GC Function: _ZN2JS3ubi13DominatorTree6CreateEP9JSContextRNS_17AutoCheckCannotGCERKNS0_4NodeE$static mozilla::Maybe<JS::ubi::DominatorTree> JS::ubi::DominatorTree::Create(JSContext*, JS::AutoCheckCannotGC&, const JS::ubi::Node&)
    uint8 JS::ubi::DominatorTree::doTraversal(struct JSContext*, JS::AutoCheckCannotGC*, JS::ubi::Node*, class mozilla::Vector<JS::ubi::Node, 0ul, js::SystemAllocPolicy>*, class js::HashMap<JS::ubi::Node, mozilla::UniquePtr<js::HashSet<JS::ubi::Node, js::DefaultHasher<JS::ubi::Node>, js::SystemAllocPolicy>, JS::DeletePolicy<js::HashSet<JS::ubi::Node, js::DefaultHasher<JS::ubi::Node>, js::SystemAllocPolicy> > >, js::DefaultHasher<JS::ubi::Node>, js::SystemAllocPolicy>*)
    uint8 JS::ubi::PostOrder::addStart(JS::ubi::Node*)
    uint8 JS::ubi::PostOrder::pushForTraversing(JS::ubi::Node*)
    js::UniquePtr<JS::ubi::EdgeRange> JS::ubi::Node::edges(JSContext*, bool) const
    virtual js::UniquePtr<JS::ubi::EdgeRange> JS::ubi::ReflectorNode::edges(JSContext*, bool) const
    nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = nsINode]
    nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = nsINode]
    void nsCOMPtr<T>::assign_from_qi(nsQueryInterface, const nsIID&) [with T = nsINode; nsIID = nsID]
    uint32 nsQueryInterface::operator(nsID*, void**)(const nsIID&, void**) const
    FieldCall: nsISupports.QueryInterface

::: js/src/vm/UbiNode.cpp
@@ +540,5 @@
>  
> +void
> +Concrete<JSObject>::construct(void* storage, JSObject* ptr) {
> +    if (ptr) {
> +        AutoSuppressGCAnalysis suppress;

I would prefer if this were as narrowly scoped at possible: within the if (clas->isDOMClass()...) consequent, just before invoking callback().

Comment 45

[Kris Wright :KrisWright]

Attachment: JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Created a runtime hook to handle DOM nodes. Specialized ubi::Concrete for nsINode-inheriting objects. Displayed outgoing nsISupports* edges on reflector JSObjects. Generated outgoing child edges from nsINodes by examining their children. Updated the UbiNodeCensus to ignore zone checks if there is no zone to be found in a node.

Comment 46

[Boris Zbarsky [:bzbarsky]]

> I'm actually unsure how safe that one is, I seem to recall that some QI calls *can* GC

QI in general can run script, so could GC.  QI for things that are DOM objects...  I think the main issue is nsBindingManager::GetBindingImplementation which I am not at all sanguine about not gcing once it starts going through the XPCWrappedJS machinery.

It might be better to do something like this:

  // UNWRAP_OBJECT assumes the object is completely initialized, but ours
  // may not be.  Luckily, UnwrapDOMObjectToISupports checks for the 
  // uninitialized case (and returns null if uninitialized), so we can use
  // that to guard against uninitialized objects.
  nsISupports* supp = UnwrapDOMObjectToISupports(&get());
  if (supp) {
     // OK, we have a DOM object; check whether it's a Node.
     nsCOMPtr<nsINode> node;
     UNWRAP_OBJECT(Node, &get(), node);
     if (node) {

Looking at the patch, why do we need to suppress analysis in JS::ubi::Concrete<nsINode>::edges?

Comment 47

[Kris Wright :KrisWright]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #46)
> QI in general can run script, so could GC.  QI for things that are DOM
> objects...  I think the main issue is
> nsBindingManager::GetBindingImplementation which I am not at all sanguine
> about not gcing once it starts going through the XPCWrappedJS machinery.
> 
> It might be better to do something like this:
> 
>   // UNWRAP_OBJECT assumes the object is completely initialized, but ours
>   // may not be.  Luckily, UnwrapDOMObjectToISupports checks for the 
>   // uninitialized case (and returns null if uninitialized), so we can use
>   // that to guard against uninitialized objects.
>   nsISupports* supp = UnwrapDOMObjectToISupports(&get());
>   if (supp) {
>      // OK, we have a DOM object; check whether it's a Node.
>      nsCOMPtr<nsINode> node;
>      UNWRAP_OBJECT(Node, &get(), node);
>      if (node) {
> 
> Looking at the patch, why do we need to suppress analysis in
> JS::ubi::Concrete<nsINode>::edges?

The reason why JS::ubi::Concrete<nsINode>::edges has to suppress analysis isn't particularly clear to me, but it reported a hazard along with the Concrete<nsINode>::size method. I guess we'll want to address it in the cleanup.

In changing the QI, should I continue to suppress GC analysis? Or does UNWRAP_OBJECT solve that?

Comment 48

[Boris Zbarsky [:bzbarsky]]

> I guess we'll want to address it in the cleanup.

Ok.  Eyeballing it doesn't show obvious reasons for it to be a hazard; GetParent, GetFirstChild, GetNextSibling are all non-virtual and can't gc...  Would be interesting to see what the actual hazard looked like.

> In changing the QI, should I continue to suppress GC analysis? Or does UNWRAP_OBJECT solve that?

UNWRAP_OBJECT should solve that if I understand this stuff correctly.  But I clearly don't understand it as well as I thought I did, if edges() triggers the analysis.  :(

Comment 49

[Kris Wright :KrisWright]

Attachment: JSRuntime hook to construct ubi::Nodes specialized for nsINode instances

Created a runtime hook to handle DOM nodes. Specialized ubi::Concrete for nsINode-inheriting objects. Displayed outgoing nsISupports* edges on reflector JSObjects. Generated outgoing child edges from nsINodes by examining their children. Updated the UbiNodeCensus to ignore zone checks if there is no zone to be found in a node.

Comment 50

[Kris Wright :KrisWright]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #48)
> UNWRAP_OBJECT should solve that if I understand this stuff correctly.

Looks like you're right! Hazard build isn't throwing any failures on try.

Comment 51

[Steve Fink [:sfink] [:s:]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #48)
> > I guess we'll want to address it in the cleanup.
> 
> Ok.  Eyeballing it doesn't show obvious reasons for it to be a hazard;
> GetParent, GetFirstChild, GetNextSibling are all non-virtual and can't gc...
> Would be interesting to see what the actual hazard looked like.

Lots of them. It's the QI. I thought that was what you were pointing out in comment 46?

Example:

Function '_ZN7mozilla8devtools12HeapSnapshot20ComputeDominatorTreeERNS_11ErrorResultE$already_AddRefed<mozilla::devtools::DominatorTree> mozilla::devtools::HeapSnapshot::ComputeDominatorTree(mozilla::ErrorResult&)' has unrooted 'nogc' of type 'JS::AutoCheckCannotGC' live across GC call '_ZN2JS3ubi13DominatorTree6CreateEP9JSContextRNS_17AutoCheckCannotGCERKNS0_4NodeE$static mozilla::Maybe<JS::ubi::DominatorTree> JS::ubi::DominatorTree::Create(JSContext*, JS::AutoCheckCannotGC&, const JS::ubi::Node&)' at devtools/shared/heapsnapshot/HeapSnapshot.cpp:573

GC Function: _ZN2JS3ubi13DominatorTree6CreateEP9JSContextRNS_17AutoCheckCannotGCERKNS0_4NodeE$static mozilla::Maybe<JS::ubi::DominatorTree> JS::ubi::DominatorTree::Create(JSContext*, JS::AutoCheckCannotGC&, const JS::ubi::Node&)
    uint8 JS::ubi::DominatorTree::doTraversal(struct JSContext*, JS::AutoCheckCannotGC*, JS::ubi::Node*, class mozilla::Vector<JS::ubi::Node, 0ul, js::SystemAllocPolicy>*, class js::HashMap<JS::ubi::Node, mozilla::UniquePtr<js::HashSet<JS::ubi::Node, js::DefaultHasher<JS::ubi::Node>, js::SystemAllocPolicy>, JS::DeletePolicy<js::HashSet<JS::ubi::Node, js::DefaultHasher<JS::ubi::Node>, js::SystemAllocPolicy> > >, js::DefaultHasher<JS::ubi::Node>, js::SystemAllocPolicy>*)
    uint8 JS::ubi::PostOrder::addStart(JS::ubi::Node*)
    uint8 JS::ubi::PostOrder::pushForTraversing(JS::ubi::Node*)
    js::UniquePtr<JS::ubi::EdgeRange> JS::ubi::Node::edges(JSContext*, bool) const
    virtual js::UniquePtr<JS::ubi::EdgeRange> JS::ubi::ReflectorNode::edges(JSContext*, bool) const
    nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = nsINode]
    nsCOMPtr<T>::nsCOMPtr(nsQueryInterface) [with T = nsINode]
    void nsCOMPtr<T>::assign_from_qi(nsQueryInterface, const nsIID&) [with T = nsINode; nsIID = nsID]
    uint32 nsQueryInterface::operator(nsID*, void**)(const nsIID&, void**) const
    FieldCall: nsISupports.QueryInterface

Comment 52

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/176da751b5fa
JSRuntime hook to construct ubi::Nodes specialized for nsINode instances. r=sfink,jimb,bz

Comment 53

[Boris Zbarsky [:bzbarsky]]

> Example:

That doesn't seem to involve JS::ubi::Concrete<nsINode>::edges, which is the function where I'm not clear why we need an AutoSuppressGCAnalysis.

Comment 54

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/176da751b5fa

Comment 55

[Olli Pettay [:smaug][[e-mail]]]

Curious, are the plans to support all the outgoing edges, for example event listeners?
(basically need to look for all the stuff nodes' Traverse methods have)