Bug 1582326 Comment 1 Edit History

Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.

So in order:

1) I think the right fix for ReflectorNode::edges is to change `node` to `nsINode*` and use `UNWRAP_NON_WRAPPER_OBJECT` instead of `UNWRAP_OBJECT`.  That will avoid the call to `AddRef` and the complications.  A comment about how `&get()` is not going to be a CCW, about how `UnwrapDOMObjectToISupports` can only return non-null if its argument is an actual DOM object, not a CCW, might be worthwhile to explain why `UNWRAP_NON_WRAPPER_OBJECT`  is OK here.

2) The code as it exists right now most definitely calls `AddRef` on the node.

3) We could probably use `RefPtr<nsINode>` instead of `nsCOMPtr<nsINode>` (which works with `nsISupports` under the hood) to make it clear to the compiler that the `AddRef` is happening on an `nsINode`, not a random `nsISupports*` and that therefore things like `xpcAccessibilityService::AddRef` are not relevant.  That said, the fact that `xpcAccessibilityService::AddRef` can in fact GC (and can in fact run arbitrary code or something!) is rather disturbing.

4) The path from `nsINode::AddRef` (which would still be what we're doing with a `RefPtr<nsINode>`) to `nsCycleCollector::Suspect` is in fact correct.  The `CanonicalizeXPCOMParticipant` call there is debug-only (inside an assertion) but does in fact happen in a debug build, because the `AddRef` call passes a null `nsCycleCollectionParticipant`.  However the QI to `nsCycleCollectionISupports` in that code cannot land in `WrappedJSHolder::QueryInterface`, because the thing we're calling QI on is in fact an `nsINode` if the caller did not screw up.  So the rest of that stack can't happen in this case, and `nsINode::QueryInterface` overrides will check for `nsCycleCollectionISupports` up front and return without reaching the parts of `Element::QueryInterface` that jump out into XBL and can in fact GC or run script or whatnot.

So the current code is, I believe, safe, but see item 1 for probably the best way to make this clear to the analysis.
So in order:

1) I think the right fix for `ReflectorNode::edges` is to change `node` to `nsINode*` and use `UNWRAP_NON_WRAPPER_OBJECT` instead of `UNWRAP_OBJECT`.  That will avoid the call to `AddRef` and the complications.  A comment about how `&get()` is not going to be a CCW, or about how `UnwrapDOMObjectToISupports` can only return non-null if its argument is an actual DOM object, not a CCW, might be worthwhile to explain why `UNWRAP_NON_WRAPPER_OBJECT`  is OK here.  So something like this:
```
  nsINode* node;
  if (NS_SUCCEEDED(UNWRAP_NON_WRAPPER_OBJECT(Node, &get(), node)) {
    // Do things with `node` here.
  }
```

2) The code as it exists right now most definitely calls `AddRef` on the node.

3) We could probably use `RefPtr<nsINode>` instead of `nsCOMPtr<nsINode>` (which works with `nsISupports` under the hood) to make it clear to the compiler that the `AddRef` is happening on an `nsINode`, not a random `nsISupports*` and that therefore things like `xpcAccessibilityService::AddRef` are not relevant.  That said, the fact that `xpcAccessibilityService::AddRef` can in fact GC (and can in fact run arbitrary code or something!) is rather disturbing.

4) The path from `nsINode::AddRef` (which would still be what we're doing with a `RefPtr<nsINode>`) to `nsCycleCollector::Suspect` is in fact correct.  The `CanonicalizeXPCOMParticipant` call there is debug-only (inside an assertion) but does in fact happen in a debug build, because the `AddRef` call passes a null `nsCycleCollectionParticipant`.  However the QI to `nsCycleCollectionISupports` in that code cannot land in `WrappedJSHolder::QueryInterface`, because the thing we're calling QI on is in fact an `nsINode` if the caller did not screw up.  So the rest of that stack can't happen in this case, and `nsINode::QueryInterface` overrides will check for `nsCycleCollectionISupports` up front and return without reaching the parts of `Element::QueryInterface` that jump out into XBL and can in fact GC or run script or whatnot.

So the current code is, I believe, safe, but see item 1 for probably the best way to make this clear to the analysis.

Back to Bug 1582326 Comment 1