It seems to fail the following 2 tests, which seem unrelated to the commit (and I'm not sure why the tests are being run against 1.8.2-beta.2):

Your build ran 903 tests with 2 failures

chromium feature PDF Viewer opens when loading a pdf resource in a iframe - opens when loading a pdf resource in a iframe
/home/builduser/project/spec/chromium-spec.js
Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

chromium feature PDF Viewer opens when loading a pdf resource in a nested iframe - opens when loading a pdf resource in a nested iframe
/home/builduser/project/spec/chromium-spec.js
Error: Timeout of 30000ms exceeded. For async tests and hooks, ensure "done()" is called; if returning a Promise, ensure it resolves.

@xaviergonz considering the same two tests failed across window, mac, and linux, i would say it's likely to be related. Did you test locally, and did those two tests pass there?

@xaviergonz since running CI takes a while, when you make changes like disabling things to see if tests pass I would highly recommend doing that locally instead of here to save time

@codebytere I just wanted to give hard proof that the commit was unrelated to the failing test :)

I'll just make one more commit disabling everything the commit does and if those test still fail then restore it as it was, would that be ok?

(also I do it here since I don't have the electron build environment set up locally on this machine at the moment)

ok, apparently the commit does really break that functionality, I'll restore the branch and set up a local env and fix there

(and I'm not sure why the tests are being run against 1.8.2-beta.2)

They're not. Looks like there are a few 1.8.2-beta.2 strings in master..

ok, I've been investigating and it seems that just adding an empty async hook with a single callback (whichever it is) such as this:

const asyncHooks = require('async_hooks')
const asyncHook = asyncHooks.createHook({
  init: (asyncId, type, triggerAsyncId) => {
  },
})
asyncHook.enable()

makes the pdf viewer (and only when inside an iframe) crash.

Unfortunately I haven't been able to make the crashreporter report why it crashes and I don't have enough knowledge of the pdfviewer plugin to trace it properly.

Any tips (even if it is just to see what the stacktrace of what makes the webcontent crash) would be appreciated.

Upon further inspection, the interaction seems to be caused by promise hooking made by node on async_hook.js:

if (prev_kTotals === 0 && hook_fields[kTotals] > 0) {
      enablePromiseHook();
      hook_fields[kCheck] += 1;
    }

if I disable enablePromiseHook() then the pdf test passes...

The failure might be due to a node bug in the end related to promise hooking (unconfirmed), see #12108

Confirmed, it is a node bug. This pull request should be ok once the node version is updated... I still have to confirm in which versions the fix is going to be applied though.

The relevant node pull request is here: nodejs/node#19134

I would be hesitant to make an entire platform rely on an API marked Experimental in Node core, especially when the monkeypatching the patch is replacing is relatively contained and innocuous. It's one thing if you're using async_hooks as an opt-in as an app developer (e.g. to apply APM instrumentation), but riskier to do it platform-wide. The code itself is pretty well worked-over (and it's marked as experimental more because it still has some blind spots (e.g. promise support is incomplete) and the API might change a bit, but there are definitely still some bugs. Aside from the fact that monkeypatching is seen as generically undesirable, does this PR address any bugs?

I would be -0 to landing this change if I had a vote.

@othiym23 I'd also be against making any changes if everything just worked (tm), but the fact is that stuff started breaking because of the monkey patching assumptions (see #12108 or #11086) and according to the own node developers it will break down even further in the future and this is the proper way to fix the use case.

Just to be clear, are you saying the Node developers are recommending that Electron switch to this API now, even though it's still marked as experimental?

Surely the future breakage won't happen while this is still in the experimental mode?

@ckerr that's right, I didn't even think of using that API to fix this problem, they were the ones suggesting to use it. see #12108

sorry I meant see nodejs/node#19104

@xaviergonz Thanks for the pointing again to the related issues, and my apologies for not having read through all that discussion in the first place. Now that I understand the motivation behind this PR, it makes a lot more sense. As the discussion on nodejs/node#19104 shows, there are very real consequences to the fact that async_hooks is still experimental (it does not surprise me at all that you ended up having to patch around something having to do do with async_hooks's efforts to deal with Promises). That said, I understand Node's position on keeping the internal nextTick hidden away from user code, and if they're willing to work with Electron (and you) to address the problems with async_hooks to make it work for Electron, that addresses my reservations.

As @ckerr says in #12108 (comment), though, that does create at least a temporary integration problem, so it may make sense to belay landing this PR until the bugs you and @addaleax discussed on nodejs/node#19104 have landed (and been backported to the requisite release lines) in Node.

No problem, there is quite a bit to read 😁

I also think that the PR should just be kept on hold until the fix is within node itself and then once electron is updated to use that particular version (whichever it is gonna be) then it can be merged.

Though using that approach means electron 1.8 is not gonna get patched though I guess (unless stable versions are allowed a node version bump) .

Remainder: The pdf rendering issues that show on CI won't get fixed until this lands in node:

nodejs/node#19134

so this can't be merged until so

the fix has finally landed in node 10.7

is there a path forward for this PR?

I think the concern here is that async_hooks is still experimental in node. If we can get someone from node to look at this usage and sign off that this use case is likely to continue to be supported (if perhaps not this exact API), then I think we should move forward.

So what's the best course of actions for anyone experiencing this currently? Any way to just straight up disable hooks? I'm having those errors at random times, last time was when electron app got throttled in the background and I have no idea what exactly is causing this. If it's impossible to turn them off on a per-app basis, which version of electron should I revert to, so it wouldn't have those hooks at all? (I'm currently using electron 6.12.0)