Bug 136961 – JSC fails to check object property in prototype sometimes

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

[Igor Kiselev]

Created attachment 238389 [details]
Test case for bug reproducing

Under certain conditions JSC fails to look in prototype chain. I've attached test case to reproduce such behavior. It uses JSIL libraries, but example is pretty simple:

function clone(kvp){
  return kvp.MemberwiseClone();
}

function runMain() {
  var kvp = JSIL.CreateInstanceOfType(System.Collections.Generic.KeyValuePair$b2.Of($jsilcore.System.Int32, $jsilcore.System.String).__Type__);
  for (var i=0; i<40; i++) {
    clone(kvp);
  }

  kvp = JSIL.CreateInstanceOfType(System.Collections.Generic.KeyValuePair$b2.Of($jsilcore.System.Int32, $jsilcore.System.Int32).__Type__)
  for (var i=0; i<2; i++) {
    clone(kvp);
  }
}

JSIL calls runMain(), that call clone function 42 times. On last call, kvp.MemberwiseClone call fails with: "'undefined' is not a function". Nothing has been changed in object on which it was called between 41st an 42nd call. Note: previous 40 calls was on another object. If we lower first cycle upper bound from 40 to lower value, problem will be not reproducible. until recent changes in Webkit (before Safari 7.1/8.0 release) problem was reproducible even with 8 as upper bound in first cycle. 

Problem is also not reproducible if clone function will be rewritten in next form:
function clone(kvp){
  return kvp['MemberwiseClone']();
}

[Geoffrey Garen]

<rdar://problem/18416918>

[Matthew Mirman]

Created attachment 242005 [details]
Fixes inline cache fast path accessing nonexistent getters.

[WebKit Commit Bot]

Attachment 242005 [details] did not pass style-queue:


ERROR: Source/JavaScriptCore/runtime/JSCJSValue.h:266:  The parameter name "propertyName" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 1 in 5 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Filip Pizlo]

Comment on attachment 242005 [details]
Fixes inline cache fast path accessing nonexistent getters.

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

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:713
> +    // If this is a primitive, we'll need to synthesize the prototype -
> +    // and if it's a string there are special properties to check first.
> +    JSObject* object;
> +    if (UNLIKELY(!isObject())) {
> +        if (isCell() && asString(*this)->getStringPropertySlot(exec, propertyName, slot))
> +            return true;
> +        object = synthesizePrototype(exec);
> +    } else
> +        object = asObject(asCell());
> +    
> +    if (object->getPropertySlot(exec, propertyName, slot))
> +        return true;

Looks exactly like JSValue::get() except for the return type.  Maybe that one should wrap this?

[Geoffrey Garen]

Comment on attachment 242005 [details]
Fixes inline cache fast path accessing nonexistent getters.

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

> Source/JavaScriptCore/jit/JITOperations.cpp:152
>      if (accessType == static_cast<AccessType>(stubInfo->accessType))

I think you can remove this line of code now. Perhaps this was a previous attempt to guard against the property type changing? In any case, it seems trivially true in the current state of the code, since we are testing something we just loaded against the place we loaded it from.

[Geoffrey Garen]

What about the other cases in which we perform a property access optimization only after doing a get()? For example, operationPutByIdStrictBuildList calls put() and then calls buildPutByIdList(), passing it the slot from prior to the property access.

[Matthew Mirman]

(In reply to comment #6)
> What about the other cases in which we perform a property access
> optimization only after doing a get()? For example,
> operationPutByIdStrictBuildList calls put() and then calls
> buildPutByIdList(), passing it the slot from prior to the property access.

If possible, I'd like to fix puts in a separate patch and keep this one small.

[Matthew Mirman]

Created attachment 242086 [details]
Fixes inline cache fast path accessing nonexistent getters.

Fixes get not being a reference to getPropertySlot.

[Andreas Kling]

Comment on attachment 242086 [details]
Fixes inline cache fast path accessing nonexistent getters.

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

Fix looks okay to me, but I'll let someone else do final review.
Some drive-by comments:

> Source/JavaScriptCore/ChangeLog:5
> +        https://bugs.webkit.org/show_bug.cgi?id=136961

Please include the radar link here as well.

> Source/JavaScriptCore/ChangeLog:23
> +        * tests/stress/badproperty.js: Added test case for bug.

badproperty.js seems like a very generic name for this test. :)

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:686
> +    bool hasResult = getPropertySlot(exec, propertyName, slot);
> +    return hasResult ? slot.getValue(exec, propertyName) : jsUndefined();

I don't think the local variable here adds anything. My suggestion:

if (getPropertySlot(exec, propertyName, slot))
    return slot.getValue(exec, propertyName);
return jsUndefined();

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:703
>      if (object->getPropertySlot(exec, propertyName, slot))
> -        return slot.getValue(exec, propertyName);
> -    return jsUndefined();
> +        return true;
> +    return false;

This would look nicer:
return object->getPropertySlot(exec, propertyName, slot);

[Andreas Kling]

Comment on attachment 242086 [details]
Fixes inline cache fast path accessing nonexistent getters.

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

Fix looks okay to me, but I'll let someone else do final review.
Some drive-by comments:

> Source/JavaScriptCore/ChangeLog:5
> +        https://bugs.webkit.org/show_bug.cgi?id=136961

Please include the radar link here as well.

> Source/JavaScriptCore/ChangeLog:23
> +        * tests/stress/badproperty.js: Added test case for bug.

badproperty.js seems like a very generic name for this test. :)

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:686
> +    bool hasResult = getPropertySlot(exec, propertyName, slot);
> +    return hasResult ? slot.getValue(exec, propertyName) : jsUndefined();

I don't think the local variable here adds anything. My suggestion:

if (getPropertySlot(exec, propertyName, slot))
    return slot.getValue(exec, propertyName);
return jsUndefined();

> Source/JavaScriptCore/runtime/JSCJSValueInlines.h:703
>      if (object->getPropertySlot(exec, propertyName, slot))
> -        return slot.getValue(exec, propertyName);
> -    return jsUndefined();
> +        return true;
> +    return false;

This would look nicer:
return object->getPropertySlot(exec, propertyName, slot);

[Matthew Mirman]

Created attachment 242324 [details]
Fixes inline cache fast path accessing nonexistent getters.

Responds to kling's comments.

[WebKit Commit Bot]

Comment on attachment 242324 [details]
Fixes inline cache fast path accessing nonexistent getters.

Clearing flags on attachment: 242324

Committed r176676: <http://trac.webkit.org/changeset/176676>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.