Not using strict mode within ClassDeclaration statement

• Component: JavaScriptCore
• Status: RESOLVED FIXED

The bug can be reproduced in latest jsc version.
code:

class c extends(d = function() {
    return 0;
}, d) {};

JSC print nothing, however, it should throw an error.

According to ecma-262
• All parts of a ClassDeclaration or a ClassExpression are strict mode code.
• In strict mode code, assignment to an undeclared identifier or otherwise unresolvable reference does not create a property in the global object. When a simple assignment occurs within strict mode code, its LeftHandSideExpression must not evaluate to an unresolvable Reference. If it does, a ReferenceError exception is thrown.
So, assignment to ‘d’, which is undeclared, should throw a ReferenceError exception.

ISec Lab
2019.12.24

[Radar WebKit Bug Importer]

<rdar://problem/58194589>

[Tadeu Zagallo]

Created attachment 394293 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 394293 [details]
Patch

Can you check whether bytecode execution works if we have `codeBlock->isStrictMode()` thing in runtime code?
For example, we are checking `codeBlock()->isStrictMode()` in deleteById called from operationDeleteByIdOptimize.

[Tadeu Zagallo]

Comment on attachment 394293 [details]
Patch

You are right, we use that in several places, this is not correct...

[Tadeu Zagallo]

Created attachment 394733 [details]
Patch

[Tadeu Zagallo]

Created attachment 394759 [details]
Patch

Rebase

[Yusuke Suzuki]

Comment on attachment 394759 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394759&action=review

> Source/JavaScriptCore/bytecode/BytecodeList.rb:513
> +        ecmaMode: ECMAMode,

Can you merge ecmaMode into PutByIdFlags? size of put_by_id is important to make bytecodes small to save memory.

> Source/JavaScriptCore/bytecode/BytecodeList.rb:790
> +        ecmaMode: ECMAMode,

I think this is dangerous. A lot of place is assuming that op_call / op_call_xxx etc.'s bytecode length are same IIRC.
Can you add `call_eval_sloppy` / `call_eval_strict` instead to keep the above invariant?

> Source/JavaScriptCore/bytecode/BytecodeList.rb:971
> +        ecmaMode: ECMAMode,

Can you add this into GetPutInfo to squeeze the length of put_to_scope. This is important to save memory.

> Source/JavaScriptCore/runtime/JSCJSValue.h:87
> +    static constexpr uint8_t NotStrictMode = 1;

You can name it SloppyMode. This is not official term, but well-used term in practice.
https://developer.mozilla.org/en-US/docs/Glossary/Sloppy_mode

> Source/JavaScriptCore/runtime/JSCJSValue.h:92
> +    static constexpr ECMAMode notStrict() { return ECMAMode(NotStrictMode); }

change it to `sloppy()`.

[Tadeu Zagallo]

Comment on attachment 394759 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394759&action=review

>> Source/JavaScriptCore/bytecode/BytecodeList.rb:513
>> +        ecmaMode: ECMAMode,
> 
> Can you merge ecmaMode into PutByIdFlags? size of put_by_id is important to make bytecodes small to save memory.

Done.

>> Source/JavaScriptCore/bytecode/BytecodeList.rb:790
>> +        ecmaMode: ECMAMode,
> 
> I think this is dangerous. A lot of place is assuming that op_call / op_call_xxx etc.'s bytecode length are same IIRC.
> Can you add `call_eval_sloppy` / `call_eval_strict` instead to keep the above invariant?

I don't think that's true anymore. With the new bytecode format there should be no assumptions about the size of any bytecodes.

>> Source/JavaScriptCore/bytecode/BytecodeList.rb:971
>> +        ecmaMode: ECMAMode,
> 
> Can you add this into GetPutInfo to squeeze the length of put_to_scope. This is important to save memory.

Done

