Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Array.copyWithin should throw when invoked on TypedArray with missing slots #4790

Merged
merged 1 commit into from
Mar 9, 2018
Merged

Array.copyWithin should throw when invoked on TypedArray with missing slots #4790

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 1 addition & 5 deletions lib/Runtime/Language/JavascriptOperators.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4818,11 +4818,7 @@ namespace Js
#endif
}

if (result == FALSE && (propertyOperationFlags & PropertyOperation_StrictMode))
Copy link
Contributor

@MSLaguana MSLaguana Mar 9, 2018 •
edited by IrinaYatsenko
Loading

Choose a reason for hiding this comment

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

To try and catch any errors in future, it might be worth to add an assert here along the lines of Assert(result || !(propertyOperationFlags & PropertyOperation_StrictMode); that way there is no overhead in release builds, but we should catch regressions in testing. #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

maybe result || !(propertyOperationFlags & (PropertyOperation_StrictMode | PropertyOperation_ThrowOnDeleteOrNotConfig)), since the point of this change is updating the contract elsewhere to respect both flags

In reply to: 173516816 [](ancestors = 173516816)

{
JavascriptError::ThrowCantDelete(propertyOperationFlags, scriptContext, GetPropertyDisplayNameForError(index, scriptContext)->GetString());
}

Assert(result || !(propertyOperationFlags & (PropertyOperation_StrictMode | PropertyOperation_ThrowOnDeleteIfNotConfig)));
return scriptContext->GetLibrary()->CreateBoolean(result);
}

Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Library/JavascriptError.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -694,7 +694,7 @@ namespace Js
return false;
}

bool JavascriptError::ThrowCantDelete(PropertyOperationFlags flags, ScriptContext* scriptContext, PCWSTR varName)
bool JavascriptError::ThrowCantDeleteIfStrictModeOrNonconfigurable(PropertyOperationFlags flags, ScriptContext* scriptContext, PCWSTR varName)
Copy link
Contributor Author

@IrinaYatsenko IrinaYatsenko Mar 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

ThrowCantDeleteIfStrictModeOrNonconfigurable [](start = 26, length = 44)

Ugly name, but similar to "ThrowCantDeleteIfStrictMode". #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

ThrowCantDeleteIfRequiredForNonConfigurableProperty? Current name sounds like it would throw if the property was nonconfigurable (regardless of the operation flags), which isn't true (and in fact this method has no way to even know if the property was configurable, which must be checked by the caller).

In reply to: 173516752 [](ancestors = 173516752)

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, on the topic of this method, I don't like how it forces the caller to do a possibly-expensive conversion to string before figuring out whether we need it. I'd rather take a Var and do GetPropertyDisplayNameForError(varName, scriptContext)->GetString() only once we determine that we actually need to throw. Throwing is super expensive anyway, so any additional conversion cost there doesn't particularly matter. Anyhow, this wasn't introduced by your change, so feel free to leave as-is.

