inspector: use final API #9028

eugeneo · 2016-10-11T16:39:50Z

Checklist

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

Affected core subsystem(s)

deps: new version of the v8_inspector dependency
inspector: updated the code to use the new API

Description of change

This implementation switches to V8 inspector from the V8 repository. The
new inspector integration is now using final APIs and exposes a stable
wire protocol, removing the need for pointing the users to specific
devtools version.

CC: @ofrobots

eugeneo · 2016-10-11T16:52:21Z

I'll be looking into CI failures - CI UI seems to be down, so this might take some time.

jasnell

Rubber stamp LGTM once CI is good.

bnoordhuis · 2016-10-11T18:24:51Z

src/inspector_agent.cc

+        view.length());
+
+  // Reserve 1 character for terminator.
+  std::string result(view.length() * sizeof(UChar) + 1, '\0');


sizeof(UChar) doesn't look self-evidently correct. I assume it's shorthand for sizeof(*view.characters16()) (which is sizeof(uint16_t)) but code points >= 0x800 encode to more than two bytes.

Thanks. I changed the sizeof & allowed the code to resize the buffer if need be.

bnoordhuis · 2016-10-11T18:27:21Z

src/inspector_agent.cc

 public:
  explicit ChannelImpl(AgentImpl* agent): agent_(agent) {}
  virtual ~ChannelImpl() {}
 private:
