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

Commit 90c554d

Browse files
kfarnungkfarnung
committed
chakrashim: Fix Promise::Resolver
The promisify function calls into native code to create a new Promise::Resolver object and returns it back to script. In script it's expected to be a promise object, but in our case it wasn't. The solution is to hide the resolver data in an external object and attach it to the promise object. PR-URL: #470 Reviewed-By: Jimmy Thomson <jithomso@microsoft.com> Reviewed-By: Hitesh Kanwathirtha <hiteshk@microsoft.com>
1 parent 2ef9cf9 commit 90c554d

File tree

5 files changed

+143
-140
lines changed

5 files changed

+143
-140
lines changed

‎deps/chakrashim/include/v8.h‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -703,6 +703,7 @@ class Persistent : public PersistentBase<T> {
703703
friend class Utils;
704704
template <class F> friend class Local;
705705
template <class F> friend class ReturnValue;
706+
friend class PromiseResolverData;
706707

707708
V8_INLINE Persistent(T* that)
708709
: PersistentBase<T>(PersistentBase<T>::New(nullptr, that)) { }

‎deps/chakrashim/src/jsrtutils.cc‎

Lines changed: 8 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -391,6 +391,12 @@ JsErrorCode GetPropertyNames(JsValueRef object,
391391
object, result);
392392
}
393393

