Skip to content
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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inspector: add support for uncaught exception #8043

Closed
wants to merge 1 commit into from
Closed

inspector: add support for uncaught exception #8043

wants to merge 1 commit into from

Conversation

alexkozy
Copy link
Member

Checklist
  • make -j4 test (UNIX), or vcbuild test nosign (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector

Description of change

To output exception in DevTools console method exceptionThrown should be
called on uncaught exception on V8Inspector object.
Additionally we need to wait disconnect to provide user way to inspect
exception.

@nodejs-github-bot nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Aug 10, 2016
@alexkozy
Copy link
Member Author

/cc @eugeneo @ofrobots @dgozman

@alexkozy
Copy link
Member Author

alexkozy commented Aug 10, 2016

This pull request is based on latest version of V8Inspector that hasn't pushed to master yet https://github.com/eugeneo/node/commit/2864bfdbf1d8448e4286b402b90a54bc3c2e36ef.
Please take a look. Only V8NodeInspector::inspector getter will change after latest V8Inspector merge.

I checked lint and test on latest version of V8Inspector.

return String16();
}
v8::Local<v8::String> string_value = v8::Local<v8::String>::Cast(value);
std::unique_ptr<UChar[]> buffer(new UChar[string_value->Length()]);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You probably cannot use std::unique_ptr as long as we still support OS X 10.8 and 10.9.

(I'm not 100% sure about 10.9 but 10.8 certainly doesn't have it, not even in tr1.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect this may be using the std::unique_ptr leaking out of the v8_inspector headers. A fake stl implementation was added to allow v8_inspector to work w/ Node.js v6.x. I can build this on my mac, with the Node.js build default of -mmacosx-version-min=10.7.

@alexkozy
Copy link
Member Author

Thanks!
Addressed all comments.

@ofrobots ofrobots mentioned this pull request Aug 10, 2016
2 tasks
v8::Local<v8::Context> context = env->context();

int script_id = message->GetScriptOrigin().ScriptID()->Value();
std::unique_ptr<blink::V8StackTrace> stack_trace =
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you replace the std::unique_ptr?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to get this pointer, use it to check scriptId and then pass it back to V8Inspector::exceptionThrown API call and I prefer to use unique_ptr here.
I can use auto instead of unique_ptr to hide real type of stackTrace. WDYT?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like @ofrobots is right that it picks up a non-system std::unique_ptr because this PR builds for me on OS X 10.8. If you want to restore the other use, go ahead.

@alexkozy
Copy link
Member Author

I've rebased. It's ready for real review.
Please take a look!

@bnoordhuis
Copy link
Member

LGTM

@jasnell
Copy link
Member

jasnell commented Aug 12, 2016

@jasnell
Copy link
Member

jasnell commented Aug 12, 2016

LGTM if CI is green.

@ofrobots
Copy link
Contributor

@ak239: there were some failures in the CI. Can you take a look?

To output exception in DevTools console method exceptionThrown should be
called on uncaught exception on V8Inspector object.
Additionally we need to wait disconnect to provide user way to inspect
exception.
@alexkozy
Copy link
Member Author

I've fixed compilation and uploaded new version.

@ofrobots
Copy link
Contributor

@ofrobots
Copy link
Contributor

LGTM.

ofrobots pushed a commit that referenced this pull request Aug 15, 2016
To output exception in DevTools console method exceptionThrown should
be called on uncaught exception on V8Inspector object. Additionally
we need to wait disconnect to provide user way to inspect exception.

PR-URL: #8043
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
@ofrobots
Copy link
Contributor

Thanks. Landed as 26cd48f.

@ofrobots ofrobots closed this Aug 15, 2016
evanlucas pushed a commit that referenced this pull request Aug 20, 2016
To output exception in DevTools console method exceptionThrown should
be called on uncaught exception on V8Inspector object. Additionally
we need to wait disconnect to provide user way to inspect exception.

PR-URL: #8043
Reviewed-By: bnoordhuis - Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: jasnell - James M Snell <jasnell@gmail.com>
Reviewed-By: ofrobots - Ali Ijaz Sheikh <ofrobots@google.com>
evanlucas added a commit that referenced this pull request Aug 24, 2016
Notable changes:

* **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154
* **inspector**:
  * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021
  * add support for uncaught exception (Aleksei Koziatinskii) #8043
* **meta**: add @joshgav to collaborators (Josh Gavant) #8146
* **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145
* ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143
evanlucas added a commit that referenced this pull request Aug 26, 2016
Notable changes:

* **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154
* **deps**: update V8 to 5.1.281.75 (Ali Ijaz Sheikh) #8054
* **inspector**:
  * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021
  * add support for uncaught exception (Aleksei Koziatinskii) #8043
* **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145
* ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143

PR-URL: #8253
evanlucas added a commit that referenced this pull request Aug 26, 2016
Notable changes:

* **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154
* **deps**: update V8 to 5.1.281.75 (Ali Ijaz Sheikh) #8054
* **inspector**:
  * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021
  * add support for uncaught exception (Aleksei Koziatinskii) #8043
* **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145
* ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143

PR-URL: #8253
evanlucas added a commit that referenced this pull request Aug 26, 2016
Notable changes:

* **buffer**: Fix regression introduced in v6.4.0 that prevented .write() at buffer end (Anna Henningsen) #8154
* **deps**: update V8 to 5.1.281.75 (Ali Ijaz Sheikh) #8054
* **inspector**:
  * fix inspector hang while disconnecting (Aleksei Koziatinskii) #8021
  * add support for uncaught exception (Aleksei Koziatinskii) #8043
* **repl**: Fix saving editor mode text in `.save` (Prince J Wesley) #8145
* ***Revert*** "**repl,util**: insert carriage returns in output" (Evan Lucas) #8143

PR-URL: #8253
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants