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.