Bug 225277 - REGRESSION (r271119): Object methods defined with shorthand notation cannot access “caller” in non-strict mode
Summary: REGRESSION (r271119): Object methods defined with shorthand notation cannot a...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Alexey Shvayka
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-01 22:53 PDT by Jonathan Grimes
Modified: 2021-05-18 09:10 PDT (History)
11 users (show)

See Also:


Attachments
Patch (11.54 KB, patch)
2021-05-14 11:41 PDT, Alexey Shvayka
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Grimes 2021-05-01 22:53:31 PDT
In a script that is not in strict mode defining a object property that has a function as a value using shorthand notation will not be able to access its caller property.
Comment 1 Jonathan Grimes 2021-05-01 22:55:32 PDT
Here is an example:
https://jsfiddle.net/jsg2021/4voxyca2/
Comment 2 Jonathan Grimes 2021-05-01 22:56:06 PDT
If I change object-shorthand notation to old-skewl object notation it works...
ie:
{ constructor: function() {
      this.callParent(arguments);
}}

vs

{ constructor() {
      this.callParent(arguments);
}}
Comment 3 Radar WebKit Bug Importer 2021-05-06 17:43:26 PDT
<rdar://problem/77633889>
Comment 4 Alexey Shvayka 2021-05-14 11:41:38 PDT
Created attachment 428644 [details]
Patch
Comment 5 Alexey Shvayka 2021-05-14 11:43:12 PDT
(In reply to Jonathan Grimes from comment #1)
> Here is an example:
> https://jsfiddle.net/jsg2021/4voxyca2/

Hey Jonathan, thank you for nice repro and sorry for the breakage.
Comment 6 Jonathan Grimes 2021-05-14 11:46:38 PDT
thank you :)
Comment 7 Darin Adler 2021-05-15 15:55:14 PDT
Comment on attachment 428644 [details]
Patch

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

> Source/JavaScriptCore/runtime/JSFunction.cpp:413
> +    SourceParseMode parseMode = function->jsExecutable()->parseMode();

Dueling coding styles: This would be better with auto. We don’t want to name the type, risking that if we named the wrong one we could cause type conversion, just to get the function result and pass it along to predicate functions like isGeneratorParseMode.

But you have told me that auto is less in fashion in JavaScriptCore, as coding style and project culture subtly drifts apart.
Comment 8 Alexey Shvayka 2021-05-17 14:43:48 PDT
Comment on attachment 428644 [details]
Patch

(In reply to Darin Adler from comment #7)

Thank you for review!

> > Source/JavaScriptCore/runtime/JSFunction.cpp:413
> > +    SourceParseMode parseMode = function->jsExecutable()->parseMode();
> 
> Dueling coding styles: This would be better with auto. We don’t want to name
> the type, risking that if we named the wrong one we could cause type
> conversion, just to get the function result and pass it along to predicate
> functions like isGeneratorParseMode.

Coming from JS background, I would personally prefer `auto` since in most modern IDEs, type inference is available on :hover.
However, across the JSC codebase, result of parseMode() is explicitly typed.

Darin, is there a protocol to get this patch cherry-picked into Safari 14.5.X?
Comment 9 EWS 2021-05-17 16:20:25 PDT
Committed r277613 (237828@main): <https://commits.webkit.org/237828@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 428644 [details].
Comment 10 Darin Adler 2021-05-17 23:30:47 PDT
(In reply to Alexey Shvayka from comment #8)
> Darin, is there a protocol to get this patch cherry-picked into Safari
> 14.5.X?

Need to bring this to the attention of the people inside Apple who pull bugs into release branches. I’m not sure it fits the mission of those kinds of releases. Helpful comment about the impact of the bug could make difference.
Comment 11 Alexey Shvayka 2021-05-18 09:10:23 PDT
(In reply to Darin Adler from comment #10)
> Need to bring this to the attention of the people inside Apple who pull bugs
> into release branches. I’m not sure it fits the mission of those kinds of
> releases. Helpful comment about the impact of the bug could make difference.

With best intentions, r271119 made `function.caller` return `null` for callers that are getters / setters, arrow functions, and ES6 methods. The old code using `function.caller` is not aware it's being called from modern JavaScript. ExtJS 4.x (maybe even later versions) class implementation relies on `function.caller` to perform super(), and is currently broken for userland code that uses new methods syntax. Given that this bug was reported days after 14.5 release, it appears that r271119 has high chances of breaking the web.