New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Port --debug-brk support to Node 6.3.0 #7001

Merged
merged 3 commits into from Aug 29, 2016

Conversation

Projects
None yet
3 participants
@kevinsawicki
Contributor

kevinsawicki commented Aug 26, 2016

In commit nodejs/node@4d4cfb2, some changes were made to how Node supports the --debug-brk flag that caused Electron's support for it to stop working in 1.3.0+.

From that commit's message:

Do not depend on the existence of global.v8debug as a mechanism to
determine if --debug-brk was specified.

Excerpts from that commit's diff:

-  if (debug_wait_connect) {
-    const char expose_debug_as[] = "--expose_debug_as=v8debug";
-    V8::SetFlagsFromString(expose_debug_as, sizeof(expose_debug_as) - 1);
-  }
+  // --debug-brk
+  if (debug_wait_connect) {
+    READONLY_PROPERTY(process, "_debugWaitConnect", True(env->isolate()));
+  }
-        if (global.v8debug &&
+        if (process._debugWaitConnect &&
-      global.v8debug.Debug.setListener(function() {});
-      global.v8debug.Debug.setBreakPoint(compiledWrapper, 0, 0);
+      delete process._debugWaitConnect;
+      const Debug = vm.runInDebugContext('Debug');
+      Debug.setBreakPoint(compiledWrapper, 0, 0);

This pull request sets process._debugWaitConnect to true when --debug-brk is specified on the command line and removes setting --expose_debug_as=v8debug which is no longer used by Node.

Screenshot of npm start -- --debug-brk working on master 馃憞
screen shot 2016-08-26 at 12 14 41 pm

Closes #6608

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Aug 26, 2016

/cc @ofrobots any feedback on this change? Should setting process._debugWaitConnect be sufficient?

@ofrobots

This comment has been minimized.

ofrobots commented Aug 26, 2016

That should be sufficient in theory, but _debugWaitConnect is intended to be an internal mechanism. Is there a way for you to pass flags (--debug-brk) to Node and let Node handle this internal business? I am not too familiar with how Electron embeds Node, so I am not sure if this is possible.

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Aug 26, 2016

Is there a way for you to pass flags (--debug-brk) to Node and let Node handle this internal business?

Hmm, yeah, the node integration in Electron is done via calling into node::CreateEnvironment, and the --debug-brk argument seems to be parsed via calling node::Start.

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Aug 26, 2016

@ofrobots would it be better to call node:: ParseDebugOpt directly with the argument?

@kevinsawicki

This comment has been minimized.

Contributor

kevinsawicki commented Aug 26, 2016

would it be better to call node:: ParseDebugOpt directly with the argument?

Nevermind, that isn't public right?

@ofrobots

This comment has been minimized.

ofrobots commented Aug 26, 2016

Hmm, yeah, the node integration in Electron is done via calling into node::CreateEnvironment, and the --debug-brk argument seems to be parsed via calling node::Start.

Yeah, that's unfortunate. Some of the flags do need to be parsed early, but the others (like this one) could theoretically be handled after Environment::Start(). Too bad this isn't how the code works today.

@zcbenz

This comment has been minimized.

Contributor

zcbenz commented Aug 29, 2016

馃憤

@zcbenz zcbenz merged commit ac9e64c into master Aug 29, 2016

7 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
electron-linux-arm Build #3968902 succeeded in 53s
Details
electron-linux-ia32 Build #3968903 succeeded in 47s
Details
electron-linux-x64 Build #3968904 succeeded in 82s
Details
electron-win-ia32 Build #1321 succeeded in 6 min 28 sec
Details
electron-win-x64 Build #1300 succeeded in 6 min 31 sec
Details

@zcbenz zcbenz deleted the debug-wait-connect branch Aug 29, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment