Skip to content

Commit bb6d734

Browse files
tcaretcare
committed
[MERGE chakra-core#3522 @tcare] Fix chakra-core#2983 WeakMap + HostObject === Sadness (TypeError when using HostObject with WeakMap)
Merge pull request chakra-core#3522 from tcare:weakhost Our implementation of WeakMap and WeakSet uses internal properties on DynamicObjects to store the <WeakMap pointer, value> pair data. This allows us to follow the lifetime of the object. Unfortunately, HostDispatch objects such as window.location are not DynamicObjects, and so we throw a type error. We did not expect widespread use of HostDispatch objects as keys, so this was left as a TODO. Since Ember is now encountering this with window.location, we need to support this scenario. This change allows HostDispatch objects to be used as a WeakMap key. HostDispatch has a matching change to support this WeakMap data. No other RecyclableObjects are supported, and we assert as such when trying to access the object key map data from within it.
2 parents 78e7b10 + 9a4a397 commit bb6d734

4 files changed

Lines changed: 44 additions & 41 deletions

File tree

‎lib/Runtime/Library/JavascriptWeakMap.cpp‎

Lines changed: 23 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -24,9 +24,12 @@ namespace Js
2424
return static_cast<JavascriptWeakMap *>(RecyclableObject::FromVar(aValue));
2525
}
2626

