Last Comment Bug 645464 - js::ClassMethodIsNative is so wrong
: js::ClassMethodIsNative is so wrong
Status: RESOLVED FIXED
fixed-in-tracemonkey
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla7
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
javascript: var s = new String(); s.v...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-27 01:57 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-06-20 17:03 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch and tests (17.64 KB, patch)
2011-03-29 11:40 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
luke: review+
Details | Diff | Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-03-27 01:57:04 PDT
It probes for the specified function directly in the object, and if it doesn't find the property *or* the property doesn't contain that function, it probes the prototype.  It should only keep trying to optimize by probing the prototype if the property doesn't exist.

I'm aghast that I never managed to find this writing any of the stepwise tests I've written that probed for proper implementation of ToString or ToNumber.  At least this test found it for us so we know to fix it:

http://hg.ecmascript.org/tests/test262/file/a7a912229e2d/test/suite/sputnik_converted/15_Native/15.5_String_Objects/15.5.5_Properties_of_String_Instances/S15.5.5.1_A5.js
Comment 1 Luke Wagner [:luke] 2011-03-27 17:59:56 PDT
yikes, good catch
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-03-29 11:40:03 PDT
Created attachment 522746 [details] [diff] [review]
Patch and tests

Note in particular the hasDefaultGetter->hasDefaultGetterOrIsMethod change, and note that this depends on the patch for bug 640503 in order for properties on String objects to not have str_getProperty as their getter op.
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-10 10:36:47 PDT
Comment on attachment 522746 [details] [diff] [review]
Patch and tests

I seem to be doing a great job getting reviews in bug 660438, bug 646129, and this bug in exactly the opposite order of a dependency topsort of them.  Let's get this one done so I can actually land any of them, mmkay?  :-)
Comment 4 Luke Wagner [:luke] 2011-06-10 11:54:35 PDT
Comment on attachment 522746 [details] [diff] [review]
Patch and tests

Review of attachment 522746 [details] [diff] [review]:
-----------------------------------------------------------------

Wow, good find!

::: js/src/jsfun.h
@@ +225,1 @@
>      }

Preempted!

::: js/src/jsobj.cpp
@@ +5872,3 @@
>  {
>      const Shape *shape = obj->nativeLookup(methodid);
> +    if (!shape || !shape->hasDefaultGetterOrIsMethod() || !obj->containsSlot(shape->slot))

I would feel better if the !hasDefaultGetterOrIsMethod() disjunct was stated in a positive form since for the reader who doesn't know the complete set of cases, its hard to take the set difference.

::: js/src/jsobj.h
@@ +1841,3 @@
>   */
> +extern bool
> +HasDataProperty(JSObject *obj, jsid methodid, js::Value *vp);

Is this method equivalent to:
  [[HasProperty]](methodid) and IsDataDescriptor([[GetProperty]](methodid))
? Could you comment if we are or, if not, why we aren't?

::: js/src/jsobjinlines.h
@@ +1246,5 @@
> +ValueIsNative(const js::Value &v, Native native)
> +{
> +    JSObject *funobj;
> +    return IsFunctionObject(v, &funobj) && funobj->getFunctionPrivate()->maybeNative() == native;
> +}

Almost preempted!  Could you add this as a IsNativeFunction() overload in jsfun.h below the other two?
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-06-13 18:18:06 PDT
http://hg.mozilla.org/tracemonkey/rev/ccd771857394
Comment 6 Chris Leary [:cdleary] (not checking bugmail) 2011-06-20 17:03:36 PDT
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/ccd771857394

Note You need to log in before you can comment on or make changes to this bug.