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

IrinaYatsenko · 2018-03-07T22:09:36Z

	delete obj.foo non-configurable	delete ta.foo configurable	delete ta.foo doesn't exist	delete ta.foo non-configurable	delete ta[index]	Array.copyWithin with delete
strict	throw	delete, return true	nop, return true	throw	throw	throw
non-strict	nop, return false	delete, return true	nop, return true	nop, return false	nop, return false	throw

MSLaguana · 2018-03-07T23:59:23Z

lib/Runtime/Language/JavascriptOperators.cpp

@@ -4818,7 +4818,8 @@ namespace Js
 #endif
        }

-        if (result == FALSE && (propertyOperationFlags & PropertyOperation_StrictMode))
+        if (result == FALSE && 
+            (propertyOperationFlags & (PropertyOperation_StrictMode | PropertyOperation_ThrowOnDeleteIfNotConfig)))


This looks reasonable to me, but it does make me wonder whether there's some other scenario somewhere that might directly call into someArray->DeleteItem and expect that it throws when appropriate; a quick look around showed code like this: https://github.com/Microsoft/ChakraCore/blob/master/lib/Runtime/Library/JavascriptArray.cpp#L5649 which I think converts the false return into throwing explicitly, but seems to expect the typed array will do something with the flag it is given rather than completely ignoring it. Do you know if there are any other potential scenarios? #Resolved

+1

Not sure this is the right place for this check. When result == FALSE, it might be for another reason other than not configurable. #Resolved

result looks like it always is returned by JavascriptOperators::DeleteProperty. What other reason could it return false for? #Resolved

It looks to me like result is only false in the non-configurable case, so this should work.

Like Jimmy mentioned, various other methods invite misuse because they accept a PropertyOperationFlags parameter but don't pay attention to PropertyOperation_ThrowOnDeleteIfNotConfig. It seems somewhat inconsistent; some methods like PathTypeHandlerBase::DeleteProperty do respect that flag, but others don't. I'm not sure if it's worth trying to track them all down at this point.

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

After all, it seems that it's better to have this handling in TypedArrayBase::DeleteItem as we discussed before :)

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

boingoing · 2018-03-08T20:36:55Z

@dotnet-bot test Pre-Merge signing tool please #Resolved

MSLaguana · 2018-03-08T20:39:12Z

That's not part of the dotnet CI; I think a push will be required to trigger that. #Resolved

digitalinfinity · 2018-03-09T00:19:20Z

lib/Runtime/Library/JavascriptArray.cpp

