Bug 140093 – Argument object created by "Function dot arguments" should use a clone of argument values

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

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

[Radar WebKit Bug Importer]

<rdar://problem/19377712>

[Mark Lam]

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.

[Mark Lam]

Created attachment 244277 [details]
patch 2: fixed a typo in the ChangeLog.

[Geoffrey Garen]

Comment on attachment 244277 [details]
patch 2: fixed a typo in the ChangeLog.

r=me

[Geoffrey Garen]

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

[Mark Lam]

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

[WebKit Commit Bot]

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>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Mark Lam]

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

[Geoffrey Garen]

> 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];
> >     })();
> > }

[Mark Lam]

Reopened because we've found that the previous patch is insufficient.  Will upload a follow up patch in a moment.

[Mark Lam]

Created attachment 244676 [details]
patch 2: fixed bugs from the previous patch + add needed tests.

[Geoffrey Garen]

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.

[Mark Lam]

Thanks for the review.  The patch has been updated based on the feedback, and landed in r178517: <http://trac.webkit.org/r178517>.