In reply to: 173520340 [](ancestors = 173520340,173516752)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  1. name: it throws either in strict mode or if non-configurable. Because this information can only be extracted from the flags, I think the name is more clear now (it doesn't give the impression that it guarantees to throw anymore).
  2. string vs var: the callers seem to differ in how they get hold of the string and I don't have a good grasp of details to assess whether var would make perf better for them.

In reply to: 173521221 [](ancestors = 173521221,173520340,173516752)

Copy link
Contributor

Choose a reason for hiding this comment

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

Would be fair to have an overload of this method which takes a Var for varName and then use GetPropertyDisplayNameForError if we'll throw an error. Otherwise don't do the conversion. Could save some time in that case.

{
bool isNonConfigThrow = (flags & PropertyOperation_ThrowOnDeleteIfNotConfig) == PropertyOperation_ThrowOnDeleteIfNotConfig;

Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Library/JavascriptError.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ namespace Js
static bool ThrowCantAssignIfStrictMode(PropertyOperationFlags flags, ScriptContext* scriptContext);
static bool ThrowCantExtendIfStrictMode(PropertyOperationFlags flags, ScriptContext* scriptContext);
static bool ThrowCantDeleteIfStrictMode(PropertyOperationFlags flags, ScriptContext* scriptContext, PCWSTR varName);
static bool ThrowCantDelete(PropertyOperationFlags flags, ScriptContext* scriptContext, PCWSTR varName);
static bool ThrowCantDeleteIfStrictModeOrNonconfigurable(PropertyOperationFlags flags, ScriptContext* scriptContext, PCWSTR varName);
static bool ThrowIfStrictModeUndefinedSetter(PropertyOperationFlags flags, Var setterValue, ScriptContext* scriptContext);
static bool ThrowIfNotExtensibleUndefinedSetter(PropertyOperationFlags flags, Var setterValue, ScriptContext* scriptContext);

Expand Down
7 changes: 7 additions & 0 deletions lib/Runtime/Library/TypedArray.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,6 +851,13 @@ namespace Js
return PropertyQueryFlags::Property_NotFound_NoProto;
}

BOOL TypedArrayBase::DeleteItem(uint32 index, Js::PropertyOperationFlags flags)
Copy link
Contributor Author

@IrinaYatsenko IrinaYatsenko Mar 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

flags [](start = 77, length = 5)

when running in strict mode, the flags will contain PropertyOperation_StrictMode
methods such as copyWithin would pass PropertyOperation_ThrowOnDeleteIfNotConfig #Resolved

{
JavascriptError::ThrowCantDeleteIfStrictModeOrNonconfigurable(
flags, GetScriptContext(), GetScriptContext()->GetIntegerString(index)->GetString());
return FALSE;
}

PropertyQueryFlags TypedArrayBase::GetItemQuery(Var originalInstance, uint32 index, Var* value, ScriptContext* requestContext)
{
*value = DirectGetItem(index);
Expand Down
2 changes: 1 addition & 1 deletion lib/Runtime/Library/TypedArray.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,7 +108,7 @@ namespace Js
virtual PropertyQueryFlags GetPropertyQuery(Js::Var originalInstance, Js::JavascriptString* propertyNameString, Js::Var* value, Js::PropertyValueInfo* info, Js::ScriptContext* requestContext) override;
virtual PropertyQueryFlags GetPropertyReferenceQuery(Js::Var originalInstance, Js::PropertyId propertyId, Js::Var* value, Js::PropertyValueInfo* info, Js::ScriptContext* requestContext) override;
virtual PropertyQueryFlags HasItemQuery(uint32 index) override;
virtual BOOL DeleteItem(uint32 index, Js::PropertyOperationFlags flags) override { return false; }
virtual BOOL DeleteItem(uint32 index, Js::PropertyOperationFlags flags) override;
virtual PropertyQueryFlags GetItemQuery(Js::Var originalInstance, uint32 index, Js::Var* value, Js::ScriptContext * requestContext) override;
virtual BOOL SetItem(uint32 index, Js::Var value, Js::PropertyOperationFlags flags = PropertyOperation_None) override;
virtual BOOL SetProperty(Js::PropertyId propertyId, Js::Var value, Js::PropertyOperationFlags flags, Js::PropertyValueInfo* info) override;
Expand Down
6 changes: 4 additions & 2 deletions lib/Runtime/Types/DictionaryTypeHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -912,7 +912,8 @@ namespace Js
else if (!(descriptor->Attributes & PropertyConfigurable))
{
// Let/const properties do not have attributes and they cannot be deleted
JavascriptError::ThrowCantDelete(propertyOperationFlags, scriptContext, propertyNameString->GetString());
JavascriptError::ThrowCantDeleteIfStrictModeOrNonconfigurable(
Copy link
Contributor Author

@IrinaYatsenko IrinaYatsenko Mar 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

ThrowCantDeleteIfStrictModeOrNonconfigurable [](start = 33, length = 44)

I wonder whether this one (and a few other current callers of ThrowCantDeleteIfStrictModeOrNonconfigurable) should actually call ThrowCantDeleteIfStrictMode because I don't think the flags would be set here to PropertyOperation_ThrowOnDeleteIfNotConfig (but I don't know how to confirm this). #Resolved

Copy link
Contributor

Choose a reason for hiding this comment

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

Seems safer to call the one that respects both flags. If ThrowOnDeleteIfNotConfig is never set, then it would make no difference in behavior.

In reply to: 173517742 [](ancestors = 173517742)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree, that it's safer. I was wondering about whether we could limit usage of ThrowCantDeleteIfStrictModeOrNonconfigurable (because I don't like this method :)).

In reply to: 173523027 [](ancestors = 173523027,173517742)

propertyOperationFlags, scriptContext, propertyNameString->GetString());