>> Source/JavaScriptCore/runtime/JSCJSValue.h:87
>> +    static constexpr uint8_t NotStrictMode = 1;
> 
> You can name it SloppyMode. This is not official term, but well-used term in practice.
> https://developer.mozilla.org/en-US/docs/Glossary/Sloppy_mode

I thought about, but figured I'd keep the naming. I'll update it.

>> Source/JavaScriptCore/runtime/JSCJSValue.h:92
>> +    static constexpr ECMAMode notStrict() { return ECMAMode(NotStrictMode); }
> 
> change it to `sloppy()`.

👍

[Tadeu Zagallo]

Created attachment 394965 [details]
Patch

[Tadeu Zagallo]

Created attachment 394998 [details]
Patch

Rebase

[Tadeu Zagallo]

Created attachment 395008 [details]
Patch

Fix WebCore build

[Tadeu Zagallo]

Created attachment 395054 [details]
Patch

Fix Fits<GetPutInfo>

[Tadeu Zagallo]

Created attachment 395089 [details]
Patch

Fix Windows build

[Tadeu Zagallo]

Created attachment 395601 [details]
Patch

Try to fix i386 build

[Tadeu Zagallo]

Created attachment 395607 [details]
Patch

Rebase

[Tadeu Zagallo]

Created attachment 395623 [details]
Patch

Fix CMake builds

[Yusuke Suzuki]

Comment on attachment 395623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395623&action=review

r=me, nice.

> Source/JavaScriptCore/bytecode/CodeBlock.h:-239
> -        if (reg == thisRegister() && !isStrictMode())
> -            return true;
> -

Can we make it work by passing ECMAMode to this function?

> Source/JavaScriptCore/bytecode/Fits.h:375
> +    // a pair of (ResultType::Type, ResultType::Type) - try to fit each type into 4 bits
> +    // additionally, encode unknown types as 0 rather than the | of all types
> +    static constexpr unsigned typeWidth = 4;
> +    static constexpr unsigned maxType = (1 << typeWidth) - 1;

ECMAMode does not include ResultType. Can you fix this implementation? I think ECMAMode is 1bit flag.

> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1317
> +    class StrictModeScope {

Let's implement it by using WTF::SetForScope.

[Tadeu Zagallo]

Comment on attachment 395623 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=395623&action=review

>> Source/JavaScriptCore/bytecode/CodeBlock.h:-239
>> -
> 
> Can we make it work by passing ECMAMode to this function?

That's what I started with, but it's used from many places where we don't currently have the mode. We'd need to add it to many more opcodes. I discussed it with Saam offline and he seemed to believe it should be ok to remove this optimization.

>> Source/JavaScriptCore/bytecode/Fits.h:375
>> +    static constexpr unsigned maxType = (1 << typeWidth) - 1;
> 
> ECMAMode does not include ResultType. Can you fix this implementation? I think ECMAMode is 1bit flag.

Oops, this is just left over from copy paste. The implementation below doesn't use it, I'll remove.

>> Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h:1317
>> +    class StrictModeScope {
> 
> Let's implement it by using WTF::SetForScope.

Will do.

[Tadeu Zagallo]

Created attachment 395722 [details]
Patch for landing

[Tadeu Zagallo]

Created attachment 395727 [details]
Patch for landing

[Tadeu Zagallo]

Comment on attachment 395727 [details]
Patch for landing

I'll add an extra test provided by Saam

[Tadeu Zagallo]

Created attachment 395741 [details]
Patch for landing

[EWS]

Committed r259676: <https://trac.webkit.org/changeset/259676>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 395741 [details].

[Michael Catanzaro]

Looks like this broke the 32-bit JSCOnly bots pretty badly. DFGSpeculativeJIT32_64.cpp was not updated.

[Pablo Saavedra]

(In reply to Michael Catanzaro from comment #24)
> Looks like this broke the 32-bit JSCOnly bots pretty badly.
> DFGSpeculativeJIT32_64.cpp was not updated.

yes. I just filled a bug reporting the same: https://bugs.webkit.org/show_bug.cgi?id=210176