src: remove redundant calls to HandleScope

[ryzokuken]

Refs: #19972 (comment)
Refs: #20045

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• tests and/or benchmarks are included
• documentation is changed or added
• commit message follows commit guidelines

cc @addaleax @hashseed @nodejs/v8

[ryzokuken]

CI: https://ci.nodejs.org/job/node-test-pull-request/14408/

[addaleax]

I don’t think we can be sure that these calls are unnecessary. CallbackScope is part of the public API, and without these scopes, addons could theoretically run into problems with patterns like this:

HandleScope handle_scope(…);
for (int i = 0; i < n; i++) {
  CallbackScope callback_scope(…);
  HandleScope handle_scope(…);
  /* do something with Node.js objects here */
}

Omitting the callback scopes here means that all of the handles allocated by the constructor/destructor (I only see 2 that I’m sure of, but it’s hard to tell just by looking at the code) would fall into the outer scope now, and would accumulate and be kept alive for as long as that loop runs.

I don’t think the impact would be huge, but I would prefer to err on the side of caution and not remove these?

That being said: I am not sure whether there are handle allocations inside the InternalCallbackScope() constructor, and whether there are any before the env_->process_object() call in the close function. If there are none, then yes, we can remove these scopes (or, in the Close() case, move the scope to the part that deals with process.nextTick). A reliable way to check whether there are handle allocations or not would be to use a v8::SealHandleScope and see whether there’s a crash or not.

[ryzokuken]

@addaleax I had been removing any calls to HandleScope preceding a call to Emit (and by extension, EmitBefore and EmitAfter) because we just started calling HandleScope within Emit in #20045.

That said, I'm really new to all this, and I might be very wrong. Let me know if I am, I would be more than happy to take my changes back.

[addaleax]

@ryzokuken Yeah, and that totally makes sense for cases like #20045 where it’s obvious that the HandleScope we’d be removing does not capture any other handle allocations as well. But that’s not the case here, so things get more complicated…

[ryzokuken]

@addaleax you're right. That said, if all the tests pass (benchmarks included), then perhaps it wouldn't be that bad of an idea anyway, right?

[addaleax]

@ryzokuken Like I said, this may affect memory usage in some odd cases – The handle scopes aren’t quite as redundant as it was the case in #20045.

[ryzokuken]

Hmm. Is there a way to find out? If they aren't redundant, we should just close this.

[addaleax]

@ryzokuken Basically what I said above – replace them with SealHandleScopes and see what happens. In the Close() case there are definitely other handles being created, so you would have to add another handle scope later in the function to catch that.

[BridgeAR]

Ping @ryzokuken

[ryzokuken]

@BridgeAR @addaleax sorry I completely forgot about this. Please give me a few more days to take another stab at this.

[addaleax]

@ryzokuken I think you can assume that this is not urgent in any way :)

[ryzokuken]

@addaleax It definitely isn't. I'm using PRs like these to widen my understanding of the C++ part of the codebase, tbh. Need to know internals better.

[apapirovski]

I'm going to close this out. Adding SealHandleScope results in crashes so clearly this isn't safe to do.