Correctly annotate v8::StackTrace and v8::StackFrame API methods

src/api.cc

 // Copyright 2012 the V8 project authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "src/api.h"
 
 #include <string.h>  // For memcpy, strlen.
 #ifdef V8_USE_ADDRESS_SANITIZER
 #include <sanitizer/asan_interface.h>
 #endif  // V8_USE_ADDRESS_SANITIZER
@@ skipping 2823 matching lines @@
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
   ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate);
   i_isolate->PrintCurrentStackTrace(out);
 }
 
 
 // --- S t a c k T r a c e ---
 
 Local<StackFrame> StackTrace::GetFrame(uint32_t index) const {
   i::Isolate* isolate = Utils::OpenHandle(this)->GetIsolate();
-  ENTER_V8(isolate);
+  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
   EscapableHandleScope scope(reinterpret_cast<Isolate*>(isolate));
   auto self = Utils::OpenHandle(this);
   auto obj = i::JSReceiver::GetElement(isolate, self, index).ToHandleChecked();
   auto jsobj = i::Handle<i::JSObject>::cast(obj);
   return scope.Escape(Utils::StackFrameToLocal(jsobj));
 }
 
 
 int StackTrace::GetFrameCount() const {
   return i::Smi::cast(Utils::OpenHandle(this)->length())->value();
 }
 
 
 Local<Array> StackTrace::AsArray() {
   return Utils::ToLocal(Utils::OpenHandle(this));
 }
 
 
 Local<StackTrace> StackTrace::CurrentStackTrace(
     Isolate* isolate,
     int frame_limit,
     StackTraceOptions options) {
   i::Isolate* i_isolate = reinterpret_cast<i::Isolate*>(isolate);
-  ENTER_V8(i_isolate);
-  // TODO(dcarney): remove when ScriptDebugServer is fixed.

[mmoroz, 2017/03/21 13:12:58]

Looks like dcarney@ is not at Google anymore. Should we go through other his
TODO / FIXME comments? There are ~35 of them.

[jochen (gone - plz use gerrit), 2017/03/22 09:59:06]

yeah, eventually we should do that... however, we'll need to address them /
assign to a new owner. Just deleting is no good :/

[mmoroz, 2017/03/22 11:55:43]

Yeah, I didn't mean to simply delete others. Sorry, I should have been more
clear. I can try to go through TODOs and address those ones that I can resolve +
reassign all the rest.

[jochen (gone - plz use gerrit), 2017/03/22 12:45:33]

great! I'd do that in separate CLs, however.

in this instance, you can probably move kExposeFramesAcrossSecurityOrigins to
https://cs.chromium.org/chromium/src/v8/src/inspector/v8-stack-trace-impl.cc?...

[mmoroz, 2017/03/22 13:34:17]

Done.

+  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(i_isolate);
   options = static_cast<StackTraceOptions>(
       static_cast<int>(options) | kExposeFramesAcrossSecurityOrigins);

[mmoroz, 2017/03/21 13:12:58]

Why do we need to expose frames across security origins? I've tried to remove
this, and lots of tests failed after that. So it feels that this is important
flag, but I don't understand the logic behind it. Why do we want to expose
cross-origin stack frames?

[jochen (gone - plz use gerrit), 2017/03/22 09:59:06]

when we expose a stack to javascript, e.g. via new Error().stack, we can't
expose cross origin frames. The debugger, however, will want to expose such
stacks via devtools.

[mmoroz, 2017/03/22 11:55:43]

Acknowledged.

   i::Handle<i::JSArray> stackTrace =
       i_isolate->CaptureCurrentStackTrace(frame_limit, options);
   return Utils::StackTraceToLocal(stackTrace);
 }
 
 
 // --- S t a c k F r a m e ---
 
 static int getIntProperty(const StackFrame* f, const char* propertyName,
                           int defaultValue) {
   i::Isolate* isolate = Utils::OpenHandle(f)->GetIsolate();
-  ENTER_V8(isolate);
+  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
   i::HandleScope scope(isolate);
   i::Handle<i::JSObject> self = Utils::OpenHandle(f);
   i::Handle<i::Object> obj =
       i::JSReceiver::GetProperty(isolate, self, propertyName).ToHandleChecked();

[mmoroz, 2017/03/21 13:12:58]

None of the tests failed, but I cannot understand whether GetProperty() can
execute script or not?

   return obj->IsSmi() ? i::Smi::cast(*obj)->value() : defaultValue;
 }
 
 
 int StackFrame::GetLineNumber() const {
   return getIntProperty(this, "lineNumber", Message::kNoLineNumberInfo);
 }
 
 
 int StackFrame::GetColumn() const {
   return getIntProperty(this, "column", Message::kNoColumnInfo);
 }
 
 
 int StackFrame::GetScriptId() const {
   return getIntProperty(this, "scriptId", Message::kNoScriptIdInfo);
 }
 
 
 static Local<String> getStringProperty(const StackFrame* f,
                                        const char* propertyName) {
   i::Isolate* isolate = Utils::OpenHandle(f)->GetIsolate();
-  ENTER_V8(isolate);
+  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
   EscapableHandleScope scope(reinterpret_cast<Isolate*>(isolate));
   i::Handle<i::JSObject> self = Utils::OpenHandle(f);
   i::Handle<i::Object> obj =
       i::JSReceiver::GetProperty(isolate, self, propertyName).ToHandleChecked();
   return obj->IsString()
              ? scope.Escape(Local<String>::Cast(Utils::ToLocal(obj)))
              : Local<String>();
 }
 
 
 Local<String> StackFrame::GetScriptName() const {
   return getStringProperty(this, "scriptName");
 }
 
 
 Local<String> StackFrame::GetScriptNameOrSourceURL() const {
   return getStringProperty(this, "scriptNameOrSourceURL");
 }
 
 
 Local<String> StackFrame::GetFunctionName() const {
   return getStringProperty(this, "functionName");
 }
 
 
 static bool getBoolProperty(const StackFrame* f, const char* propertyName) {
   i::Isolate* isolate = Utils::OpenHandle(f)->GetIsolate();
-  ENTER_V8(isolate);
+  ENTER_V8_NO_SCRIPT_NO_EXCEPTION(isolate);
   i::HandleScope scope(isolate);
   i::Handle<i::JSObject> self = Utils::OpenHandle(f);
   i::Handle<i::Object> obj =
       i::JSReceiver::GetProperty(isolate, self, propertyName).ToHandleChecked();
   return obj->IsTrue(isolate);

[mmoroz, 2017/03/21 13:12:58]

This concerns me a bit. Can "IsTrue" evaluate the value, i.e. execute some
script?

 }
 
 bool StackFrame::IsEval() const { return getBoolProperty(this, "isEval"); }
 
 
 bool StackFrame::IsConstructor() const {
   return getBoolProperty(this, "isConstructor");
 }
 
 
@@ skipping 7336 matching lines @@
   Address callback_address =
       reinterpret_cast<Address>(reinterpret_cast<intptr_t>(callback));
   VMState<EXTERNAL> state(isolate);
   ExternalCallbackScope call_scope(isolate, callback_address);
   callback(info);
 }
 
 
 }  // namespace internal
 }  // namespace v8