27-
JavascriptWeakMap::WeakMapKeyMap* JavascriptWeakMap::GetWeakMapKeyMapFromKey(DynamicObject* key) const
27+
JavascriptWeakMap::WeakMapKeyMap* JavascriptWeakMap::GetWeakMapKeyMapFromKey(RecyclableObject* key) const
2828
{
29+
AssertOrFailFast(DynamicType::Is(key->GetTypeId()) || JavascriptOperators::GetTypeId(key) == TypeIds_HostDispatch);
30+
2931
Var weakMapKeyData = nullptr;
32+
3033
if (!key->GetInternalProperty(key, InternalPropertyIds::WeakMapKeyMap, &weakMapKeyData, nullptr, key->GetScriptContext()))
3134
{
3235
return nullptr;
@@ -42,8 +45,10 @@ namespace Js
4245
return static_cast<WeakMapKeyMap*>(weakMapKeyData);
4346
}
4447

45-
JavascriptWeakMap::WeakMapKeyMap* JavascriptWeakMap::AddWeakMapKeyMapToKey(DynamicObject* key)
48+
JavascriptWeakMap::WeakMapKeyMap* JavascriptWeakMap::AddWeakMapKeyMapToKey(RecyclableObject* key)
4649
{
50+
AssertOrFailFast(DynamicType::Is(key->GetTypeId()) || JavascriptOperators::GetTypeId(key) == TypeIds_HostDispatch);
51+
4752
// The internal property may exist on an object that has had DynamicObject::ResetObject called on itself.
4853
// In that case the value stored in the property slot should be null.
4954
DebugOnly(Var unused = nullptr);
@@ -163,9 +168,9 @@ namespace Js
163168
Var key = (args.Info.Count > 1) ? args[1] : scriptContext->GetLibrary()->GetUndefined();
164169
bool didDelete = false;
165170

166-
if (JavascriptOperators::IsObject(key) && JavascriptOperators::GetTypeId(key) != TypeIds_HostDispatch)
171+
if (JavascriptOperators::IsObject(key))
167172
{
168-
DynamicObject* keyObj = DynamicObject::FromVar(key);
173+
RecyclableObject* keyObj = RecyclableObject::FromVar(key);
169174

170175
didDelete = weakMap->Delete(keyObj);
171176
}
@@ -205,9 +210,9 @@ namespace Js
205210

206211
bool loaded = false;
207212
Var value = nullptr;
208-
if (JavascriptOperators::IsObject(key) && JavascriptOperators::GetTypeId(key) != TypeIds_HostDispatch)
213+
if (JavascriptOperators::IsObject(key))
209214
{
210-
DynamicObject* keyObj = DynamicObject::FromVar(key);
215+
RecyclableObject* keyObj = RecyclableObject::FromVar(key);
211216
loaded = weakMap->Get(keyObj, &value);
212217
}
213218

@@ -245,9 +250,9 @@ namespace Js
245250
Var key = (args.Info.Count > 1) ? args[1] : scriptContext->GetLibrary()->GetUndefined();
246251
bool hasValue = false;
247252

248-
if (JavascriptOperators::IsObject(key) && JavascriptOperators::GetTypeId(key) != TypeIds_HostDispatch)
253+
if (JavascriptOperators::IsObject(key))
249254
{
250-
DynamicObject* keyObj = DynamicObject::FromVar(key);
255+
RecyclableObject* keyObj = RecyclableObject::FromVar(key);
251256

252257
hasValue = weakMap->Has(keyObj);
253258
}
@@ -286,14 +291,12 @@ namespace Js
286291
Var key = (args.Info.Count > 1) ? args[1] : scriptContext->GetLibrary()->GetUndefined();
287292
Var value = (args.Info.Count > 2) ? args[2] : scriptContext->GetLibrary()->GetUndefined();
288293

289-
if (!JavascriptOperators::IsObject(key) || JavascriptOperators::GetTypeId(key) == TypeIds_HostDispatch)
294+
if (!JavascriptOperators::IsObject(key))
290295
{
291-
// HostDispatch can not expand so can't have internal property added to it.
292-
// TODO: Support HostDispatch as WeakMap key
293296
JavascriptError::ThrowTypeError(scriptContext, JSERR_WeakMapSetKeyNotAnObject, _u("WeakMap.prototype.set"));
294297
}
295298

296-
DynamicObject* keyObj = DynamicObject::FromVar(key);
299+
RecyclableObject* keyObj = RecyclableObject::FromVar(key);
297300

298301
#if ENABLE_TTD
299302
//In replay we need to pin the object (and will release at snapshot points) -- in record we don't need to do anything
@@ -310,7 +313,7 @@ namespace Js
310313

311314
void JavascriptWeakMap::Clear()
312315
{
313-
keySet.Map([&](DynamicObject* key, bool value, const RecyclerWeakReference<DynamicObject>* weakRef) {
316+
keySet.Map([&](RecyclableObject* key, bool value, const RecyclerWeakReference<RecyclableObject>* weakRef) {
314317
WeakMapKeyMap* keyMap = GetWeakMapKeyMapFromKey(key);
315318

316319
// It may be the case that a CEO has been reset and the keyMap is now null.
@@ -326,7 +329,7 @@ namespace Js
326329
keySet.Clear();
327330
}
328331

329-
bool JavascriptWeakMap::Delete(DynamicObject* key)
332+
bool JavascriptWeakMap::Delete(RecyclableObject* key)
330333
{
331334
WeakMapKeyMap* keyMap = GetWeakMapKeyMapFromKey(key);
332335

@@ -343,7 +346,7 @@ namespace Js
343346
return false;
344347
}
345348

346-
bool JavascriptWeakMap::Get(DynamicObject* key, Var* value) const
349+
bool JavascriptWeakMap::Get(RecyclableObject* key, Var* value) const
347350
{
348351
WeakMapKeyMap* keyMap = GetWeakMapKeyMapFromKey(key);
349352

@@ -355,7 +358,7 @@ namespace Js
355358
return false;
356359
}
357360

358-
bool JavascriptWeakMap::Has(DynamicObject* key) const
361+
bool JavascriptWeakMap::Has(RecyclableObject* key) const
359362
{
360363
WeakMapKeyMap* keyMap = GetWeakMapKeyMapFromKey(key);
361364

@@ -367,7 +370,7 @@ namespace Js
367370
return false;
368371
}
369372

370-
void JavascriptWeakMap::Set(DynamicObject* key, Var value)
373+
void JavascriptWeakMap::Set(RecyclableObject* key, Var value)
371374
{
372375
WeakMapKeyMap* keyMap = GetWeakMapKeyMapFromKey(key);
373376

@@ -393,14 +396,14 @@ namespace Js
393396
Js::ScriptContext* scriptContext = this->GetScriptContext();
394397
if(scriptContext->IsTTDReplayModeEnabled())
395398
{
396-
this->Map([&](DynamicObject* key, Js::Var value)
399+
this->Map([&](RecyclableObject* key, Js::Var value)
397400
{
398401
scriptContext->TTDContextInfo->TTDWeakReferencePinSet->AddNew(key);
399402
});
400403
}
401404

402405
//Keys are weak so are always reachable from somewhere else but values are not so we must walk them
403-
this->Map([&](DynamicObject* key, Js::Var value)
406+
this->Map([&](RecyclableObject* key, Js::Var value)
404407
{
405408
extractor->MarkVisitVar(value);
406409
});
@@ -419,7 +422,7 @@ namespace Js
419422
smi->MapSize = 0;
420423
smi->MapKeyValueArray = alloc.SlabReserveArraySpace<TTD::TTDVar>(mapCountEst + 1); //always reserve at least 1 element
421424

422-
this->Map([&](DynamicObject* key, Js::Var value)
425+
this->Map([&](RecyclableObject* key, Js::Var value)
423426
{
424427
AssertMsg(smi->MapSize + 1 < mapCountEst, "We are writting junk");
425428

‎lib/Runtime/Library/JavascriptWeakMap.h‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -42,12 +42,12 @@ namespace Js
4242
// its type and therefore without invalidating cache and JIT assumptions.
4343
//
4444
typedef JsUtil::BaseDictionary<WeakMapId, Var, Recycler, PowerOf2SizePolicy, RecyclerPointerComparer> WeakMapKeyMap;
45-
typedef JsUtil::WeaklyReferencedKeyDictionary<DynamicObject, bool, RecyclerPointerComparer<const DynamicObject*>> KeySet;
45+
typedef JsUtil::WeaklyReferencedKeyDictionary<RecyclableObject, bool, RecyclerPointerComparer<const RecyclableObject*>> KeySet;
4646

4747
Field(KeySet) keySet;
4848

49-
WeakMapKeyMap* GetWeakMapKeyMapFromKey(DynamicObject* key) const;
50-
WeakMapKeyMap* AddWeakMapKeyMapToKey(DynamicObject* key);
49+
WeakMapKeyMap* GetWeakMapKeyMapFromKey(RecyclableObject* key) const;
50+
WeakMapKeyMap* AddWeakMapKeyMapToKey(RecyclableObject* key);
5151

5252
WeakMapId GetWeakMapId() const { return (void*)(((uintptr_t)this) | 1); }
5353
static JavascriptWeakMap* GetWeakMapFromId(WeakMapId id) { return reinterpret_cast<JavascriptWeakMap*>((uintptr_t)id & (~1)); }
@@ -64,10 +64,10 @@ namespace Js
6464
static JavascriptWeakMap* FromVar(Var aValue);
6565

6666
void Clear();
67-
bool Delete(DynamicObject* key);
68-
bool Get(DynamicObject* key, Var* value) const;
69-
bool Has(DynamicObject* key) const;
70-
void Set(DynamicObject* key, Var value);
67+
bool Delete(RecyclableObject* key);
68+
bool Get(RecyclableObject* key, Var* value) const;
69+
bool Has(RecyclableObject* key) const;
70+
void Set(RecyclableObject* key, Var value);
7171

7272
virtual void Finalize(bool isShutdown) override { Clear(); }
7373
virtual void Dispose(bool isShutdown) override { }
@@ -96,7 +96,7 @@ namespace Js
9696
template <typename Fn>
9797
void Map(Fn fn)
9898
{
99-
return keySet.Map([&](DynamicObject* key, bool, const RecyclerWeakReference<DynamicObject>*)
99+
return keySet.Map([&](RecyclableObject* key, bool, const RecyclerWeakReference<RecyclableObject>*)
100100
{
101101
Var value = nullptr;
102102
WeakMapKeyMap* keyMap = GetWeakMapKeyMapFromKey(key);

‎lib/Runtime/Library/JavascriptWeakSet.cpp‎

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -105,7 +105,7 @@ namespace Js
105105
JavascriptError::ThrowTypeError(scriptContext, JSERR_WeakMapSetKeyNotAnObject, _u("WeakSet.prototype.add"));
106106
}
107107

108-
DynamicObject* keyObj = DynamicObject::FromVar(key);
108+
RecyclableObject* keyObj = RecyclableObject::FromVar(key);
109109

110110
#if ENABLE_TTD
111111
//In replay we need to pin the object (and will release at snapshot points) -- in record we don't need to do anything
@@ -139,7 +139,7 @@ namespace Js
139139

140140
if (JavascriptOperators::IsObject(key) && JavascriptOperators::GetTypeId(key) != TypeIds_HostDispatch)
141141
{
142-
DynamicObject* keyObj = DynamicObject::FromVar(key);
142+
RecyclableObject* keyObj = RecyclableObject::FromVar(key);
143143

144144
didDelete = weakSet->Delete(keyObj);
145145
}
@@ -180,7 +180,7 @@ namespace Js
180180

181181
if (JavascriptOperators::IsObject(key) && JavascriptOperators::GetTypeId(key) != TypeIds_HostDispatch)
182182
{
183-
DynamicObject* keyObj = DynamicObject::FromVar(key);
183+
RecyclableObject* keyObj = RecyclableObject::FromVar(key);
184184

185185
hasValue = weakSet->Has(keyObj);
186186
}
@@ -202,18 +202,18 @@ namespace Js
202202
return scriptContext->GetLibrary()->CreateBoolean(hasValue);
203203
}
204204

205-
void JavascriptWeakSet::Add(DynamicObject* key)
205+
void JavascriptWeakSet::Add(RecyclableObject* key)
206206
{
207207
keySet.Item(key, true);
208208
}
209209

210-
bool JavascriptWeakSet::Delete(DynamicObject* key)
210+
bool JavascriptWeakSet::Delete(RecyclableObject* key)
211211
{
212212
bool unused = false;
213213
return keySet.TryGetValueAndRemove(key, &unused);
214214
}
215215

216-
bool JavascriptWeakSet::Has(DynamicObject* key)
216+
bool JavascriptWeakSet::Has(RecyclableObject* key)
217217
{
218218
bool unused = false;
219219
return keySet.TryGetValue(key, &unused);
@@ -232,7 +232,7 @@ namespace Js
232232
Js::ScriptContext* scriptContext = this->GetScriptContext();
233233
if(scriptContext->IsTTDReplayModeEnabled())
234234
{
235-
this->Map([&](DynamicObject* key)
235+
this->Map([&](RecyclableObject* key)
236236
{
237237
scriptContext->TTDContextInfo->TTDWeakReferencePinSet->AddNew(key);
238238
});
@@ -252,7 +252,7 @@ namespace Js
252252
ssi->SetSize = 0;
253253
ssi->SetValueArray = alloc.SlabReserveArraySpace<TTD::TTDVar>(setCountEst + 1); //always reserve at least 1 element
254254

255-
this->Map([&](DynamicObject* key)
255+
this->Map([&](RecyclableObject* key)
256256
{
257257
AssertMsg(ssi->SetSize < setCountEst, "We are writting junk");
258258

‎lib/Runtime/Library/JavascriptWeakSet.h‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,7 @@ namespace Js
99
class JavascriptWeakSet : public DynamicObject
1010
{
1111
private:
12-
typedef JsUtil::WeaklyReferencedKeyDictionary<DynamicObject, bool, RecyclerPointerComparer<const DynamicObject*>> KeySet;
12+
typedef JsUtil::WeaklyReferencedKeyDictionary<RecyclableObject, bool, RecyclerPointerComparer<const RecyclableObject*>> KeySet;
1313

1414
Field(KeySet) keySet;
1515

@@ -22,9 +22,9 @@ namespace Js
2222
static bool Is(Var aValue);
2323
static JavascriptWeakSet* FromVar(Var aValue);
2424

25-
void Add(DynamicObject* key);
26-
bool Delete(DynamicObject* key);
27-
bool Has(DynamicObject* key);
25+
void Add(RecyclableObject* key);
26+
bool Delete(RecyclableObject* key);
27+
bool Has(RecyclableObject* key);
2828

2929
virtual BOOL GetDiagTypeString(StringBuilder<ArenaAllocator>* stringBuilder, ScriptContext* requestContext) override;
3030

@@ -48,7 +48,7 @@ namespace Js
4848
template <typename Fn>
4949
void Map(Fn fn)
5050
{
51-
return keySet.Map([&](DynamicObject* key, bool, const RecyclerWeakReference<DynamicObject>*)
51+
return keySet.Map([&](RecyclableObject* key, bool, const RecyclerWeakReference<RecyclableObject>*)
5252
{
5353
fn(key);
5454
});

0 commit comments

Comments
 (0)