Bug 190671 – The parser should not emit a ApplyFunctionCallDotNode for Reflect.apply.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

The bytecode generator does not currently know how to inline Reflect.apply (see https://bugs.webkit.org/show_bug.cgi?id=190668).  Hence, it's a waste of time to emit the ApplyFunctionCallDotNode since the function check against Function.apply that it will generate will always fail.  It also causes DFG graph dumping to crash.

<rdar://problem/45201145>

[Mark Lam]

Created attachment 352592 [details]
proposed patch.

Let's get some EWS testing first.

[Saam Barati]

Comment on attachment 352592 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:12
> +        that it will generate will always fail.  It can also cause DFG graph dumping to

I agree with this from a performance perspective. However, it seems like you’re sidestepping a crash. Why does the dumper crash? Seems like that should be fixed

[Mark Lam]

(In reply to Saam Barati from comment #2)
> Comment on attachment 352592 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=352592&action=review
> 
> > Source/JavaScriptCore/ChangeLog:12
> > +        that it will generate will always fail.  It can also cause DFG graph dumping to
> 
> I agree with this from a performance perspective. However, it seems like
> you’re sidestepping a crash. Why does the dumper crash? Seems like that
> should be fixed

It crashed because it's expecting the callee in a CallVariant to be a function or executable of some form.  Because we're creating a ApplyFunctionCallDotNode for Reflect.apply, we end up passing the Reflect object as the callee.  This is wrong, and is the reason the dumper crashed.  We should be passing the first argument of Reflect.apply as the callee instead (when we support inlining Reflect.apply).

Hmmm ... that said, I haven't explored what happens in we can properly inline Reflect.apply later and pass a non-function value as the first argument.  I think we can leave that as an exercise for https://bugs.webkit.org/show_bug.cgi?id=190668.

[EWS Watchlist]

Comment on attachment 352592 [details]
proposed patch.

Attachment 352592 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9638635

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 352601 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 352592 [details]
proposed patch.

Attachment 352592 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9638736

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 352602 [details]
Archive of layout-test-results from ews106 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Saam Barati]

(In reply to Mark Lam from comment #3)
> (In reply to Saam Barati from comment #2)
> > Comment on attachment 352592 [details]
> > proposed patch.
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=352592&action=review
> > 
> > > Source/JavaScriptCore/ChangeLog:12
> > > +        that it will generate will always fail.  It can also cause DFG graph dumping to
> > 
> > I agree with this from a performance perspective. However, it seems like
> > you’re sidestepping a crash. Why does the dumper crash? Seems like that
> > should be fixed
> 
> It crashed because it's expecting the callee in a CallVariant to be a
> function or executable of some form.  Because we're creating a
> ApplyFunctionCallDotNode for Reflect.apply, we end up passing the Reflect
> object as the callee.  This is wrong, and is the reason the dumper crashed. 
> We should be passing the first argument of Reflect.apply as the callee
> instead (when we support inlining Reflect.apply).
I don't see how this matters at all that the callee here is Reflect. Seems like any code can trigger this.


> 
> Hmmm ... that said, I haven't explored what happens in we can properly
> inline Reflect.apply later and pass a non-function value as the first
> argument.  I think we can leave that as an exercise for
> https://bugs.webkit.org/show_bug.cgi?id=190668.

[EWS Watchlist]

Comment on attachment 352592 [details]
proposed patch.

Attachment 352592 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/9638622

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 352605 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Mark Lam]

(In reply to Saam Barati from comment #8)
> I don't see how this matters at all that the callee here is Reflect. Seems
> like any code can trigger this.

You are right.  I stand corrected.  Will fix.

[Mark Lam]

Created attachment 352635 [details]
proposed patch.

Let's try on the EWS again.

[Saam Barati]

Comment on attachment 352635 [details]
proposed patch.

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

r=me

> Source/JavaScriptCore/ChangeLog:11
> +        https://bugs.webkit.org/show_bug.cgi?id=190668).  Hence, it's a waste of time to
> +        emit the ApplyFunctionCallDotNode since the function check against Function.apply

What you're doing is a heuristic, FWIW. I agree with your change, because it's probably better, but it's still a heuristic. A user could write code where this new heuristic is worse:
```
function Reflect() { ... }
Reflect.apply
```

[Mark Lam]

Thanks for the review.

(In reply to Saam Barati from comment #13)
> > Source/JavaScriptCore/ChangeLog:11
> > +        https://bugs.webkit.org/show_bug.cgi?id=190668).  Hence, it's a waste of time to
> > +        emit the ApplyFunctionCallDotNode since the function check against Function.apply
> 
> What you're doing is a heuristic, FWIW. I agree with your change, because
> it's probably better, but it's still a heuristic. A user could write code
> where this new heuristic is worse:
> ```
> function Reflect() { ... }
> Reflect.apply
> ```

Yes, I thought of that.  My thinking is that if the user wrote code like that, he/she is just asking for it.  For most normal code, I would not expect the user to create a function named Reflect with a member named apply.  So, I opted to go with this heuristic.

[WebKit Commit Bot]

Comment on attachment 352635 [details]
proposed patch.

Clearing flags on attachment: 352635

Committed r237241: <https://trac.webkit.org/changeset/237241>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.