-  void sendProtocolResponse(int callId, const String16& message) override {
+  void sendProtocolResponse(int callId,
+      const v8_inspector::StringView& message) override {


Style nit: can you line break at the ( or line up the second argument?

There's no longer need for a line break here.

bnoordhuis · 2016-10-11T18:28:28Z

src/inspector_agent.cc

+    const char CONTEXT_NAME[] = "NodeJS Main Context";
+    inspector_->contextCreated(v8_inspector::V8ContextInfo(env->context(), 1,
+        StringView(reinterpret_cast<const uint8_t*>(CONTEXT_NAME),
+            sizeof(CONTEXT_NAME) - 1)));


Style nit: can you line up arguments more here?

I rearranged the code.

bnoordhuis · 2016-10-11T18:30:17Z

src/inspector_agent.cc

+  size_t len = string_value->Length();
+  std::basic_string<uint16_t> buffer(len, '\0');
+  string_value->Write(&buffer[0], 0, len);
+  return StringBuffer::create(StringView(buffer.c_str(), len));


For my own curiosity, is .c_str() rather than .data() necessary here if you also pass len?

Coding mistake.

bnoordhuis · 2016-10-11T18:31:13Z

src/inspector_agent.cc

+    if (wait_) {
+      std::string message(buf->base, read);
+      if (message.find("\"Runtime.runIfWaitingForDebugger\"")
+          != std::string::npos) {


Can you put the operator on the line before?

bnoordhuis · 2016-10-11T18:31:49Z

src/inspector_agent.cc

-  if (AppendMessage(&incoming_message_queue_, frontend_session_id_, message)) {
+void AgentImpl::PostIncomingMessage(const char* message, size_t len) {
+  if (AppendMessage(&incoming_message_queue_, frontend_session_id_,
+      Utf8ToStringView(message, len))) {


Style nit: can you line up the arguments?

bnoordhuis · 2016-10-11T18:32:07Z

src/inspector_agent.cc

-  AppendMessage(&outgoing_message_queue_, session_id, message);
+void AgentImpl::Write(int session_id, const StringView& inspector_message) {
+  AppendMessage(&outgoing_message_queue_, session_id,
+      StringBuffer::create(inspector_message));


bnoordhuis · 2016-10-11T18:33:22Z

src/inspector_agent.cc

+      std::string tag;
+      if (message.length() <= sizeof(TAG_CONNECT) ||
+          message.length() <= sizeof(TAG_DISCONNECT))
+        tag = StringViewToUtf8(message);


It's not exactly wrong but I'd wrap the body in braces for legibility.

eugeneo · 2016-10-13T22:02:10Z

@bnoordhuis I addressed your comments, please take another look.

CI: https://ci.nodejs.org/job/node-test-pull-request/4506/

Linux ARM failure: exception in Hudson on one bot.
Free BSD: some tests fail on one bot and run on another. The tests do not start inspector.
SmartOS: CI success, not reported to this PR.

eugeneo · 2016-10-18T17:28:51Z

@bnoordhuis Please take another look.

bnoordhuis · 2016-10-20T12:10:21Z

src/inspector_agent.cc

 #include "libplatform/libplatform.h"

+#include <unicode/unistr.h>


If you're taking a dependency on ICU, then configure should have a --with-intl=none -> --without-inspector implication.

I modified configure - now disabling either ICU or OpenSSL will also disable inspector.

bnoordhuis · 2016-10-20T12:11:04Z

src/inspector_agent.cc

+std::string StringViewToUtf8(const StringView& view) {
+  if (view.is8Bit())
+    return std::string(reinterpret_cast<const char*>(view.characters8()),
+        view.length());


Can you line up the arguments and maybe put braces around the block?

bnoordhuis · 2016-10-20T12:12:51Z

src/inspector_agent.cc

+
+  const uint16_t* source = view.characters16();
+  std::string result(view.length() * sizeof(*source), '\0');
+  UnicodeString utf16(reinterpret_cast<const UChar*>(source), view.length());


Is this reinterpret_cast necessary? If for some reason UChar != uint16_t, then this line should arguably fail to compile instead of silently doing the wrong thing.

Windows uses some different type for UChar (not clear from compiler messages - but probably wchar).

bnoordhuis · 2016-10-20T12:13:36Z

src/inspector_agent.cc

+    CheckedArrayByteSink sink(&result[0], result.length());
+    utf16.toUTF8(sink);
+    result.resize(sink.NumberOfBytesAppended());
+    done = !sink.Overflowed();


Does that mean this function may return partial strings? How bad is that?

Not sure what you mean here. This code makes sure string has enough capacity.

Correct me if I'm wrong but CheckedArrayByteSink is instantiated every time with the same arguments in this loop so logically sink.Overflowed() is either always true or always false. That means this loop either isn't (i.e., it's not a loop because it exits after the first iteration) or it's an infinite loop.

bnoordhuis · 2016-10-20T12:15:05Z

src/inspector_agent.cc

+std::unique_ptr<StringBuffer> Utf8ToStringView(const char* source,
+                                               size_t length) {
+  UnicodeString utf16 = UnicodeString::fromUTF8(StringPiece(source, length));
+  StringView view(reinterpret_cast<const uint16_t*>(utf16.getBuffer()),


Same question as on line 212: is the reinterpret_cast necessary?

bnoordhuis · 2016-10-20T12:15:36Z

src/inspector_agent.cc

@@ -350,8 +373,10 @@ class V8NodeInspector : public v8_inspector::V8InspectorClient {
                    terminated_(false),
                    running_nested_loop_(false),
                    inspector_(V8Inspector::create(env->isolate(), this)) {
-    inspector_->contextCreated(
-        v8_inspector::V8ContextInfo(env->context(), 1, "NodeJS Main Context"));
+    const uint8_t CONTEXT_NAME[] = "NodeJS Main Context";


While you're here: s/NodeJS/Node.js/

Thanks! Done.

bnoordhuis · 2016-10-20T12:16:33Z

src/inspector_agent.cc

-      String16::fromInteger(script_id) == stack_trace->topScriptId()) {
+  v8::Local<v8::StackTrace> stack_trace = message->GetStackTrace();
+
+  if (!stack_trace.IsEmpty() && stack_trace->GetFrameCount() &&


Can you put each clause on its own line?

bnoordhuis · 2016-10-20T12:18:03Z

src/inspector_agent.cc

+      StringView message = pair.second->string();
+      std::string tag;
+      if (message.length() <= sizeof(TAG_CONNECT) ||
+          message.length() <= sizeof(TAG_DISCONNECT)) {


eugeneo · 2016-10-21T00:36:31Z

@bnoordhuis Thank you for the review. I've updated the code, please take another look.

eugeneo · 2016-10-25T16:26:01Z

@bnoordhuis Please, take another look.

bnoordhuis · 2016-10-25T19:20:11Z

configure

+def configure_inspector(o):
+  o['variables']['v8_inspector'] = b(not options.without_inspector
+                                         and o['variables']['v8_enable_i18n_support']
+                                         and o['variables']['node_use_openssl'] == 'true')


Can you use options here? That way it won't depend on the order in which the configure_intl an/configure_openssl/configure_inspector functions are executed.

Also, please keep lines <= 80 columns.

bnoordhuis · 2016-10-25T19:24:59Z

src/inspector_agent.cc

+    CheckedArrayByteSink sink(&result[0], result.length());
+    utf16.toUTF8(sink);
+    result.resize(sink.NumberOfBytesAppended());
+    done = !sink.Overflowed();


Correct me if I'm wrong but CheckedArrayByteSink is instantiated every time with the same arguments in this loop so logically sink.Overflowed() is either always true or always false. That means this loop either isn't (i.e., it's not a loop because it exits after the first iteration) or it's an infinite loop.

bnoordhuis · 2016-10-25T19:25:49Z

src/inspector_agent.cc

+  v8::Local<v8::StackTrace> stack_trace = message->GetStackTrace();
+
+  if (!stack_trace.IsEmpty() &&
+      stack_trace->GetFrameCount() &&


Can you write this as stack_trace->GetFrameCount() > 0?

eugeneo · 2016-10-25T20:54:56Z

@bnoordhuis thank you for the review, I uploaded a new version - please take a look.

GitHub Ui is not allowing me to answer this comment (does not show the reply entry field for some reason):

Correct me if I'm wrong but CheckedArrayByteSink is instantiated every time with the same arguments in this loop so logically sink.Overflowed() is either always true or always false. That means this loop either isn't (i.e., it's not a loop because it exits after the first iteration) or it's an infinite loop.

The code was relying on string length, that was updated in the loop. I changed the code to make it more obvious.

bnoordhuis

LGTM with some final comments. I think I understand how sinking works now.

bnoordhuis · 2016-10-25T21:07:20Z

configure

+def configure_inspector(o):
+  disable_inspector = (options.without_inspector
+                       or options.with_intl in (None, 'none')
+                       or options.without_ssl)


I realize this is nitpicking but since we put operators at the end of the line everywhere else can you do it here too?

bnoordhuis · 2016-10-25T21:07:34Z

configure

@@ -1250,6 +1255,8 @@ configure_v8(output)
 configure_openssl(output)
 configure_intl(output)
 configure_static(output)
+# should come after configure_openssl and configure_intl


This comment is no longer necessary.

bnoordhuis · 2016-10-25T21:09:28Z

src/inspector_agent.cc

+  const uint16_t* source = view.characters16();
+  size_t result_length = view.length() * sizeof(*source);
+  std::string result(result_length, '\0');
+  UnicodeString utf16(reinterpret_cast<const UChar*>(source), view.length());


If the reinterpret_cast is non-optional here, can you at least add a static_assert that checks the pointed-to types are of the same size? See #9280 for an example of what I mean.

eugeneo · 2016-10-25T21:22:33Z

Thank you for the review, I addressed the comments and submitted to CI: https://ci.nodejs.org/job/node-test-pull-request/4674/

eugeneo · 2016-10-26T16:45:40Z

Landed as 8f00455

This implementation switches to V8 inspector from the V8 repository. The new inspector integration is now using final APIs and exposes a stable wire protocol, removing the need for pointing the users to specific devtools version. PR-URL: #9028 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>

MylesBorins · 2016-11-18T21:50:25Z

hey @eugeneo this seemed to break configure on v6.x

Would you be willing to manually backport

eugeneo · 2016-11-18T21:52:16Z

Let me take a look.

On Fri, Nov 18, 2016 at 1:50 PM Myles Borins notifications@github.com
wrote:

hey @eugeneo https://github.com/eugeneo this seemed to break configure
on v6.x

Would you be willing to manually backport

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
#9028 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARkrQ5CyRRfH7_y8ajVFRyc6bx-Fwtaks5q_h2kgaJpZM4KT2Fh
.

eugeneo · 2016-11-18T22:11:10Z

@thealphanerd backporting this inspector to V8 5.1 is a significant effort. Can we opt out of it? I am not familiar with the process.

MylesBorins · 2016-11-19T00:07:34Z

@eugeneo we absolutely can opt out. Considering that v6.x has over 2 years of support left I'd like to get something as up to date as possible though. At what point will we no longer be able to update the inspector that we ship in v6?

eugeneo · 2016-11-19T00:57:59Z

I guess this is the point inspector stops getting updated... The new
inspector that is in Node7 is the one imported from the V8 repository (as
opposed to the Blink repository) so it relies on new V8 build setup and
probably some APIs. In the past we had to do some extra work to support
older V8 that the Node uses - but that was lost with the code moving to the
V8 repository.

On Fri, Nov 18, 2016 at 4:08 PM Myles Borins notifications@github.com
wrote:

@eugeneo https://github.com/eugeneo we absolutely can opt out.
Considering that v6.x has over 2 years of support left I'd like to get
something as up to date as possible though. At what point will we no longer
be able to update the inspector that we ship in v6?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#9028 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AARkrbf_GPJxY3-YJmNzvjVy_ixxidKpks5q_j3IgaJpZM4KT2Fh
.

nodejs#9028 made Node.js use the V8 inspector bundled with the deps/v8 and removed the third party dependency. We can safely omit the non-existent license file.

nodejs-github-bot added the inspector Issues and PRs related to the V8 inspector protocol label Oct 11, 2016

eugeneo added the c++ Issues and PRs that require attention from people who are familiar with C++. label Oct 11, 2016

jasnell approved these changes Oct 11, 2016

View reviewed changes

bnoordhuis requested changes Oct 11, 2016

View reviewed changes

joshgav mentioned this pull request Oct 11, 2016

inspector,doc: update hint text, add guide #8978

Closed

4 tasks

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

bnoordhuis requested changes Oct 20, 2016

View reviewed changes

bnoordhuis requested changes Oct 25, 2016

View reviewed changes

eugeneo mentioned this pull request Oct 25, 2016

inspector: add contexts when using vm module #9272

Closed

3 tasks

bnoordhuis approved these changes Oct 25, 2016

View reviewed changes

eugeneo closed this Oct 26, 2016

eugeneo merged commit 8f00455 into nodejs:master Oct 26, 2016

evanlucas mentioned this pull request Nov 3, 2016

v7.1.0 proposal - 2016-11-08 #9438

Merged

MylesBorins added dont-land-on-v4.x labels Nov 18, 2016

Qard mentioned this pull request Nov 19, 2016

backport: debugger: make listen address configurable #9692

Closed

2 tasks

eugeneo deleted the inspector_icu branch December 27, 2016 18:49

thefourtheye mentioned this pull request Feb 11, 2017

update licenses and remove v8_inspector deps license #11310

Closed

2 tasks

MylesBorins added the backport-requested-v6.x label May 15, 2017

		#include "libplatform/libplatform.h"

		#include <unicode/unistr.h>

inspector: use final API #9028

inspector: use final API #9028

Conversation

eugeneo commented Oct 11, 2016

Checklist

Affected core subsystem(s)

Description of change

eugeneo commented Oct 11, 2016

jasnell left a 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

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

eugeneo commented Oct 13, 2016

eugeneo commented Oct 18, 2016

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

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

eugeneo commented Oct 21, 2016

eugeneo commented Oct 25, 2016

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

eugeneo commented Oct 25, 2016

bnoordhuis left a 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

eugeneo commented Oct 25, 2016

eugeneo commented Oct 26, 2016

MylesBorins commented Nov 18, 2016

eugeneo commented Nov 18, 2016

eugeneo commented Nov 18, 2016

MylesBorins commented Nov 19, 2016

eugeneo commented Nov 19, 2016