Bound functions should use the prototype of the function being bound

[Keith Miller]

Bound functions should use the prototype of the function being bound

[Keith Miller]

Created attachment 271223 [details]
Patch

[Keith Miller]

I went with bind function's global object because I talked to Mark Miller and tc39 and he said that at some point it was likely that global objects would become visible. When they do, most likely, the expected global object for bound functions would be the bind function's global object.

[Keith Miller]

Created attachment 271228 [details]
Patch

[Build Bot]

Comment on attachment 271228 [details]
Patch

Attachment 271228 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/821776

Number of test failures exceeded the failure limit.

[Build Bot]

Created attachment 271237 [details]
Archive of layout-test-results from ews115 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews115  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Keith Miller]

Created attachment 271501 [details]
Patch

[Keith Miller]

Created attachment 271508 [details]
Patch for landing

[Keith Miller]

Whoops ran land-safely in the wrong repo set back to review

[Geoffrey Garen]

Comment on attachment 271508 [details]
Patch for landing

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

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:102
> +            && result->globalObject() == globalObject) {

Why do we need this global object check? I can see that it might sometimes be false, but why would it be wrong to use the cached bound function structure in this case?

I think you need tests for these two cases.

> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:109
> +    result = globalObject->boundFunctionStructure();

This control flow is hard to follow. I would change it to an early return:

if (!targetJSFunction)
    return globalObject->boundFunctionStructure();

This cuts down on indentation and removes some control flow below.

[Geoffrey Garen]

Comment on attachment 271508 [details]
Patch for landing

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

Let's do this for readability:

inline Structure* getBoundFunctionStructure(VM& vm, JSGlobalObject* globalObject, JSObject* targetFunction)
{
    JSObject* prototype = targetFunction->structure(vm)->storedPrototypeObject();
    JSFunction* targetJSFunction = jsDynamicCast<JSFunction*>(targetFunction);

    if (targetJSFunction) {
        Structure* structure = targetJSFunction->rareData(vm)->getBoundFunctionStructure();
        if (structure
            && structure->storedPrototype() == prototype
            && structure->globalObject() == globalObject)
            return structure;
    }

    Structure* result = globalObject->boundFunctionStructure();

    if (prototype && prototype->globalObject() == globalObject) {
        result = vm.prototypeMap.emptyStructureForPrototypeFromBaseStructure(prototype, result);
        RELEASE_ASSERT_WITH_SECURTIY_IMPLICATIONS(structure->storedPrototypeObject()->globalObject() == globalObject);
    } else
        result = Structure::create(vm, globalObject, prototype, result->typeInfo(), result->classInfo());

    if (targetJSFunction)
        targetJSFunction->rareData(vm)->setBoundFunctionStructure(vm, result);

    return result;
}

>> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:102
>> +            && result->globalObject() == globalObject) {
> 
> Why do we need this global object check? I can see that it might sometimes be false, but why would it be wrong to use the cached bound function structure in this case?
> 
> I think you need tests for these two cases.

Let's not do this. The crazy here is beyond the global object question.

>> Source/JavaScriptCore/runtime/JSBoundFunction.cpp:109
>> +    result = globalObject->boundFunctionStructure();
> 
> This control flow is hard to follow. I would change it to an early return:
> 
> if (!targetJSFunction)
>     return globalObject->boundFunctionStructure();
> 
> This cuts down on indentation and removes some control flow below.

This can't be early return because prototype might be null.

[Keith Miller]

Created attachment 271945 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 271945 [details]
Patch for landing

Clearing flags on attachment: 271945

Committed r196956: <http://trac.webkit.org/changeset/196956>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Shvayka]

*** Bug 145605 has been marked as a duplicate of this bug. ***