new internalNextTick cannot be wrapped by electron

[xaviergonz]

Due to the change from using process.nextTick to const { nextTick } = require('internal/process/next_tick'); electron cannot wrap that call anymore in order to use its internal process.activateUvLoop(), which is used on renderer processes to process the event queue (as seen in https://github.com/electron/electron/blob/7ef69a5af595c9c0eef27bdf9446103776917aec/lib/common/init.js)

This is related to issue electron/electron#12108

If this internal "nextTick" were to be exported in process as process._internalNextTick and any calls that used this nextTick were changed to use process._internalNextTick instead (there are only like 4 or 5 and only used for network/http stuff so far) then electron would be able once more to wrap these calls and get those working again.

I'm open to create a pull request, but only if it is likely to get accepted.

[apapirovski]

There should almost certainly be a better way of doing this than exposing it on process — although I don't have anything to suggest off the top of my head. There's also the new setUnrefTimeout which seems affected by this.

[apapirovski]

I don't know much about electron internals but would it be possible to just use init async hooks instead of the unreliable monkey-patching? You would be looking for type Immediate, Timeout or TickObject.

[xaviergonz]

I'm not familiar with setUnrefTimeout, what is it for?
good point about the async hooks :D didn't think of it. I tried it and it seems to work. Would the async hooks fix setUnrefTimeout as well?

[apapirovski]

Yep, it would. It's going to be used in the 10.x release for http timeouts and similar, but it uses the same Timeout object under the hood so it triggers the same async hook.

[xaviergonz]

@apapirovski The solution looked really promising, but sadly there's some weird interaction between installing an async_hook (even one with a single empty handler) crashing the rendering of pdf inside an iframe (no idea how it is related at all... might be because of the promise hooking?).

Is it still a feasible option to expose the internal next tick in process?

[xaviergonz]

Actually yeah, it seems to be related to the promise hooking, as soon as I commented out enablePromiseHook() it doesn't break the pdf viewer anymore O_o

[apapirovski]

There's no chance we'll expose internalNextTick on the process object.

[apapirovski]

Also, if async hooks are crashing something within electron, it seems like there are deeper issues that need to be resolved. Seems pretty serious if that's a whole feature that can't be used. But I have literally no electron knowledge so unfortunately I'm useless there...

[xaviergonz]

I agree, I'm just trying to fix a bug I found on electron.

However (just in case) I think I found some possible cause worth looking into, in env.cc:

void Environment::EnvPromiseHook(v8::PromiseHookType type,
                                 v8::Local<v8::Promise> promise,
                                 v8::Local<v8::Value> parent) {
  Environment* env = Environment::GetCurrent(promise->CreationContext());
  for (const PromiseHookCallback& hook : env->promise_hooks_) {
    hook.cb_(type, promise, parent, hook.arg_);
  }
}

the line
Environment* env = Environment::GetCurrent(promise->CreationContext());
sometimes returns a broken (non null) pointer, which makes the process crash

however my knowledge of that part of the node code is kind of limited. Is there any reason you may know why this might happen?

[apapirovski]

Not sure. Totally possible there's a bug on our end or V8s. I'll ping some folks...

/cc @AndreasMadsen @addaleax @ofrobots

[addaleax]

Is there any reason you may know why this might happen?

@xaviergonz Yes. I actually brought this up with @gsathya – the V8 PromiseHook API does not expose an opaque pointer argument that could be used to track context for the hook function, like most C++ APIs do, so we have to figure out the pointer to Node’s internal Environment based on the V8 Context.

For that to work, Node first needs to set an embedder field of that v8::Context explicitly; the method that does this is Environment::AssignToContext(), which is not really public API at this point. It’s something that we should probably expose in some way for embedders, though.

I assume that in Electron, the Chrome parts creates a lot of contexts on its own (e.g. for iframes, and probably the PDF viewer too), where no Node Environment is registered with those contexts.

The most obvious two ways to tackle this would be either letting electron call AssignToContext() for those cases, if there is a sensible way to assign a Environment to the context, or otherwise ignore Promises from those contexts in some way.

sometimes returns a broken (non null) pointer, which makes the process crash

Just curious, does that happen to be a tagged pointer value for V8 that translates to the internal undefined value?

[xaviergonz]

@addaleax

I assume that in Electron, the Chrome parts creates a lot of contexts on its own (e.g. for iframes, and probably the PDF viewer too), where no Node Environment is registered with those contexts.

AFAIK yes, it seems to fail inside iframes, which are actually devoid of node integration features (if that's what you mean)

Just curious, does that happen to be a tagged pointer value for V8 that translates to the internal undefined value?

I'm sorry, you really lost me there. What code would I need to add to check for that in this particular case? (I just got into all this node + electron + chromium internal mess like 24h ago while trying to fix a silly bug :D )

[xaviergonz]

If what you mean is what's the value of promise->IsNullOrUndefined(), then it is false at crash time.

[addaleax]

@xaviergonz Sorry, I kinda got it in my head that you were openening this issue because you were part of the electron team? 😄 Anyway, I’m sorry, my reply was kind of written with a different reader background in mind. (I can go into more detail on everything if you’re curious, though.)

AFAIK yes, it seems to fail inside iframes, which are actually devoid of node integration features (if that's what you mean)

Yes, that’s exactly what I mean. It’s good to know that they are devoid of Node features, I assume that that’s intentional?

What code would I need to add to check for that in this particular case?

I think you could run context->GetEmbedderData(32)->IsUndefined() in a debugger like gdb at the crashing point (32 is the default value for node::Environment::kContextEmbedderDataIndex).

If that works, I think an okay-ish fix for this bug would be performing basically that check programatically in Environment::EnvPromiseHook, and just returning if the value in our embedder field actually is undefined.

[xaviergonz]

No worries, I don't get paid for any of this :D

Yes, that’s exactly what I mean. It’s good to know that they are devoid of Node features, I assume that that’s intentional?

Yes, for security reasons, there are ways in electron to disable this isolation, but usually you don't want an iframe loading some random page and having access to the node context.

With your tip I just changed the code to

void Environment::EnvPromiseHook(v8::PromiseHookType type,
                                 v8::Local<v8::Promise> promise,
                                 v8::Local<v8::Value> parent) {
  auto context = promise->CreationContext();
  if (context->GetEmbedderData(32)->IsNullOrUndefined()) {
    return;
  }
  Environment* env = Environment::GetCurrent(context);
  for (const PromiseHookCallback& hook : env->promise_hooks_) {
    hook.cb_(type, promise, parent, hook.arg_);
  }
}

and there's no more process crashing anymore, hurray!

[xaviergonz]

Makes me wonder though, is this fix going to be applied? If so, under node v8, v9 or both?

[addaleax]

Yes, for security reasons, there are ways in electron to disable this isolation, but usually you don't want an iframe loading some random page and having access to the node context.

It would be good to know what Electron does in these cases (i.e. whether a Node context is being assigned or not).

With your tip I just changed the code to

You’d probably want to use the constant name in place of 32, and ->IsUndefined() if that also fixes the issue (I assume it does?). But otherwise this should be doable as a real PR.

Makes me wonder though, is this fix going to be applied?

I would lean towards “yes”. The downside here is that async state information would get lost across Contexts, but that’s in no way worse than a hard crash. Feel free to open a PR. :)

If so, under node v8, v9 or both?

All release lines that have these hooks could receive this patch, in general. It might be worth trying to check if there’s a performance impact, that’s the only thing that might influence the decision to land this on Node 8.

[xaviergonz]

Cool, I changed the number to the constant name and created a pull request. However I didn't run any performance tests, is that done by CI?

[addaleax]

However I didn't run any performance tests, is that done by CI?

No. 😄 @bmeurer might have some decent benchmarks, afaik he was looking a lot into PromiseHook performance lately.

[AndreasMadsen]

Regarding internalNextTick itself, I think it is worth investigating how expensive it would be to switch to process.nextTick and set the kDefaultTriggerAsyncId instead. My intuition is that it wouldn't be very problematic and it would remove a lot of almost duplicate code.

[bmeurer]

@addaleax I don't have any benchmarks for the async_hooks except bmeurer/async-hooks-performance-impact. We lowered priority of async_hooks performance work until there's a clear direction and a number to express how much tankage is considered Okay.