Repository navigation
new internalNextTick cannot be wrapped by electron #19104
Description
Activity
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
setUnrefTimeoutwhich seems affected by this.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,TimeoutorTickObject.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?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.
@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?
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
There's no chance we'll expose
internalNextTickon the process object.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...
Reacted by Anna HenningsenI 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 crashhowever my knowledge of that part of the node code is kind of limited. Is there any reason you may know why this might happen?
Reacted by Anna HenningsenNot sure. Totally possible there's a bug on our end or V8s. I'll ping some folks...
Reacted by Javier GonzalezIs there any reason you may know why this might happen?
@xaviergonz Yes. I actually brought this up with @gsathya – the V8
PromiseHookAPI 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 internalEnvironmentbased on the V8Context.For that to work, Node first needs to set an embedder field of that
v8::Contextexplicitly; the method that does this isEnvironment::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
Environmentis 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 aEnvironmentto 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
undefinedvalue?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 )
If what you mean is what's the value of promise->IsNullOrUndefined(), then it is false at crash time.
@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 (32is the default value fornode::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 isundefined.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!
Makes me wonder though, is this fix going to be applied? If so, under node v8, v9 or both?
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.
- added 2 commits that reference this issue
on Mar 4, 2018 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?
Regarding
internalNextTickitself, I think it is worth investigating how expensive it would be to switch toprocess.nextTickand set thekDefaultTriggerAsyncIdinstead. My intuition is that it wouldn't be very problematic and it would remove a lot of almost duplicate code.@addaleax I don't have any benchmarks for the
async_hooksexcept bmeurer/async-hooks-performance-impact. We lowered priority ofasync_hooksperformance work until there's a clear direction and a number to express how much tankage is considered Okay.- added a commit that references this issue
on Mar 8, 2018 - added a commit that references this issue
on May 8, 2018 - added a commit that references this issue
on Jun 26, 2018 - added a commit that references this issue
on Jul 15, 2018 - added a commit that references this issue
on Jul 16, 2018
Due to the change from using
process.nextTicktoconst { nextTick } = require('internal/process/next_tick');electron cannot wrap that call anymore in order to use its internalprocess.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.