return false;
}
Expand Down Expand Up @@ -1007,7 +1008,8 @@ namespace Js
(allowLetConstGlobal && (descriptor->Attributes & PropertyLetConstGlobal)))
{
// Let/const properties do not have attributes and they cannot be deleted
JavascriptError::ThrowCantDelete(propertyOperationFlags, scriptContext, scriptContext->GetPropertyName(propertyId)->GetBuffer());
JavascriptError::ThrowCantDeleteIfStrictModeOrNonconfigurable(
propertyOperationFlags, scriptContext, scriptContext->GetPropertyName(propertyId)->GetBuffer());

return false;
}
Expand Down
3 changes: 2 additions & 1 deletion lib/Runtime/Types/ES5ArrayTypeHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -640,7 +640,8 @@ namespace Js
}
else if (!(descriptor->Attributes & PropertyConfigurable))
{
JavascriptError::ThrowCantDelete(propertyOperationFlags, instance->GetScriptContext(), TaggedInt::ToString(index, instance->GetScriptContext())->GetString());
JavascriptError::ThrowCantDeleteIfStrictModeOrNonconfigurable(
propertyOperationFlags, instance->GetScriptContext(), TaggedInt::ToString(index, instance->GetScriptContext())->GetString());

return false;
}
Expand Down
3 changes: 2 additions & 1 deletion lib/Runtime/Types/PathTypeHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -879,7 +879,8 @@ namespace Js
}
if (!(attr & ObjectSlotAttr_Configurable))
{
JavascriptError::ThrowCantDelete(PropertyOperation_None, scriptContext, scriptContext->GetPropertyName(propertyId)->GetBuffer());
JavascriptError::ThrowCantDeleteIfStrictModeOrNonconfigurable(
flags, scriptContext, scriptContext->GetPropertyName(propertyId)->GetBuffer());
Copy link
Contributor Author

@IrinaYatsenko IrinaYatsenko Mar 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

flags [](start = 16, length = 5)

Fix for #4118 (to throw when deleting non-configurable properties in strict mode) #Resolved

return FALSE;
}

Expand Down
6 changes: 4 additions & 2 deletions lib/Runtime/Types/SimpleDictionaryTypeHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1576,7 +1576,8 @@ namespace Js
}
else if (!(descriptor->Attributes & PropertyConfigurable))
{
JavascriptError::ThrowCantDelete(propertyOperationFlags, scriptContext, propertyNameString->GetString()); // or propertyName->GetBuffer
JavascriptError::ThrowCantDeleteIfStrictModeOrNonconfigurable(
propertyOperationFlags, scriptContext, propertyNameString->GetString()); // or propertyName->GetBuffer

return false;
}
Expand Down Expand Up @@ -1708,7 +1709,8 @@ namespace Js
else if (!(descriptor->Attributes & PropertyConfigurable) ||
(allowLetConstGlobal && (descriptor->Attributes & PropertyLetConstGlobal)))
{
JavascriptError::ThrowCantDelete(propertyOperationFlags, scriptContext, propertyRecord->GetBuffer());
JavascriptError::ThrowCantDeleteIfStrictModeOrNonconfigurable(
propertyOperationFlags, scriptContext, propertyRecord->GetBuffer());

return false;
}
Expand Down
3 changes: 2 additions & 1 deletion lib/Runtime/Types/SimpleTypeHandler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -556,7 +556,8 @@ namespace Js
}
if (!(descriptors[index].Attributes & PropertyConfigurable))
{
JavascriptError::ThrowCantDelete(propertyOperationFlags, scriptContext, scriptContext->GetPropertyName(propertyId)->GetBuffer());
JavascriptError::ThrowCantDeleteIfStrictModeOrNonconfigurable(
propertyOperationFlags, scriptContext, scriptContext->GetPropertyName(propertyId)->GetBuffer());

return false;
}
Expand Down
84 changes: 48 additions & 36 deletions test/Bugs/deletenonconfig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,45 +7,57 @@ WScript.LoadScriptFile("..\\UnitTestFramework\\UnitTestFramework.js");

function makeProperty(obj, prop) {
Object.defineProperty(obj, prop, {
configurable: false,
writable: true,
value:'basic'
configurable: false,
writable: true,
value: 'basic'
});
}

