Bug 136961

Summary: JSC fails to check object property in prototype sometimes
Product: WebKit Reporter: Igor Kiselev <igor>
Component: JavaScriptCoreAssignee: Matthew Mirman <mmirman>
Status: RESOLVED FIXED    
Severity: Normal CC: commit-queue, fpizlo, ggaren, mmirman
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Test case for bug reproducing
none
Fixes inline cache fast path accessing nonexistent getters.
none
Fixes inline cache fast path accessing nonexistent getters.
none
Fixes inline cache fast path accessing nonexistent getters. none

Description Igor Kiselev 2014-09-19 14:27:04 PDT
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']();
}
Comment 1 Geoffrey Garen 2014-09-22 13:34:12 PDT
<rdar://problem/18416918>
Comment 2 Matthew Mirman 2014-11-20 17:07:49 PST
Created attachment 242005 [details]
Fixes inline cache fast path accessing nonexistent getters.
Comment 3 WebKit Commit Bot 2014-11-20 17:09:45 PST
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.
Comment 4 Filip Pizlo 2014-11-20 17:10:42 PST
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?
Comment 5 Geoffrey Garen 2014-11-20 17:15:37 PST
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.
Comment 6 Geoffrey Garen 2014-11-20 17:22:47 PST
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.
Comment 7 Matthew Mirman 2014-11-21 15:24:30 PST
(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.
Comment 8 Matthew Mirman 2014-11-21 15:38:07 PST
Created attachment 242086 [details]
Fixes inline cache fast path accessing nonexistent getters.

Fixes get not being a reference to getPropertySlot.
Comment 9 Andreas Kling 2014-11-21 22:44:24 PST
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);
Comment 10 Andreas Kling 2014-11-21 22:44:25 PST
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);
Comment 11 Matthew Mirman 2014-12-01 11:42:37 PST
Created attachment 242324 [details]
Fixes inline cache fast path accessing nonexistent getters.

Responds to kling's comments.
Comment 12 WebKit Commit Bot 2014-12-02 11:27:10 PST
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>
Comment 13 WebKit Commit Bot 2014-12-02 11:27:14 PST
All reviewed patches have been landed.  Closing bug.