@@ -9131,7 +9131,8 @@ namespace Js
                }
                else
                {
-                    JS_REENTRANT(jsReentLock, obj->DeleteItem(toIndex, PropertyOperation_ThrowOnDeleteIfNotConfig));
+                    JavascriptOperators::OP_DeleteElementI(obj, JavascriptNumber::ToVar(toIndex, scriptContext), scriptContext, PropertyOperation_ThrowOnDeleteIfNotConfig);


Don't we still need to wrap this in JS_REENTRANT? #Resolved

For more context, we explicitly mark any lines in here that are allowed to call back into script (it would be a bug if any line without such a marking could call script). This pattern is useful because we can assume that certain things don't change as long as we haven't called into script (for instance, an instance of JavascriptArray hasn't magically transformed itself into an instance of ES5Array). The object here could be a Proxy with a deleteProperty handler, in which case this deletion operation would call script.

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

Thank you for the explanations! I'm reverting this file but I might need to follow up offline to understand better about the Proxies and reentrancy :)

In reply to: 173337067 [](ancestors = 173337067,173333445)

boingoing · 2018-03-09T02:10:00Z

lib/Runtime/Language/JavascriptOperators.cpp

@@ -4818,7 +4818,8 @@ namespace Js
 #endif
        }

-        if (result == FALSE && (propertyOperationFlags & PropertyOperation_StrictMode))
+        if (result == FALSE && 
+            (propertyOperationFlags & (PropertyOperation_StrictMode | PropertyOperation_ThrowOnDeleteIfNotConfig)))
        {
            JavascriptError::ThrowCantDelete(propertyOperationFlags, scriptContext, GetPropertyDisplayNameForError(index, scriptContext)->GetString());


Doesn't JavascriptError::ThrowCantDelete already check propertyOperationFlags? Can we write this as:

if (!result) { JavascriptError::ThrowCantDelete(propertyOperationFlags, scriptContext, GetPropertyDisplayNameForError(index, scriptContext)->GetString()); } ``` #Resolved

boingoing · 2018-03-09T02:10:47Z

lib/Runtime/Library/JavascriptArray.cpp

@@ -9131,7 +9131,8 @@ namespace Js
                }
                else
                {
-                    JS_REENTRANT(jsReentLock, obj->DeleteItem(toIndex, PropertyOperation_ThrowOnDeleteIfNotConfig));
+                    JavascriptOperators::OP_DeleteElementI(obj, JavascriptNumber::ToVar(toIndex, scriptContext), scriptContext, PropertyOperation_ThrowOnDeleteIfNotConfig);
+                    //JS_REENTRANT(jsReentLock, obj->DeleteItem(toIndex, PropertyOperation_ThrowOnDeleteIfNotConfig));


Also nitpick, please delete this commented line. #Resolved

I'm moving the throw into TypedArrayBase to be aligned with other DeleteProperty/DeleteItem handlers, so there will be no change here.

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

IrinaYatsenko · 2018-03-09T17:36:09Z

lib/Runtime/Library/JavascriptError.cpp

@@ -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)


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

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

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)

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)

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).

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)

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.

MSLaguana · 2018-03-09T17:36:28Z

lib/Runtime/Language/JavascriptOperators.cpp

@@ -4818,11 +4818,6 @@ namespace Js
 #endif
        }

-        if (result == FALSE && (propertyOperationFlags & PropertyOperation_StrictMode))


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

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)

IrinaYatsenko · 2018-03-09T17:38:13Z

lib/Runtime/Library/TypedArray.cpp

@@ -851,6 +851,13 @@ namespace Js
        return PropertyQueryFlags::Property_NotFound_NoProto;
    }

+    BOOL TypedArrayBase::DeleteItem(uint32 index, Js::PropertyOperationFlags flags)


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

IrinaYatsenko · 2018-03-09T17:40:12Z

lib/Runtime/Types/DictionaryTypeHandler.cpp

@@ -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(


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

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)

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)

IrinaYatsenko · 2018-03-09T17:41:07Z

lib/Runtime/Types/PathTypeHandler.cpp

@@ -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());


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

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

IrinaYatsenko · 2018-03-09T17:41:59Z

test/strict/rlexe.xml

+  <test>
+    <default>
+      <files>13.delete_sm.js</files>
+      <baseline>13.delete_sm.baseline</baseline>
    </default>


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

MSLaguana · 2018-03-09T17:42:19Z

Looks like this test has caused a test in ES6TypedArrayExtensions.js to fail. From what I can tell, it seems that it is expecting one kind of error, but is now getting an error on deleting a nonconfiguable property #Resolved

MSLaguana · 2018-03-09T17:47:02Z

I think the new behavior is more correct, so the test just needs to have its expectation updated I believe. #Resolved

IrinaYatsenko · 2018-03-09T17:57:56Z

Yes, a different error message because. I've updated the test.

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

IrinaYatsenko · 2018-03-09T17:59:06Z

I don't know why the signing tool didn't kick in on my PRs yesterday, but it seems to be doing so now.

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

… slots Rename ThrowCantDelete and fix regression from chakra-core#4118 (delete non-config props in strict mode)

boingoing

👍

… on TypedArray with missing slots Merge pull request #4790 from irinayat-MS:Throw_copyWithin Fixes #4356 and a regression from #4118

MSLaguana reviewed Mar 7, 2018

View reviewed changes

boingoing closed this Mar 8, 2018

boingoing reopened this Mar 8, 2018

digitalinfinity reviewed Mar 9, 2018

View reviewed changes

digitalinfinity requested a review from sethbrenith March 9, 2018 00:19

boingoing reviewed Mar 9, 2018

View reviewed changes

IrinaYatsenko commented Mar 9, 2018

View reviewed changes

MSLaguana reviewed Mar 9, 2018

View reviewed changes

MSLaguana approved these changes Mar 9, 2018

View reviewed changes

IrinaYatsenko commented Mar 9, 2018

View reviewed changes

Array.copyWithin should throw when invoked on TypedArray with missing…

f219294

… slots Rename ThrowCantDelete and fix regression from chakra-core#4118 (delete non-config props in strict mode)

IrinaYatsenko force-pushed the Throw_copyWithin branch from ceb0481 to f219294 Compare March 9, 2018 19:00

boingoing approved these changes Mar 9, 2018

View reviewed changes

chakrabot merged commit f219294 into chakra-core:master Mar 9, 2018

chakrabot pushed a commit that referenced this pull request Mar 9, 2018

[MERGE #4790 @irinayat-MS] Array.copyWithin should throw when invoked…

e972090

… on TypedArray with missing slots Merge pull request #4790 from irinayat-MS:Throw_copyWithin Fixes #4356 and a regression from #4118

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

IrinaYatsenko commented Mar 7, 2018 •

edited

Loading

MSLaguana Mar 7, 2018 •

edited by IrinaYatsenko

Loading

boingoing Mar 8, 2018 •

edited by IrinaYatsenko

Loading

digitalinfinity Mar 9, 2018 •

edited by IrinaYatsenko

Loading

sethbrenith Mar 9, 2018

IrinaYatsenko Mar 9, 2018

boingoing commented Mar 8, 2018 •

edited by IrinaYatsenko

Loading

MSLaguana commented Mar 8, 2018 •

edited by IrinaYatsenko

Loading

digitalinfinity Mar 9, 2018 •

edited by IrinaYatsenko

Loading

sethbrenith Mar 9, 2018

IrinaYatsenko Mar 9, 2018

boingoing Mar 9, 2018 •

edited by IrinaYatsenko

Loading

boingoing Mar 9, 2018 •

edited by IrinaYatsenko

Loading

IrinaYatsenko Mar 9, 2018

IrinaYatsenko Mar 9, 2018 •

edited

Loading

sethbrenith Mar 9, 2018

sethbrenith Mar 9, 2018

IrinaYatsenko Mar 9, 2018

boingoing Mar 9, 2018

MSLaguana Mar 9, 2018 •

edited by IrinaYatsenko

Loading

sethbrenith Mar 9, 2018

IrinaYatsenko Mar 9, 2018 •

edited

Loading

IrinaYatsenko Mar 9, 2018 •

edited

Loading

sethbrenith Mar 9, 2018

IrinaYatsenko Mar 9, 2018

IrinaYatsenko Mar 9, 2018 •

edited

Loading

IrinaYatsenko Mar 9, 2018 •

edited

Loading

MSLaguana commented Mar 9, 2018 •

edited by IrinaYatsenko

Loading

MSLaguana commented Mar 9, 2018 •

edited by IrinaYatsenko

Loading

IrinaYatsenko commented Mar 9, 2018

IrinaYatsenko commented Mar 9, 2018

boingoing left a comment

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

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

Conversation

IrinaYatsenko commented Mar 7, 2018 • edited Loading

MSLaguana Mar 7, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

boingoing Mar 8, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

digitalinfinity Mar 9, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing commented Mar 8, 2018 • edited by IrinaYatsenko Loading

MSLaguana commented Mar 8, 2018 • edited by IrinaYatsenko Loading

digitalinfinity Mar 9, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

boingoing Mar 9, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

boingoing Mar 9, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IrinaYatsenko Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MSLaguana Mar 9, 2018 • edited by IrinaYatsenko Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IrinaYatsenko Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

IrinaYatsenko Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IrinaYatsenko Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

IrinaYatsenko Mar 9, 2018 • edited Loading

Choose a reason for hiding this comment

MSLaguana commented Mar 9, 2018 • edited by IrinaYatsenko Loading

MSLaguana commented Mar 9, 2018 • edited by IrinaYatsenko Loading

IrinaYatsenko commented Mar 9, 2018

IrinaYatsenko commented Mar 9, 2018

boingoing left a comment

Choose a reason for hiding this comment

IrinaYatsenko commented Mar 7, 2018 •

edited

Loading

MSLaguana Mar 7, 2018 •

edited by IrinaYatsenko

Loading

boingoing Mar 8, 2018 •

edited by IrinaYatsenko

Loading

digitalinfinity Mar 9, 2018 •

edited by IrinaYatsenko

Loading

boingoing commented Mar 8, 2018 •

edited by IrinaYatsenko

Loading

MSLaguana commented Mar 8, 2018 •

edited by IrinaYatsenko

Loading

digitalinfinity Mar 9, 2018 •

edited by IrinaYatsenko

Loading

boingoing Mar 9, 2018 •

edited by IrinaYatsenko

Loading

boingoing Mar 9, 2018 •

edited by IrinaYatsenko

Loading

IrinaYatsenko Mar 9, 2018 •

edited

Loading

MSLaguana Mar 9, 2018 •

edited by IrinaYatsenko

Loading

IrinaYatsenko Mar 9, 2018 •

edited

Loading

IrinaYatsenko Mar 9, 2018 •

edited

Loading

IrinaYatsenko Mar 9, 2018 •

edited

Loading

IrinaYatsenko Mar 9, 2018 •

edited

Loading

MSLaguana commented Mar 9, 2018 •

edited by IrinaYatsenko

Loading

MSLaguana commented Mar 9, 2018 •

edited by IrinaYatsenko

Loading