scope: disallow copying scopes to prevent double close of scopes

[legendecas]

Early detect and prevent unexpected crashing on closing an already closed handle scope.

• npm test passed

[NickNaso]

I think that this should prevent runtime error in case of double close. @legendecas Am I right?

[legendecas]

I think that this should prevent runtime error in case of double close. @legendecas Am I right?

Yes, napi_handle_scope_mismatch would be returned if double closing a handle scope.

Though I've found that return statuses of napi_close_*_scope have been ignored 🤔 https://github.com/nodejs/node-addon-api/blob/master/napi-inl.h#L3475. Are these behavior expected?

[gabrielschulhof]

@legendecas I believe the reason for that is that, if C++ exceptions are turned on, one would be throwing a C++ exception from a destructor which by my understanding is a big no-no.

[legendecas]

Since no exceptions were expected to be thrown in the destructor, I'd think to disallow copying of scopes should be an intuitive approach. What do you think? @gabrielschulhof

[mhdawson]

This would be a breaking change. While we've not said that will never happen for node-addon-api, we have chosen to avoid those in all cases before.

I think we'd want to have a really good reason to introduce a breaking change and I'm not sure it's going to be worth it in this case.

[mhdawson]

Marking as request changes until there is a discussion on whether making a breaking change is what we want to do.

[legendecas]

@mhdawson: This would be a breaking change.

Technically it would depend on platforms and possibly crash on double closing a handle scope -- since in node core napi_handle_scope is just a pointer to be deleted on napi_close_handle_scope.

So I'd wonder there would hardly be possible any use case in the wild. But yes, it is a breaking change.

[legendecas]

Append: node core has a pre-condition of deleting the HandleScopeWrapper*, so a napi_handle_scope_mismatch would be returned but not crashing immediately, which is what #566 was expected to address.

[legendecas]

PPS: still, I could produce the crash with the following snippet:

Value doubleCloseScopeNapi(const CallbackInfo& info) {
  Env env = info.Env();
  HandleScope scope(env);
  HandleScope scope2(env);
  HandleScope scope3 = scope2;
  return env.Undefined();
}

Output:

node(30396,0x10cd83d40) malloc: *** error for object 0x104817990: pointer being freed was not allocated
node(30396,0x10cd83d40) malloc: *** set a breakpoint in malloc_error_break to debug

[mhdawson]

Discussed in meeting today:

1. team is comfortable that it is extremely unlikely this will break any existing code, as the code would already be crashing.
2. V8 and NaN don't have the copy constructors as they don't make sense.

Consensus is we should we remove them.

[mhdawson]

LGTM

[NickNaso]

LGTM

[mhdawson]

We did discuss and this should land as a SemVer major even though it should not affect any existing code.

[mhdawson]

Landed as 34c11cf