Closed Bug 1227555 Opened 4 years ago Closed 4 years ago

JSObject::is() reports incorrect results for ProxyObject subclasses

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox45 --- fixed

People

(Reporter: jonco, Assigned: jonco)

Details

Attachments

(1 file)

For any ProxyObject subclasses Foo and Bar, fooInstance->is<Bar>() will return true.

This is because ProxyObject subclasses don't declare a class_ member, so the inherited ProxyObject::class_ is used instead.

I guess this is part of a wider problem for JSObject subclasses that don't declare their own class, but it's more obvious for ProxyObject subclasses because none of them declare class_ and they should all be considered different.
Here's a patch which does the following:

1) Remove ProxyObject::class_.  This breaks any use of JSObject::is() for ProxyObject and derived classes.

2) Add a specialisation of is() for ProxyObject that does not apply to derived classes.

3) Adds js::IsDerivedProxyObject() which tests both the class and the handler.

4) Adds specialisations of is() for ModuleNamespaceObject and DebugScopeObject using the above.

Hopefully this is tidier and less error-prone.
Attachment #8691408 - Flags: review?(shu)
Comment on attachment 8691408 [details] [diff] [review]
bug1227555-proxy-object-is

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

This is a fine change.

::: js/src/vm/ProxyObject.h
@@ +105,5 @@
>      void nuke(const BaseProxyHandler* handler);
>  
> +    // There is no class_ member to force specialization of JSObject::is<T>().
> +    // The implementation in JSObject is incorrect for proxies since it doesn't
> +    // take account of the handler type.

Small nit: perhaps just rename class_? Like klass_ or something? I don't see reason to put it outside of the ProxyObject class entirely. (And keep this comment, of course).
Attachment #8691408 - Flags: review?(shu) → review+
https://hg.mozilla.org/mozilla-central/rev/b61c35cebe4f
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.