785941 - Nodelist length getter seems to work even if the this object is not a nodelist, because getters on a proxy's prototype chain are always handed the proxy, not the real thisObj

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Boris Zbarsky [:bzbarsky]]

Consider this testcase

  <script name="foo">
  var bug = {};
  bug.__proto__ = document.getElementsByTagName("script");
  try {
    alert(bug.length)
  } catch(e) {
    console.log(e.message);
  }
  </script>

I believe per spec this should throw, but it alerts 1 instead.  When we land in length_getter, the "this" object is already the proxy.  I don't understand why that is; that seems broken.

Comment 1

[Cameron McCormack (:heycam)]

I agree it should throw.

Comment 2

[Cameron McCormack (:heycam)]

But I wonder if it would be more useful not to throw, but to delegate to an object of the right type in the prototype chain.  We could make the getter and setter functions of the accessor properties for IDL attributes and operations look up the prototype chain if the this object they get isn't of the right type.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Gecko used to sort of do that.  We removed it, because it slows down the DOM in the non-proxy case, significantly complicates implementation (the former is a result of the latter), and isn't required on the web in practice.  I would not be particularly interested in adding it back for most DOM objects.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Well, correction.  We _could_ make it work for this-unwrapping.  We definitely do not want to do it for argument unwrapping.

Comment 5

[Cameron McCormack (:heycam)]

I see.  Well, it's probably not worth doing if it would be inconsistent like that.

Comment 6

[Cameron McCormack (:heycam)]

(By which I mean, you can get properties from them but can't pass them to other functions.)

Comment 7

[Peter Van der Beken [:peterv]]

proxy_LookupGeneric uses has(...) to check whether the proxy has the property, and if has(...) returns true then it sets objp to the proxy, even if the property was found on the proto.

Comment 8

[Peter Van der Beken [:peterv]]

(In reply to Peter Van der Beken [:peterv] from comment #7)
> even if the property was found on the proto.

Actually, that's ok but the problem is that we then call the getter with *objp as the this (in js_GetPropertyHelperInline).

Comment 9

[Peter Van der Beken [:peterv]]

In particular, we end up in this block in js_GetPropertyHelperInline:

    if (!obj2->isNative()) {
        return obj2->isProxy()
               ? Proxy::get(cx, obj2, receiver, id, vp)
               : JSObject::getGeneric(cx, obj2, obj2, id, vp);
    }

Comment 10

[Boris Zbarsky [:bzbarsky]]

That sounds like a JS engine bug to me, then...  We should be getting the original object in our getter, not the proxy.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Jason, you've been working on proxy correctness stuff, right?

Comment 12

[Tom S [:evilpie]]

Could GetPropertyOnPrototype be the problem? It looks like that just drops the receiver on the floor.

Comment 13

[Tom S [:evilpie]]

Attachment: getter-on-receiver

Yep, this makes it throw.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Aha!  See bug 1118360.

I do think the issue used to be higher upstream in the JS engine, but sounds like that got fixed in the interim.

Comment 15

[Tom S [:evilpie]]

(In reply to Boris Zbarsky [:bz] from comment #14)
> Aha!  See bug 1118360.
> 
> I do think the issue used to be higher upstream in the JS engine, but sounds
> like that got fixed in the interim.
Agreed.

Comment 16

[Boris Zbarsky [:bzbarsky]]

This got fixed by passing the actual receiver in GetPropertyOnPrototype.  That happened in bug 603201.

Need to add a test.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Add a test for

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8735455 [details] [diff] [review]
Add a test for

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

::: testing/web-platform/tests/dom/collections/HTMLCollection-as-proto-length-get-throws.html
@@ +1,3 @@
> +<!doctype html>
> +<meta charset=utf-8>
> +<title></title>

A title would be nice.

Comment 19

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2832f3510896

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2832f3510896