Closed Bug 1331668 Opened 3 years ago Closed 3 years ago

Replace use of tagged shape pointers with an object that encapsulates the result of a property lookup

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla53
Tracking Status
firefox53 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(1 file, 1 obsolete file)

At the moment property lookups return a special shape pointer to indicate non native properties or dense array elements (see MarkNonNativePropertyFound, IsImplicitNonNativeProperty and the like).

We should replace this with an object that encapsulates the result of the lookup and doesn't allow bogus shape pointers to leak out (which the GC then has to check for specially).
Blocks: 1205909
Attached patch bug1331668-property-result (obsolete) — Splinter Review
Patch to introduce PropertyResult and use this as the result of looking up a property.  Hopefully this is along the lines you were expecting.

This turned out to involve quite a lot of changes and I'm not sure about the correctness of all of them, especially the JIT ones.  I've put a bunch of XXX comments in the patch.  Can you check whether these are right?
Attachment #8827530 - Flags: feedback?(jdemooij)
Comment on attachment 8827530 [details] [diff] [review]
bug1331668-property-result

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

So much nicer! \o/

::: js/public/Class.h
@@ +256,5 @@
> +    };
> +
> +    static const uintptr_t NotFound = 0;
> +    static const uintptr_t NonNativeProperty = 1;
> +    static const uintptr_t DenseOrTypedArrayElement = 1;

It makes sense to match the current code for now, but please file a follow-up bug to use different values for NonNativeProperty and DenseOrTypedArrayElement. We would have to audit all callers of these accessors, but I would be happy to do that.

@@ +261,5 @@
> +
> +  public:
> +    PropertyResult() : bits_(NotFound) {}
> +
> +    operator bool() const {

Nit: explicit operator bool (static analysis build checks this I think).

@@ +291,5 @@
> +        MOZ_ASSERT(isNativeProperty());
> +        return shape_;
> +    }
> +
> +    void setNone() {

Nit: setNotFound, for consistency with NotFound and isFound?

::: js/src/jit/BaselineIC.cpp
@@ +2595,5 @@
>          return true;
>  
> +    RootedShape shape(cx);
> +    if (obj->isNative()) {
> +        // XXX: We seem to assume we've got a native property below.

Yes it should be, because we check obj != holder before this.

::: js/src/jit/CacheIR.cpp
@@ +278,5 @@
>          holder.set(&baseHolder->as<NativeObject>());
>      }
>  
> +    // XXX: This is a string or symbol id found on a native object so I guess
> +    // this is true, right?

Yes.

::: js/src/jit/IonCaches.cpp
@@ +1317,5 @@
>          holder.set(&baseHolder->as<NativeObject>());
>      }
>  
> +    // XXX: This is a string or symbol id found on a native object so I guess
> +    // this is true, right?

Yes. (FWIW this code will be gone next week, after the merge. It's currently disabled and we only use the CacheIR.cpp version.)

@@ +2925,5 @@
>      if (!checkObj)
>          return false;
>  
> +    PropertyResult prop;
> +    if (!LookupPropertyPure(cx, obj, id, holder.address(), &prop))

Hrm wow, this code looks broken. We pass |obj| instead of |checkObj|, so we will always fail this lookup: obj is a DOM proxy and LookupPropertyPure knows nothing about that.

Nothing to do here, I'll fix this when I convert all this code to CacheIR soon.

@@ +2932,5 @@
>      if (!holder)
>          return false;
>  
> +    // XXX: Safe because we know it's a DOM proxy, right?
> +    shape.set(prop.shape());

To be extra safe, we could change the if-statement before this to:

if (!holder || !holder->isNative())
    return false;

@@ +3351,4 @@
>          return SetPropertyIC::CanAttachNone;
>  
> +    // XXX: Is this true?
> +    MOZ_ASSERT_IF(obj->isNative() && prop, prop.isNativeProperty());

I think so yes.

::: js/src/vm/Interpreter-inl.h
@@ +88,5 @@
> +    // We check for isDenseOrTypedArrayElement even though the shape is always a
> +    // non-indexed property because proxy hooks may return a "non-native
> +    // property found" shape, which happens to be encoded in the same way as the
> +    // "dense element" shape. See PropertyResult::setNonNativeProperty.
> +    if (prop.isDenseOrTypedArrayElement())

We could change this to prop.isNonNativeProperty() and simplify the comment a bit.

@@ +195,5 @@
>          if (!GetProperty(cx, obj, obj, id, vp))
>              return false;
>      } else {
> +        // XXX: is this safe?
> +        RootedShape shape(cx, prop.shape());

I think so, due to the if above.

@@ +225,5 @@
> +    if (!prop || !pobj->isNative())
> +        return false;
> +
> +    // XXX: Is this safe?
> +    MOZ_ASSERT(prop.isNativeProperty());

Same.
Attachment #8827530 - Flags: feedback?(jdemooij) → feedback+
Thanks for all the feedback.

I had to make a bunch more changes to the jit parts because it turned our that non-native properties can leak through in a number of places.  This showed up on try but not for our own tests.
Attachment #8827530 - Attachment is obsolete: true
Attachment #8828420 - Flags: review?(jdemooij)
Comment on attachment 8828420 [details] [diff] [review]
bug1331668-property-result v2

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

LGTM.
Attachment #8828420 - Flags: review?(jdemooij) → review+
Pushed by jcoppeard@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e91f28eb9a88
Add a class to encapsulate the possible results of a property lookup r=jandem
https://hg.mozilla.org/mozilla-central/rev/e91f28eb9a88
https://hg.mozilla.org/mozilla-central/rev/cb9e0b616c22
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
You need to log in before you can comment on or make changes to this bug.