Last Comment Bug 771026 - Kill JSProperty and replace with rooted shapes
: Kill JSProperty and replace with rooted shapes
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
-- normal (vote)
: mozilla16
Assigned To: Bill McCloskey (:billm)
: Jason Orendorff [:jorendorff]
Depends on:
  Show dependency treegraph
Reported: 2012-07-04 17:01 PDT by Bill McCloskey (:billm)
Modified: 2012-07-05 17:19 PDT (History)
3 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

patch (97.00 KB, patch)
2012-07-04 17:01 PDT, Bill McCloskey (:billm)
bhackett1024: review+
Details | Diff | Splinter Review

Description User image Bill McCloskey (:billm) 2012-07-04 17:01:26 PDT
Created attachment 639201 [details] [diff] [review]

As far as I can tell, JSProperty always holds a Shape or the values 0 or 1. When it holds 1, we're always dealing with non-native objects where we never look at the shape anyway. So we could replace 1 with anything as long as it's not 0, which denotes "not found."

This patch converts every instance of JSProperty to Shape. And it roots the Shape and passes it through handles. In the places where we used to use (JSProperty*)1, I instead just stuck an arbitrary Shape. I think this is okay.
Comment 1 User image Brian Hackett (:bhackett) 2012-07-04 17:18:01 PDT
Comment on attachment 639201 [details] [diff] [review]

Review of attachment 639201 [details] [diff] [review]:

Double yay!  We should have holiday traditions of shoveling obsolete code out of the engine.

::: js/src/jsarray.cpp
@@ +728,5 @@
>      if (!obj->isDenseArray())
>          return baseops::LookupElement(cx, obj, index, objp, propp);
>      if (IsDenseArrayIndex(obj, index)) {
> +        propp.set(obj->lastProperty()); /* just need to set to something non-null */

This and other non-native lookups should use some unwieldy common function like MarkNonNativeLookupSuccess, with a comment explaining what's going on.
Comment 2 User image Bill McCloskey (:billm) 2012-07-04 19:35:35 PDT
Comment 3 User image Bill McCloskey (:billm) 2012-07-04 22:26:01 PDT
Had to back this out.
Comment 4 User image Stefan Fleiter (:sfleiter) 2012-07-05 09:28:00 PDT
Shouldn't this be reopened?
Comment 6 User image Ryan VanderMeulen [:RyanVM] 2012-07-05 17:19:17 PDT

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