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

Commit

Permalink
Merge pull request #64 from Microsoft/CodeCleanup
Browse files Browse the repository at this point in the history 
chakrashim: Code cleanup

Reviewed most of the CHAKRA-TODO for error handling and updated the code
accordingly. Removed these comments from the code. 
Minor clean up to undo the @eb3d8ba now Chakra propagates the OOM errors.
Converted keepalive property name to symbols.
  • Loading branch information
@kunalspathak
kunalspathak committed Aug 11, 2015
2 parents da8a753 + 028fa17 commit 5a54c00
Show file tree
Hide file tree
Showing 10 changed files with 36 additions and 114 deletions.
3 changes: 0 additions & 3 deletions deps/chakrashim/include/v8.h
View file
Expand Up @@ -1448,9 +1448,6 @@ class EXPORT Isolate {
static Isolate* New();
static Isolate* GetCurrent();

typedef bool (*abort_on_uncaught_exception_t)();
void SetAbortOnUncaughtException(abort_on_uncaught_exception_t callback);

void Enter();
void Exit();
void Dispose();
Expand Down
2 changes: 1 addition & 1 deletion deps/chakrashim/src/jsrtcachedpropertyidref.inc
View file
Expand Up @@ -42,7 +42,6 @@ DEF(ownKeys)
DEF(preventExtensions)
DEF(set)
DEF(setPrototypeOf)
DEF(__keepalive__)
DEF(length)
DEF(writable)
DEF(configurable)
Expand Down Expand Up @@ -77,6 +76,7 @@ DEFSYMBOL(self)
DEFSYMBOL(__external__)
DEFSYMBOL(__hiddenvalues__)
DEFSYMBOL(__isexternal__)
DEFSYMBOL(__keepalive__)

#undef DEF
#undef DEFSYMBOL
30 changes: 15 additions & 15 deletions deps/chakrashim/src/jsrtisolateshim.cc
View file
Expand Up @@ -113,6 +113,8 @@ bool IsolateShim::Dispose() {
// Set the current IsolateShim scope
v8::Isolate::Scope scope(ToIsolate(this));
if (JsDisposeRuntime(runtime) != JsNoError) {
// Can't do much at this point. Assert that this doesn't happen in debug
CHAKRA_ASSERT(false);
return false;
}
}
Expand All @@ -135,8 +137,8 @@ bool IsolateShim::IsDisposing() {
return isDisposing;
}

//TODO: Chakra: This is not called after cross context work in chakra. Fix this else we will
// leak chakrashim object.
// CHAKRA-TODO: This is not called after cross context work in chakra. Fix this
// else we will leak chakrashim object.
void CALLBACK IsolateShim::JsContextBeforeCollectCallback(
_In_ JsRef contextRef, _In_opt_ void *data) {
IsolateShim * isolateShim = reinterpret_cast<IsolateShim *>(data);
Expand Down Expand Up @@ -185,7 +187,6 @@ void IsolateShim::DisposeAll() {
IsolateShim * curr = s_isolateList;
s_isolateList = nullptr;
while (curr) {
// CHAKRA-TODO: Handle error?
curr->Dispose();
curr = curr->next;
}
Expand All @@ -202,10 +203,10 @@ void IsolateShim::PushScope(
scope->previous = this->contextScopeStack;
this->contextScopeStack = scope;

// CHAKRA-TODO: Error handling?
JsSetCurrentContext(contextShim->GetContextRef());
// Don't crash even if we fail to set the context
JsErrorCode errorCode = JsSetCurrentContext(contextShim->GetContextRef());
CHAKRA_ASSERT(errorCode == JsNoError);

// CHAKRA-TODO: Error handling?
if (!contextShim->EnsureInitialized()) {
Fatal("Failed to initialize context");
}
Expand All @@ -215,7 +216,6 @@ void IsolateShim::PopScope(ContextShim::Scope * scope) {
assert(this->contextScopeStack == scope);
ContextShim::Scope * prevScope = scope->previous;
if (prevScope != nullptr) {
// Marshal the pending exception
JsValueRef exception = JS_INVALID_REFERENCE;
bool hasException;
if (scope->contextShim != prevScope->contextShim &&
Expand All @@ -224,9 +224,12 @@ void IsolateShim::PopScope(ContextShim::Scope * scope) {
JsGetAndClearException(&exception) == JsNoError) {
}

JsSetCurrentContext(prevScope->contextShim->GetContextRef());
// Don't crash even if we fail to set the context
JsErrorCode errorCode = JsSetCurrentContext(
prevScope->contextShim->GetContextRef());
CHAKRA_ASSERT(errorCode == JsNoError);

// CHAKRA-TODO: Error handling?
// Propagate the exception to parent scope
if (exception != JS_INVALID_REFERENCE) {
JsSetException(exception);
}
Expand All @@ -245,9 +248,7 @@ JsPropertyIdRef IsolateShim::GetSelfSymbolPropertyIdRef() {
}

JsPropertyIdRef IsolateShim::GetKeepAliveObjectSymbolPropertyIdRef() {
// CHAKRA-TODO: has a bug with symbols and proxy, just a real property name
return GetCachedPropertyIdRef(CachedPropertyIdRef::__keepalive__);
// return EnsurePrivateSymbol(&keepAliveObjectSymbolPropertyIdRef);
return GetCachedSymbolPropertyIdRef(CachedSymbolPropertyIdRef::__keepalive__);
}

template <class Index, class CreatePropertyIdFunc>
Expand Down Expand Up @@ -296,8 +297,7 @@ JsPropertyIdRef IsolateShim::GetProxyTrapPropertyIdRef(ProxyTraps trap) {

ContextShim * IsolateShim::GetContextShimOfObject(JsValueRef valueRef) {
JsContextRef contextRef;
if (JsGetContextOfObject(valueRef, &contextRef) != JsNoError)
{
if (JsGetContextOfObject(valueRef, &contextRef) != JsNoError) {
return nullptr;
}
assert(contextRef != nullptr);
Expand Down Expand Up @@ -327,7 +327,7 @@ bool IsolateShim::AddMessageListener(void * that) {
try {
messageListeners.push_back(that);
return true;
} catch (...) {
} catch(...) {
return false;
}
}
Expand Down
12 changes: 0 additions & 12 deletions deps/chakrashim/src/jsrtutils.cc
View file
Expand Up @@ -897,16 +897,4 @@ void Fatal(const char * format, ...) {

abort();
}

void SetOutOfMemoryErrorIfExist(_In_ JsErrorCode errorCode) {
if (errorCode == JsErrorOutOfMemory) {
const wchar_t txt[] = L"process out of memory";
JsValueRef msg, err;
if (JsPointerToString(txt, _countof(txt) - 1, &msg) == JsNoError &&
JsCreateError(msg, &err) == JsNoError) {
JsSetException(err);
}
}
}

} // namespace jsrt
5 changes: 0 additions & 5 deletions deps/chakrashim/src/jsrtutils.h
View file
Expand Up @@ -319,11 +319,6 @@ class JsArguments {
}
};

// Check if the errorCode was JsErrorOutOfMemory and if yes,
// create and set the exception on the context
void SetOutOfMemoryErrorIfExist(_In_ JsErrorCode errorCode);


template <bool LIKELY,
class JsConvertToValueFunc,
class JsValueToNativeFunc,
Expand Down
56 changes: 4 additions & 52 deletions deps/chakrashim/src/v8function.cc
View file
Expand Up @@ -66,60 +66,12 @@ Local<Value> Function::Call(
JsValueRef result;
{
TryCatch tryCatch;
JsErrorCode error =
JsCallFunction((JsValueRef)this, args.get(), argc + 1, &result);

jsrt::SetOutOfMemoryErrorIfExist(error);

if (error == JsNoError) {
return Local<Value>::New(static_cast<Value*>(result));
}
if (error != JsErrorInvalidArgument) {
if (JsCallFunction((JsValueRef)this, args.get(),
argc + 1, &result) != JsNoError) {
tryCatch.CheckReportExternalException();
return Local<Value>();
}
}

// Invalid argument may mean some of the object are from another context
// Check and marshal and call again.

// NOTE: Ideally, we will never run into this situation where we will
// also marshal correctly and use object from the same context.
// But CopyProperty in node_contextify.cc violate that so, we have this
// to paper over it.
IsolateShim * isolateShim = IsolateShim::GetCurrent();
ContextShim * currentContextShim = isolateShim->GetCurrentContextShim();
if (currentContextShim == nullptr) {
return Local<Value>();
}

for (int i = 0; i < argc + 1; i++) {
JsValueRef valueRef = args.get()[i];
ContextShim * objectContextShim = isolateShim->GetContextShimOfObject(valueRef);
if (currentContextShim == objectContextShim) {
continue;
}
if (objectContextShim != nullptr) {
ContextShim::Scope scope(objectContextShim);
args.get()[i] = valueRef;
} else {
// Can't find a context
return Local<Value>();
}
}

{
TryCatch tryCatch;
JsErrorCode error = JsCallFunction((JsValueRef)this,
args.get(), argc + 1, &result);

jsrt::SetOutOfMemoryErrorIfExist(error);

if (error != JsNoError) {
tryCatch.CheckReportExternalException();
return Local<Value>();
}
return Local<Value>::New(static_cast<Value*>(result));
return Local<Value>::New(static_cast<Value*>(result));
}
}

Expand All @@ -131,7 +83,7 @@ void Function::SetName(Handle<String> name) {
(JsValueRef)*name,
JS_INVALID_REFERENCE,
JS_INVALID_REFERENCE);
// CHAKRA-TODO: Check error?
CHAKRA_ASSERT(error == JsNoError);
}

Function *Function::Cast(Value *obj) {
Expand Down
6 changes: 4 additions & 2 deletions deps/chakrashim/src/v8handlescope.cc
View file
Expand Up @@ -20,6 +20,7 @@

#include "v8.h"
#include "jsrt.h"
#include "jsrtutils.h"

namespace v8 {

Expand All @@ -41,8 +42,9 @@ HandleScope::~HandleScope() {
AddRefRecord * currRecord = this->_addRefRecordHead;
while (currRecord != nullptr) {
AddRefRecord * nextRecord = currRecord->_next;
// CHAKRA-TODO: Report error?
JsRelease(currRecord->_ref, nullptr);
// Don't crash even if we fail to release the scope
JsErrorCode errorCode = JsRelease(currRecord->_ref, nullptr);
CHAKRA_ASSERT(errorCode == JsNoError);
delete currRecord;
currRecord = nextRecord;
}
Expand Down
6 changes: 0 additions & 6 deletions deps/chakrashim/src/v8isolate.cc
View file
Expand Up @@ -35,11 +35,6 @@ Isolate *Isolate::GetCurrent() {
return jsrt::IsolateShim::GetCurrentAsIsolate();
}

void Isolate::SetAbortOnUncaughtException(
abort_on_uncaught_exception_t callback) {
// CHAKRA-TODO
}

void Isolate::Enter() {
return jsrt::IsolateShim::FromIsolate(this)->Enter();
}
Expand All @@ -50,7 +45,6 @@ void Isolate::Exit() {
}

void Isolate::Dispose() {
// CHAKRA-TODO: Handle Error?
jsrt::IsolateShim::FromIsolate(this)->Dispose();
}

Expand Down
10 changes: 2 additions & 8 deletions deps/chakrashim/src/v8script.cc
View file
Expand Up @@ -91,9 +91,6 @@ Local<Script> Script::Compile(Handle<String> source, Handle<String> file_name) {
}
}
}

jsrt::SetOutOfMemoryErrorIfExist(error);

return Local<Script>();
}

Expand All @@ -104,11 +101,8 @@ Local<Value> Script::Run() {
}

JsValueRef result;
JsErrorCode errorCode = JsCallFunction(scriptFunction, nullptr, 0, &result);

jsrt::SetOutOfMemoryErrorIfExist(errorCode);

if (errorCode != JsNoError) {
if (JsCallFunction(scriptFunction, nullptr,
0, &result) != JsNoError) {
return Local<Value>();
}

Expand Down
20 changes: 10 additions & 10 deletions deps/chakrashim/src/v8trycatch.cc
View file
Expand Up @@ -50,15 +50,14 @@ bool TryCatch::HasCaught() const {
if (error != JS_INVALID_REFERENCE) {
return true;
}

bool hasException;
JsErrorCode errorCode = JsHasException(&hasException);
if (errorCode != JsNoError) {
if (errorCode == JsErrorInDisabledState) {
return true;
}
// CHAKRA-TODO: report error
assert(false);
// Should never get errorCode other than JsNoError/JsErrorInDisabledState
CHAKRA_ASSERT(false);
return false;
}

Expand All @@ -73,20 +72,21 @@ void TryCatch::GetAndClearException() {
bool hasException;
JsErrorCode errorCode = JsHasException(&hasException);
if (errorCode != JsNoError) {
// CHAKRA-TODO: report error
assert(errorCode == JsErrorInDisabledState);
// If script is in disabled state, no need to fail here.
// We will fail subsequent calls to Jsrt anyway.
CHAKRA_ASSERT(errorCode == JsErrorInDisabledState);
return;
}

if (hasException) {
JsValueRef exceptionRef;
errorCode = JsGetAndClearException(&exceptionRef);
if (errorCode != JsNoError) {
// CHAKRA-TODO: report error
assert(errorCode == JsErrorInDisabledState);
return;
// We came here through JsHasException, so script shouldn't be in disabled
// state.
CHAKRA_ASSERT(errorCode != JsErrorInDisabledState);
if (errorCode == JsNoError) {
error = exceptionRef;
}
error = exceptionRef;
}
}

Expand Down

0 comments on commit 5a54c00

Please sign in to comment.