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