394+
JsPropertyIdRef GetExternalPropertyId() {
395+
IsolateShim* iso = IsolateShim::GetCurrent();
396+
return iso->GetCachedSymbolPropertyIdRef(
397+
CachedSymbolPropertyIdRef::__external__);
398+
}
399+
394400
JsErrorCode AddExternalData(JsValueRef ref,
395401
JsPropertyIdRef externalDataPropertyId,
396402
void *data,
@@ -413,11 +419,7 @@ JsErrorCode AddExternalData(JsValueRef ref,
413419
JsErrorCode AddExternalData(JsValueRef ref,
414420
void *data,
415421
JsFinalizeCallback onObjectFinalize) {
416-
IsolateShim* iso = IsolateShim::GetCurrent();
417-
JsPropertyIdRef propId = iso->GetCachedSymbolPropertyIdRef(
418-
CachedSymbolPropertyIdRef::__external__);
419-
420-
return AddExternalData(ref, propId, data, onObjectFinalize);
422+
return AddExternalData(ref, GetExternalPropertyId(), data, onObjectFinalize);
421423
}
422424

423425
JsErrorCode GetExternalData(JsValueRef ref,
@@ -437,11 +439,7 @@ JsErrorCode GetExternalData(JsValueRef ref,
437439

438440
JsErrorCode GetExternalData(JsValueRef ref,
439441
void **data) {
440-
IsolateShim* iso = IsolateShim::GetCurrent();
441-
JsPropertyIdRef propId = iso->GetCachedSymbolPropertyIdRef(
442-
CachedSymbolPropertyIdRef::__external__);
443-
444-
return GetExternalData(ref, propId, data);
442+
return GetExternalData(ref, GetExternalPropertyId(), data);
445443
}
446444

447445
JsErrorCode CreateFunctionWithExternalData(

‎deps/chakrashim/src/jsrtutils.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -281,6 +281,8 @@ JsErrorCode CloneObject(JsValueRef source,
281281
JsErrorCode GetPropertyNames(JsValueRef object,
282282
JsValueRef *namesArray);
283283

284+
JsPropertyIdRef GetExternalPropertyId();
285+
284286
JsErrorCode AddExternalData(JsValueRef ref,
285287
JsPropertyIdRef externalDataPropertyId,
286288
void *data,

‎deps/chakrashim/src/v8chakra.h‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@ enum class ExternalDataTypes {
3636
ObjectData,
3737
FunctionTemplateData,
3838
FunctionCallbackData,
39+
PromiseResolverData,
3940
};
4041

4142
// Base class for external object data
@@ -75,6 +76,17 @@ class ExternalData {
7576
static bool TryGet(JsValueRef ref, T** data) {
7677
return GetExternalData(ref, data) == JsNoError && *data != nullptr;
7778
}
79+
80+
template <class T>
81+
static bool TryGetFromProperty(JsValueRef ref, JsPropertyIdRef propertyId,
82+
T** data) {
83+
JsValueRef propertyValue = nullptr;
84+
if (JsGetProperty(ref, propertyId, &propertyValue) != JsNoError) {
85+
return false;
86+
}
87+
88+
return T::TryGet(propertyValue, data);
89+
}
7890
};
7991

8092
struct SetterGetterInterceptor {

‎deps/chakrashim/src/v8resolver.cc‎

Lines changed: 120 additions & 130 deletions
Original file line numberDiff line numberDiff line change
@@ -21,164 +21,154 @@
2121
#include "v8chakra.h"
2222
#include "jsrtutils.h"
2323

24-
namespace v8 {
25-
using Resolver = Promise::Resolver;
26-
// CHAKRA-TODO: Unimplemented completely
27-
MaybeLocal<Resolver> Resolver::New(Local<Context> context) {
28-
JsValueRef resolver;
29-
if (JsCreateObject(&resolver) != JsNoError) {
30-
return Local<Resolver>();
31-
}
32-
33-
JsValueRef promise, resolve, reject;
34-
if (JsCreatePromise(&promise, &resolve, &reject) != JsNoError) {
35-
return Local<Resolver>();
36-
}
24+
#include <array>
3725

38-
if (jsrt::SetProperty(
39-
resolver,
40-
jsrt::CachedPropertyIdRef::promise,
41-
promise) != JsNoError) {
42-
return Local<Resolver>();
43-
}
44-
45-
if (jsrt::SetProperty(
46-
resolver,
47-
jsrt::CachedPropertyIdRef::resolve,
48-
resolve) != JsNoError) {
49-
return Local<Resolver>();
50-
}
26+
namespace v8 {
5127

52-
if (jsrt::SetProperty(
53-
resolver,
54-
jsrt::CachedPropertyIdRef::reject,
55-
reject) != JsNoError) {
56-
return Local<Resolver>();
57-
}
28+
class PromiseResolverData : public ExternalData {
29+
public:
30+
static const ExternalDataTypes ExternalDataType =
31+
ExternalDataTypes::PromiseResolverData;
32+
33+
private:
34+
Persistent<Value> resolve;
35+
Persistent<Value> reject;
36+
37+
public:
38+
PromiseResolverData(Local<Value> resolve,
39+
Local<Value> reject)
40+
: ExternalData(ExternalDataType),
41+
resolve(nullptr, resolve),
42+
reject(nullptr, reject) {}
43+
44+
~PromiseResolverData() {
45+
resolve.Reset();
46+
reject.Reset();
47+
}
5848

59-
JsValueRef state;
60-
jsrt::UintToValue(0, &state);
61-
if (jsrt::SetProperty(
62-
promise,
63-
jsrt::CachedPropertyIdRef::value,
64-
state) != JsNoError) {
65-
return Local<Resolver>();
49+
static void CHAKRA_CALLBACK FinalizeCallback(void *data) {
50+
if (data != nullptr) {
51+
PromiseResolverData* promiseResolverData =
52+
reinterpret_cast<PromiseResolverData*>(data);
53+
delete promiseResolverData;
6654
}
55+
}
6756

68-
return Local<Resolver>::New(static_cast<Resolver*>(resolver));
57+
JsErrorCode Resolve(Local<Value> value) {
58+
JsValueRef result = nullptr;
59+
std::array<JsValueRef, 2> args = { jsrt::GetUndefined(), *value };
60+
return JsCallFunction(*resolve, args.data(), args.size(), &result);
6961
}
7062

71-
Local<Resolver> Resolver::New(Isolate* isolate) {
72-
return New(isolate->GetCurrentContext()).ToLocalChecked();
63+
JsErrorCode Reject(Local<Value> value) {
64+
JsValueRef result = nullptr;
65+
std::array<JsValueRef, 2> args = { jsrt::GetUndefined(), *value };
66+
return JsCallFunction(*reject, args.data(), args.size(), &result);
7367
}
68+
};
7469

75-
Local<Promise> Resolver::GetPromise() {
76-
JsValueRef promise;
77-
if (jsrt::GetProperty(
78-
this,
79-
jsrt::CachedPropertyIdRef::promise,
80-
&promise) != JsNoError) {
81-
return Local<Promise>();
82-
}
83-
return Local<Promise>::New(static_cast<Promise*>(promise));
70+
MaybeLocal<Promise::Resolver> Promise::Resolver::New(Local<Context> context) {
71+
JsValueRef promise, resolve, reject;
72+
if (JsCreatePromise(&promise, &resolve, &reject) != JsNoError) {
73+
return Local<Promise::Resolver>();
8474
}
8575

86-
Resolver* Resolver::Cast(Value* obj) {
87-
return static_cast<Resolver*>(obj);
76+
PromiseResolverData* data = new PromiseResolverData(resolve, reject);
77+
if (jsrt::AddExternalData(
78+
promise, data, PromiseResolverData::FinalizeCallback) != JsNoError) {
79+
delete data;
80+
return Local<Promise::Resolver>();
8881
}
8982

90-
Maybe<bool> Resolver::Resolve(Local<Context> context, Local<Value> value) {
91-
JsValueRef resolve;
92-
if (jsrt::GetProperty(
93-
this,
94-
jsrt::CachedPropertyIdRef::resolve,
95-
&resolve) != JsNoError) {
96-
return Nothing<bool>();
97-
}
83+
JsValueRef state;
84+
jsrt::UintToValue(0, &state);
85+
if (jsrt::SetProperty(
86+
promise,
87+
jsrt::CachedPropertyIdRef::state,
88+
state) != JsNoError) {
89+
return Local<Promise::Resolver>();
90+
}
9891

99-
JsValueRef promise;
100-
if (jsrt::GetProperty(
101-
this,
102-
jsrt::CachedPropertyIdRef::promise,
103-
&promise) != JsNoError) {
104-
return Nothing<bool>();
105-
}
92+
return Local<Promise::Resolver>::New(promise);
93+
}
10694

107-
if (jsrt::SetProperty(
108-
promise,
109-
jsrt::CachedPropertyIdRef::value,
110-
*value) != JsNoError) {
111-
return Nothing<bool>();
112-
}
95+
Local<Promise::Resolver> Promise::Resolver::New(Isolate* isolate) {
96+
return New(isolate->GetCurrentContext()).ToLocalChecked();
97+
}
11398

114-
JsValueRef state;
115-
jsrt::UintToValue(1, &state);
116-
if (jsrt::SetProperty(
117-
promise,
118-
jsrt::CachedPropertyIdRef::value,
119-
state) != JsNoError) {
120-
return Nothing<bool>();
121-
}
99+
Local<Promise> Promise::Resolver::GetPromise() {
100+
return Local<Promise>::New(static_cast<JsValueRef>(this));
101+
}
122102

123-
JsValueRef result;
124-
JsValueRef args[2];
125-
args[0] = this; // ? What is the "this" of the resolver here?
126-
args[1] = reinterpret_cast<JsValueRef>(*value);
103+
Promise::Resolver* Promise::Resolver::Cast(Value* obj) {
104+
return static_cast<Promise::Resolver*>(obj);
105+
}
127106

128-
if (JsCallFunction(resolve, args, 2, &result) != JsNoError) {
129-
return Nothing<bool>();
130-
}
107+
Maybe<bool> Promise::Resolver::Resolve(Local<Context> context,
108+
Local<Value> value) {
109+
PromiseResolverData* data = nullptr;
110+
if (!ExternalData::TryGetFromProperty(this, jsrt::GetExternalPropertyId(),
111+
&data)) {
112+
return Nothing<bool>();
113+
}
131114

132-
return Just(true);
115+
if (jsrt::SetProperty(
116+
this,
117+
jsrt::CachedPropertyIdRef::value,
118+
*value) != JsNoError) {
119+
return Nothing<bool>();
133120
}
134121

135-
void Resolver::Resolve(Local<Value> value) {
136-
Local<Context> context;
137-
Resolve(context, value);
122+
JsValueRef state;
123+
jsrt::UintToValue(1, &state);
124+
if (jsrt::SetProperty(
125+
this,
126+
jsrt::CachedPropertyIdRef::state,
127+
state) != JsNoError) {
128+
return Nothing<bool>();
138129
}
139130

140-
Maybe<bool> Resolver::Reject(Local<Context> context, Local<Value> value) {
141-
JsValueRef reject;
142-
if (jsrt::GetProperty(
143-
this,
144-
jsrt::CachedPropertyIdRef::reject,
145-
&reject) != JsNoError) {
146-
return Nothing<bool>();
147-
}
131+
if (data->Resolve(value) != JsNoError) {
132+
return Nothing<bool>();
133+
}
148134

149-
JsValueRef promise;
150-
if (jsrt::GetProperty(
151-
this,
152-
jsrt::CachedPropertyIdRef::promise,
153-
&promise) != JsNoError) {
154-
return Nothing<bool>();
155-
}
135+
return Just(true);
136+
}
156137

157-
if (jsrt::SetProperty(
158-
promise,
159-
jsrt::CachedPropertyIdRef::value,
160-
*value) != JsNoError) {
161-
return Nothing<bool>();
162-
}
138+
void Promise::Resolver::Resolve(Local<Value> value) {
139+
Local<Context> context;
140+
Resolve(context, value);
141+
}
163142

164-
JsValueRef state;
165-
jsrt::UintToValue(2, &state);
166-
if (jsrt::SetProperty(
167-
promise,
168-
jsrt::CachedPropertyIdRef::value,
169-
state) != JsNoError) {
170-
return Nothing<bool>();
171-
}
143+
Maybe<bool> Promise::Resolver::Reject(Local<Context> context,
144+
Local<Value> value) {
145+
PromiseResolverData* data = nullptr;
146+
if (!ExternalData::TryGetFromProperty(this, jsrt::GetExternalPropertyId(),
147+
&data)) {
148+
return Nothing<bool>();
149+
}
172150

173-
JsValueRef result;
174-
JsValueRef args[2];
175-
args[0] = this; // ? What is the "this" of the resolver here?
176-
args[1] = reinterpret_cast<JsValueRef>(*value);
151+
if (jsrt::SetProperty(
152+
this,
153+
jsrt::CachedPropertyIdRef::value,
154+
*value) != JsNoError) {
155+
return Nothing<bool>();
156+
}
177157

178-
if (JsCallFunction(reject, args, 2, &result) != JsNoError) {
179-
return Nothing<bool>();
180-
}
158+
JsValueRef state;
159+
jsrt::UintToValue(2, &state);
160+
if (jsrt::SetProperty(
161+
this,
162+
jsrt::CachedPropertyIdRef::state,
163+
state) != JsNoError) {
164+
return Nothing<bool>();
165+
}
181166

182-
return Just(true);
167+
if (data->Reject(value) != JsNoError) {
168+
return Nothing<bool>();
183169
}
170+
171+
return Just(true);
172+
}
173+
184174
} // namespace v8

0 commit comments

Comments
 (0)