inspector: enable TimeTravel support #244

kfarnung · 2017-05-15T21:32:32Z

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

Affected core subsystem(s)

chakrashim, inspector

kfarnung · 2017-05-15T21:33:55Z

CI: https://ci.nodejs.org/job/chakracore-test-pull-request/33/ - PASSED

Edit: With the way we currently handle node tests in node-chakracore, the test passes will always end with "UNSTABLE" status. For the purposes of CI we consider that to be a passing run.

kfarnung · 2017-05-15T21:39:54Z

Resolves #228

kunalspathak · 2017-05-15T23:33:42Z

src/node.cc

  // Start debug agent when argv has --debug
-  if (debug_enabled)
+  if (debug_options.debugger_enabled() || debug_options.inspector_enabled())


debug_options.debugger_enabled() [](start = 6, length = 32)

This is no longer present in upstream. See if you can get rid of this.

Did we take that change yet or is the hope that I just eliminate it now in preparation for that change?

I am planning to do it in a day or 2. So yeah, consider it as prep work.

In reply to: 116625440 [](ancestors = 116625440)

kunalspathak · 2017-05-16T00:05:45Z

deps/chakrashim/src/inspector/v8-inspector-session-impl.cc

@@ -28,7 +31,9 @@ bool V8InspectorSession::canDispatchMethod(const StringView& method) {
         stringViewStartsWith(method,
                              protocol::Console::Metainfo::commandPrefix) ||
         stringViewStartsWith(method,
-                              protocol::Schema::Metainfo::commandPrefix);
+                              protocol::Schema::Metainfo::commandPrefix) ||
+         stringViewStartsWith(method,


stringViewStartsWith [](start = 9, length = 20)

not too familiar with this code, but does this have to be inside TTT if-condition?

I thought about it and decided not to mess with the dispatcher dynamically. The TTD dispatcher will be wired up, but any attempts to use it will return an error.

Since we already have code under ENABLE_TTD_NODE might be good to have all this under same

In reply to: 116629150 [](ancestors = 116629150)

There are currently no usages of ENABLE_TTD_NODE anywhere in chakrashim. I even see one instance where it's present, but commented out, so I suspect it's not possible.

kunalspathak · 2017-05-16T00:06:02Z

deps/chakrashim/src/inspector/v8-inspector-session-impl.cc

+  m_timeTravelAgent = wrapUnique(new V8TimeTravelAgentImpl(
+      this, this, agentState(protocol::TimeTravel::Metainfo::domainName)));
+  protocol::TimeTravel::Dispatcher::wire(&m_dispatcher,
+                                         m_timeTravelAgent.get());


same here. Should they be only if TTT is enabled?

I thought about it and decided not to mess with the dispatcher dynamically. The TTD dispatcher will be wired up, but any attempts to use it will return an error.

kunalspathak · 2017-05-16T00:07:38Z

deps/chakrashim/src/inspector/v8-debugger-agent-impl.cc

+  m_debugger->reverse();
+}
+
+void V8DebuggerAgentImpl::stepBack(ErrorString* errorString) {


stepBack [](start = 26, length = 8)

should we add unit test or open an issue to add later?

It's already the list in #227.

kfarnung · 2017-05-16T18:27:21Z

@agarwal-sandeep Can you take a look when you have a few minutes?

agarwal-sandeep · 2017-05-16T18:42:50Z

deps/chakrashim/include/v8-debug.h

@@ -91,7 +91,7 @@ class V8_EXPORT Debug {
  static Local<Context> GetDebugContext(Isolate* isolate);

  static void EnableDebug();
-  static void EnableInspector();
+  static void EnableInspector(bool enableReplayDebug = false);


EnableInspector [](start = 14, length = 15)

This function is not in deps/v8/include/v8-debug.h generally we keep V8 header same between v8 and chakrashim. Is this specific to chakrashim?

As far as I could see there was no EnableDebug function defined either, so I didn't worry about it. The calls to both functions were gated behind CHAKRACORE conditions.

https://github.com/nodejs/node-chakracore/search?utf8=%E2%9C%93&q=enabledebug&type=

Seems to remember that it was present when I implemented debugger support. If not I am fine with adding new function.

If it was present, it no longer is. Looking at the usage, I suspect we added it as a workaround.

EDIT: Seems like it was added in 99d6392 as part of the debugger enablement work.

agarwal-sandeep · 2017-05-16T18:46:08Z

deps/chakrashim/src/inspector/v8-debugger-agent-impl.h

@@ -171,7 +175,14 @@ class V8DebuggerAgentImpl : public protocol::Debugger::Backend {
  using DebugServerBreakpointToBreakpointIdAndSourceMap =
      protocol::HashMap<String16, std::pair<String16, BreakpointSource>>;

-  enum DebuggerStep { NoStep = 0, StepInto, StepOver, StepOut };
+  enum DebuggerStep {


DebuggerStep [](start = 7, length = 12)

Good to keep this in same order as JsDiagStepType

The streams don't ever cross currently, but I've updated it.

agarwal-sandeep · 2017-05-16T18:50:21Z

deps/chakrashim/src/inspector/v8-timetravel-agent-impl.cc

+#include "src/inspector/v8-inspector-session-impl.h"
+#include "src/inspector/v8-debugger-agent-impl.h"
+
+#include "src/jsrtinspector.h"


nit: remove extra empty lines

agarwal-sandeep · 2017-05-16T18:51:03Z

deps/chakrashim/src/inspector/v8-timetravel-agent-impl.cc

+
+bool V8TimeTravelAgentImpl::checkEnabled(ErrorString* errorString) {
+  if (enabled()) {
+    return true;


return true; [](start = 4, length = 12)

Initialize errorString to nullptr for this case? Same for reverse and stepBack functions

The pattern elsewhere is to only touch the variable in cases where there's an error. This was consistent with the state of the V8 code at the time it was forked.

agarwal-sandeep · 2017-05-16T18:58:26Z

Beside some general changes the code looks good to me. Please incorporate feedback and merge if CI is clean.

If the debugger attempts to set a breakpoint that gets resolved to an existing breakpoint, then when we go to clear the same breakpoint will be removed twice. On the second removal the API returns JsErrorInvalidArgument which is a no-op for this function. PR-URL: nodejs#244 Reviewed-By: Kunal Pathak <Kunal.Pathak@microsoft.com> Reviewed-By: Sandeep Agarwal <saagarwa@microsoft.com>

* Added the TimeTravel domain and wired it up * Debuggers can now detect time travel mode by looking at the available domains. PR-URL: nodejs#244 Reviewed-By: Kunal Pathak <Kunal.Pathak@microsoft.com> Reviewed-By: Sandeep Agarwal <saagarwa@microsoft.com>

kfarnung self-assigned this May 15, 2017

kfarnung requested review from digitalinfinity, agarwal-sandeep, kunalspathak and mrkmarron May 15, 2017 21:32

kunalspathak reviewed May 15, 2017

View reviewed changes

kfarnung force-pushed the ttd branch from 39623c4 to f2e06f2 Compare May 15, 2017 23:47

kunalspathak reviewed May 16, 2017

View reviewed changes

kfarnung mentioned this pull request May 16, 2017

Enable and fix the inspector tests #227

Open

3 tasks

agarwal-sandeep reviewed May 16, 2017

View reviewed changes

kfarnung force-pushed the ttd branch 2 times, most recently from 90d839e to 828656c Compare May 16, 2017 19:26

kfarnung added 2 commits May 17, 2017 13:32

kfarnung force-pushed the ttd branch from 828656c to f3d3097 Compare May 17, 2017 20:32

kfarnung merged commit f3d3097 into nodejs:xplat May 17, 2017

kfarnung deleted the ttd branch May 17, 2017 20:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

inspector: enable TimeTravel support #244

inspector: enable TimeTravel support #244

kfarnung commented May 15, 2017

kfarnung commented May 15, 2017 •

edited

Loading

kfarnung commented May 15, 2017 •

edited

Loading

kunalspathak May 15, 2017

kfarnung May 15, 2017

kunalspathak May 16, 2017

kunalspathak May 16, 2017

kfarnung May 16, 2017

agarwal-sandeep May 16, 2017

kfarnung May 16, 2017

kunalspathak May 16, 2017

kfarnung May 16, 2017

kunalspathak May 16, 2017

kfarnung May 16, 2017

kfarnung commented May 16, 2017

agarwal-sandeep May 16, 2017

kfarnung May 16, 2017

agarwal-sandeep May 16, 2017

kfarnung May 16, 2017 •

edited

Loading

agarwal-sandeep May 16, 2017

kfarnung May 16, 2017

agarwal-sandeep May 16, 2017

agarwal-sandeep May 16, 2017 •

edited

Loading

kfarnung May 16, 2017 •

edited

Loading

agarwal-sandeep commented May 16, 2017

inspector: enable TimeTravel support #244

inspector: enable TimeTravel support #244

Conversation

kfarnung commented May 15, 2017

Checklist

Affected core subsystem(s)

kfarnung commented May 15, 2017 • edited Loading

kfarnung commented May 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfarnung commented May 16, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfarnung May 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

agarwal-sandeep May 16, 2017 • edited Loading

Choose a reason for hiding this comment

kfarnung May 16, 2017 • edited Loading

Choose a reason for hiding this comment

agarwal-sandeep commented May 16, 2017

kfarnung commented May 15, 2017 •

edited

Loading

kfarnung commented May 15, 2017 •

edited

Loading

kfarnung May 16, 2017 •

edited

Loading

agarwal-sandeep May 16, 2017 •

edited

Loading

kfarnung May 16, 2017 •

edited

Loading