var tests = [
{
name: "Delete non-configurable property on Array.prototype.copyWithin",
body: function () {
var obj = {length: 4};
makeProperty(obj, '3');
assert.throws(() => Array.prototype.copyWithin.call(obj, 3, 0), TypeError, "copyWithin is trying to delete the non-configurable property","Cannot delete non-configurable property '3'");
}
},
{
name: "Delete non-configurable property on Array.prototype.pop",
body: function () {
var obj = {length: 4};
makeProperty(obj, '3');
assert.throws(() => Array.prototype.pop.call(obj), TypeError, "pop is trying to delete the non-configurable property", "Cannot delete non-configurable property '3'");
}
},
{
name: "Delete non-configurable property on Array.prototype.shift",
body: function () {
var obj = {length: 4};
makeProperty(obj, '3');
assert.throws(() => Array.prototype.shift.call(obj), TypeError, "shift is trying to delete the non-configurable property", "Cannot delete non-configurable property '3'");
}
},
{
name: "Delete non-configurable property on Array.prototype.reverse",
body: function () {
var obj = {length: 4};
makeProperty(obj, '3');
assert.throws(() => Array.prototype.reverse.call(obj), TypeError, "reverse is trying to delete the non-configurable property", "Cannot delete non-configurable property '3'");
}
},

{
name: "Delete non-configurable property on Array.prototype.copyWithin",
body: function () {
var obj = { length: 4 };
makeProperty(obj, '3');
assert.throws(() => Array.prototype.copyWithin.call(obj, 3, 0), TypeError, "copyWithin is trying to delete the non-configurable property", "Cannot delete non-configurable property '3'");
}
},
{
name: "Delete non-configurable indexed property of TypedArray on Array.prototype.copyWithin",
body: function () {
var ta = Int8Array.of(0, 1, 2);
// Array's implementation of copyWithin uses 'length' property to determine the source items
// to copy and it would attempt to delete the target items if the source items are missing,
// which is not supported by TypedArrays.
Object.defineProperty(ta, "length", { value: 4 });
assert.throws(() => Array.prototype.copyWithin.call(ta, 1, 2), TypeError, "copyWithin is trying to delete the non-configurable indexed property", "Cannot delete non-configurable property '2'");
}
},
{
name: "Delete non-configurable property on Array.prototype.pop",
body: function () {
var obj = { length: 4 };
makeProperty(obj, '3');
assert.throws(() => Array.prototype.pop.call(obj), TypeError, "pop is trying to delete the non-configurable property", "Cannot delete non-configurable property '3'");
}
},
{
name: "Delete non-configurable property on Array.prototype.shift",
body: function () {
var obj = { length: 4 };
makeProperty(obj, '3');
assert.throws(() => Array.prototype.shift.call(obj), TypeError, "shift is trying to delete the non-configurable property", "Cannot delete non-configurable property '3'");
}
},
{
name: "Delete non-configurable property on Array.prototype.reverse",
body: function () {
var obj = { length: 4 };
makeProperty(obj, '3');
assert.throws(() => Array.prototype.reverse.call(obj), TypeError, "reverse is trying to delete the non-configurable property", "Cannot delete non-configurable property '3'");
}
},

];

testRunner.runTests(tests, { verbose: WScript.Arguments[0] != "summary" });
4 changes: 2 additions & 2 deletions test/es6/ES6TypedArrayExtensions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1314,12 +1314,12 @@ var tests = [
// Call Array.prototype.reverse passing a TypedArray that lies about length. We should only reverse the part of it less than the indicated length.
u = getTypedArray();
Object.defineProperty(u, 'length', { value: 5 });
assert.areEqual([4,3,2,1,0,5,6,7,8,9], Array.prototype.reverse.call(u), "Calling %TypedArrayPrototype%.reverse with a TypedArray that lies about length");
assert.areEqual([4,3,2,1,0,5,6,7,8,9], Array.prototype.reverse.call(u), "Calling %ArrayPrototype%.reverse with a TypedArray that lies about length");

// Call Array.prototype.reverse passing a TypedArray that lies about length. TypedArrays do not support delete so we will throw if indicated length is longer than actual.
u = getTypedArray();
Object.defineProperty(u, 'length', { value: 20 });
assert.throws(function() { Array.prototype.reverse.call(u); }, TypeError, "Calling %TypedArrayPrototype%.reverse with a TypedArray that says it has longer length than actual throws", "Object doesn't support this action");
assert.throws(function () { Array.prototype.reverse.call(u); }, TypeError, "Calling %ArrayPrototype%.reverse with a TypedArray that says it has longer length than actual throws", "Cannot delete non-configurable property '0'");

assert.throws(function() { reverse.call(); }, TypeError, "Calling %TypedArrayPrototype%.reverse with no this throws TypeError", "'this' is not a typed array object");
assert.throws(function() { reverse.call(undefined); }, TypeError, "Calling %TypedArrayPrototype%.reverse with undefined this throws TypeError", "'this' is not a typed array object");
Expand Down
7 changes: 6 additions & 1 deletion test/strict/rlexe.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,12 @@
<files>13.delete.js</files>
<baseline>13.delete.baseline</baseline>
<compile-flags>-ES6RegExPrototypeProperties-</compile-flags>
<!-- TODO: Delete depends on flag on scriptContext. Add a new opcode for delete 13.delete_sm.js -->
</default>
</test>
<test>
<default>
<files>13.delete_sm.js</files>
<baseline>13.delete_sm.baseline</baseline>
</default>
Copy link
Contributor Author

@IrinaYatsenko IrinaYatsenko Mar 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This existing test passes "as is" so decided not to rewrite it :) #Resolved

</test>
<test>
Expand Down