Bug 140093 - Argument object created by "Function dot arguments" should use a clone of argument values
Summary: Argument object created by "Function dot arguments" should use a clone of arg...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks: 139827
  Show dependency treegraph
 
Reported: 2015-01-05 13:07 PST by Mark Lam
Modified: 2015-01-15 11:21 PST (History)
7 users (show)

See Also:


Attachments
the patch. (4.24 KB, patch)
2015-01-05 13:36 PST, Mark Lam
no flags Details | Formatted Diff | Diff
patch 2: fixed a typo in the ChangeLog. (4.25 KB, patch)
2015-01-08 11:50 PST, Mark Lam
no flags Details | Formatted Diff | Diff
patch 2: fixed bugs from the previous patch + add needed tests. (21.78 KB, patch)
2015-01-14 20:40 PST, Mark Lam
ggaren: review+
ggaren: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2015-01-05 13:07:04 PST
After https://bugs.webkit.org/show_bug.cgi?id=139827, a few test failures will start failing.  One of them is dfg-tear-off-arguments-not-activation.js, which can be run this way:

$ jsc --useFTLJIT\=false --enableFunctionDotArguments\=true --testTheFTL\=true --useFTLJIT\=true resources/standalone-pre.js dfg-tear-off-arguments-not-activation.js resources/standalone-post.js

The relevant code is as follows:

    function bar() {
        return foo.arguments;
    }

    function foo(p) {
        var x = 42;
        if (p)
            return (function() { return x; });
        else
            return bar();
    }

In this case, foo() has no knowledge of bar() needing the LexicalEnvironment and has dead code eliminated the SetLocal that stores it into its designated local.  In bar(), the factory for the Arguments object (for creating foo.arguments) tries to read foo's LexicalEnvironment from its designated lexicalEnvironment local, but instead, finds it to be uninitialized.  This results in a null pointer access which causes a crash.

This can be resolved by having bar() instantiate a clone of the Arguments object instead, and populate its elements with values fetched directly from foo's frame.  There's no need to reference foo's LexicalEnvironment (whether present or not).
Comment 1 Radar WebKit Bug Importer 2015-01-05 13:28:00 PST
<rdar://problem/19377712>
Comment 2 Mark Lam 2015-01-05 13:36:04 PST
Created attachment 243998 [details]
the patch.

This patch is for keeping track of the work in progress.  Will finalize this fix once the remaining issues that fall out of <https://webkit.org/b/139827> become clearer.
Comment 3 Mark Lam 2015-01-08 11:50:54 PST
Created attachment 244277 [details]
patch 2: fixed a typo in the ChangeLog.
Comment 4 Geoffrey Garen 2015-01-08 16:07:19 PST
Comment on attachment 244277 [details]
patch 2: fixed a typo in the ChangeLog.

r=me
Comment 5 Geoffrey Garen 2015-01-08 16:10:11 PST
You should also add a test, if one doesn't exist, that notes the change in behavior in this case:

function a(arg0)
{
    var unused = arguments;
    arg0 = "changed";
    return function b()
    {
        var unused = arg0;
        return a.arguments[0];
    }
}

a("original")();

Before your patch, you'll get "changed", and after you'll get "original".
Comment 6 Mark Lam 2015-01-08 16:12:24 PST
(In reply to comment #5)
> You should also add a test, if one doesn't exist, that notes the change in
> behavior in this case: ...

OK, will do.
Comment 7 WebKit Commit Bot 2015-01-08 16:49:37 PST
Comment on attachment 244277 [details]
patch 2: fixed a typo in the ChangeLog.

Clearing flags on attachment: 244277

Committed r178145: <http://trac.webkit.org/changeset/178145>
Comment 8 WebKit Commit Bot 2015-01-08 16:49:41 PST
All reviewed patches have been landed.  Closing bug.
Comment 9 Mark Lam 2015-01-13 18:48:36 PST
(In reply to comment #5)
> You should also add a test, if one doesn't exist, that notes the change in
> behavior in this case:
> 
> function a(arg0)
> {
>     var unused = arguments;
>     arg0 = "changed";
>     return function b()
>     {
>         var unused = arg0;
>         return a.arguments[0];
>     }
> }
> 
> a("original")();
> 
> Before your patch, you'll get "changed", and after you'll get "original".

Turns out this use of function dot argument does not do what we think it does.  The value of a.arguments in b turns out to be null.  This is consistently so in Safari, Chrome, and Firefox.

If b() was invoked while a() is still on the stack (invoked either directly or indirectly from inside a()), then a.arguments[0] will be "changed" (also consistently so across all 3 browsers).

With WebKit ToT, something appears to be broken.  The cases that should have returned "changed" is now returning undefined.  Will investigate.
Comment 10 Geoffrey Garen 2015-01-14 11:08:48 PST
> Turns out this use of function dot argument does not do what we think it
> does.  The value of a.arguments in b turns out to be null.  This is
> consistently so in Safari, Chrome, and Firefox.

It does what we think it does -- I just wrote the test case incorrectly. Here is the correct test:

> > function a(arg0)
> > {
> >     var unused = arguments;
> >     arg0 = "changed";
> >     return (function b()
> >     {
> >         var unused = arg0;
> >         return a.arguments[0];
> >     })();
> > }
Comment 11 Mark Lam 2015-01-14 20:39:44 PST
Reopened because we've found that the previous patch is insufficient.  Will upload a follow up patch in a moment.
Comment 12 Mark Lam 2015-01-14 20:40:27 PST
Created attachment 244676 [details]
patch 2: fixed bugs from the previous patch + add needed tests.
Comment 13 Geoffrey Garen 2015-01-15 10:40:41 PST
Comment on attachment 244676 [details]
patch 2: fixed bugs from the previous patch + add needed tests.

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

r=me with comments

> Source/JavaScriptCore/runtime/Arguments.cpp:389
> +    if (isTornOff())
> +        return;

This should be an ASSERT. Only non-cloned arguments is ambiguous about when it will be torn off.

> Source/JavaScriptCore/runtime/Arguments.cpp:399
> +    ASSERT(!m_slowArgumentData.get());

No need for .get() here.

> Source/JavaScriptCore/runtime/Arguments.cpp:408
> +    if (isTornOff())
> +        return;

Ditto.

> Source/JavaScriptCore/runtime/Arguments.cpp:415
> +    ASSERT(!m_slowArgumentData.get());

Ditto.
Comment 14 Mark Lam 2015-01-15 11:21:03 PST
Thanks for the review.  The patch has been updated based on the feedback, and landed in r178517: <http://trac.webkit.org/r178517>.