Skip to content
This repository has been archived by the owner on Oct 15, 2020. It is now read-only.

chakrashim: check return value for inspector call #261

Merged
merged 1 commit into from
May 26, 2017
Merged

chakrashim: check return value for inspector call #261

merged 1 commit into from
May 26, 2017

Conversation

kfarnung
Copy link
Contributor

The return value for JsDiagGetScripts was not being validated and
continuing on during an error condition.

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

chakrashim

@kfarnung kfarnung self-assigned this May 24, 2017
digitalinfinity
digitalinfinity approved these changes May 24, 2017
View reviewed changes
deps/chakrashim/src/inspector/v8-debugger.cc Outdated
@@ -77,22 +77,25 @@ void V8Debugger::getCompiledScripts(
int contextGroupId,
std::vector<std::unique_ptr<V8DebuggerScript>>& result) {
JsValueRef scripts;
JsDiagGetScripts(&scripts);
if (JsDiagGetScripts(&scripts) != JsNoError) {
assert(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this assert needed? Seems like this is a legitimate situation that just needs to be handled?

Copy link
Member

Choose a reason for hiding this comment

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

I agree.

Copy link
Contributor

Choose a reason for hiding this comment

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

Looking at code only expected failure is JsErrorDiagNotInDebugMode, in all other case you should at least get an empty array, so assert in all other cases.

Copy link
Member

Choose a reason for hiding this comment

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

@kfarnung - curious, is this failing any unit test? I am doing a merge and this test case is failing because of this issue. I will ignore this failure for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kunalspathak This was discovered due to that test case, but it won't actually fix it. The test case needs to be marked flaky.

deps/chakrashim/src/inspector/v8-debugger.cc Outdated
if (jsrt::InspectorHelpers::GetIntProperty(scripts, "length", &length) != JsNoError) {
assert(false);
return;
}

for (int i = 0; i < length; i++) {
JsValueRef index;
JsValueRef index = nullptr;
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably better to initialize these to JS_INVALID_REFERENCE

@kfarnung
Copy link
Contributor Author

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

@kfarnung
Copy link
Contributor Author

@agarwal-sandeep @digitalinfinity Can you take another look? I made changes to remove asserts and fast fail in more unexpected scenarios. I also return an error to the calls if they attempt to enable inspector when the launch argument is missing.

agarwal-sandeep
agarwal-sandeep reviewed May 25, 2017
View reviewed changes
deps/chakrashim/src/inspector/v8-debugger.cc Outdated

return false;
}

JavaScriptCallFrames V8Debugger::currentCallFrames(int limit) {
if (!m_isolate->InContext()) return JavaScriptCallFrames();

JsValueRef stackTrace;
JsValueRef stackTrace = JS_INVALID_REFERENCE;
JsDiagGetStackTrace(&stackTrace);

int length;
Copy link
Contributor

Choose a reason for hiding this comment

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

int length; [](start = 2, length = 11)

Initialize to 0?

agarwal-sandeep
agarwal-sandeep reviewed May 25, 2017
View reviewed changes
deps/chakrashim/src/inspector/v8-debugger.cc Outdated
@@ -365,50 +357,47 @@ void V8Debugger::setAsyncCallStackDepth(V8DebuggerAgentImpl* agent, int depth) {
void V8Debugger::asyncTaskScheduled(const StringView& taskName, void* task,
bool recurring) {
// CHAKRA-TODO - Figure out what to do here
assert(false);
CHAKRA_VERIFY(false);
Copy link
Contributor

Choose a reason for hiding this comment

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

CHAKRA_VERIFY(false); [](start = 2, length = 21)

CHAKRA_UNIMPLEMENTED is more appropriate for all non-implemented functions

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, I forgot about that one.

agarwal-sandeep
agarwal-sandeep reviewed May 25, 2017
View reviewed changes
deps/chakrashim/src/inspector/v8-debugger.cc Outdated
}
CHAKRA_VERIFY_NOERROR(jsrt::InspectorHelpers::HasProperty(eventData,
"breakpointId",
&hasBreakpointId));

if (hasBreakpointId) {
breakpointIds.reserve(1);

int breakpointId;
Copy link
Contributor

Choose a reason for hiding this comment

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

breakpointId [](start = 8, length = 12)

initialize to 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I caught all the rest of these now.

agarwal-sandeep
agarwal-sandeep reviewed May 25, 2017
View reviewed changes
src/node.cc Outdated
@@ -4786,7 +4786,7 @@ inline int Start_TTDReplay(Isolate* isolate, void* isolate_context,
// Start debug agent when argv has --debug
StartDebug(&env, nullptr, debug_options);

if (debug_options.inspector_enabled())
if (debug_options.inspector_enabled() && !v8_platform.InspectorStarted(&env))
Copy link
Contributor

Choose a reason for hiding this comment

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

!v8_platform.InspectorStarted(&env) [](start = 43, length = 35)

StartDebug above already does this check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is just matching the pattern used by the non-TTD start method.

@agarwal-sandeep
Copy link
Contributor

Rest looks good to me

@kfarnung kfarnung force-pushed the inspectfix branch 2 times, most recently from 08a893a to 7d081b0 Compare May 25, 2017 21:39
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request May 25, 2017
@kfarnung
chakrashim: fix inspector asserts
7d081b0 
* Added an error that's returned when debugger can't be enabled
* Added validation for the JsGetScripts return value
* Switched to CHAKRA_VERIFY(_NOERROR) macros for all asserted calls
* Fixed uninitialized variables
* Fixed line lengths
* Switched to CHAKRA_UNIMPLEMENTED for unimplemented methods

PR-URL: nodejs#261
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Reviewed-By: Sandeep Agarwal <saagarwa@microsoft.com>
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request May 25, 2017
@kfarnung
chakrashim: fix inspector asserts
cf04607 
* Added an error that's returned when debugger can't be enabled
* Added validation for the JsGetScripts return value
* Switched to CHAKRA_VERIFY(_NOERROR) macros for all asserted calls
* Fixed uninitialized variables
* Fixed line lengths
* Switched to CHAKRA_UNIMPLEMENTED for unimplemented methods

PR-URL: nodejs#261
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Reviewed-By: Sandeep Agarwal <saagarwa@microsoft.com>
@kfarnung
chakrashim: fix inspector asserts
184b3dd 
* Added an error that's returned when debugger can't be enabled
* Added validation for the JsGetScripts return value
* Switched to CHAKRA_VERIFY(_NOERROR) macros for all asserted calls
* Fixed uninitialized variables
* Fixed line lengths
* Switched to CHAKRA_UNIMPLEMENTED for unimplemented methods

PR-URL: nodejs#261
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Reviewed-By: Sandeep Agarwal <saagarwa@microsoft.com>
@kfarnung kfarnung merged commit 184b3dd into nodejs:xplat May 26, 2017
@kfarnung kfarnung deleted the inspectfix branch May 26, 2017 16:26
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request May 31, 2017
@kfarnung
chakrashim: fix inspector asserts
074d872 
* Added an error that's returned when debugger can't be enabled
* Added validation for the JsGetScripts return value
* Switched to CHAKRA_VERIFY(_NOERROR) macros for all asserted calls
* Fixed uninitialized variables
* Fixed line lengths
* Switched to CHAKRA_UNIMPLEMENTED for unimplemented methods

PR-URL: nodejs#261
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Reviewed-By: Sandeep Agarwal <saagarwa@microsoft.com>
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request May 31, 2017
@kfarnung
chakrashim: fix inspector asserts
73b6130 
* Added an error that's returned when debugger can't be enabled
* Added validation for the JsGetScripts return value
* Switched to CHAKRA_VERIFY(_NOERROR) macros for all asserted calls
* Fixed uninitialized variables
* Fixed line lengths
* Switched to CHAKRA_UNIMPLEMENTED for unimplemented methods

PR-URL: nodejs#261
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Reviewed-By: Sandeep Agarwal <saagarwa@microsoft.com>
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request May 31, 2017
@kfarnung
chakrashim: fix inspector asserts
3a42ae3 
* Added an error that's returned when debugger can't be enabled
* Added validation for the JsGetScripts return value
* Switched to CHAKRA_VERIFY(_NOERROR) macros for all asserted calls
* Fixed uninitialized variables
* Fixed line lengths
* Switched to CHAKRA_UNIMPLEMENTED for unimplemented methods

PR-URL: nodejs#261
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Reviewed-By: Sandeep Agarwal <saagarwa@microsoft.com>
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request Jun 5, 2017
@kfarnung
chakrashim: fix inspector asserts
54d80bb 
* Added an error that's returned when debugger can't be enabled
* Added validation for the JsGetScripts return value
* Switched to CHAKRA_VERIFY(_NOERROR) macros for all asserted calls
* Fixed uninitialized variables
* Fixed line lengths
* Switched to CHAKRA_UNIMPLEMENTED for unimplemented methods

PR-URL: nodejs#261
Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
Reviewed-By: Sandeep Agarwal <saagarwa@microsoft.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@agarwal-sandeep agarwal-sandeep agarwal-sandeep left review comments

@digitalinfinity digitalinfinity digitalinfinity approved these changes

@kunalspathak kunalspathak Awaiting requested review from kunalspathak

Assignees

@kfarnung kfarnung

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@kfarnung @agarwal-sandeep @digitalinfinity @kunalspathak