Closed Bug 785941 Opened 12 years ago Closed 8 years ago

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

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
firefox48 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Depends on 1 open bug, Blocks 1 open bug, )

Details

(Whiteboard: [js:p2])

Attachments

(2 files)

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.
I agree it should throw.
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.
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.
Well, correction.  We _could_ make it work for this-unwrapping.  We definitely do not want to do it for argument unwrapping.
I see.  Well, it's probably not worth doing if it would be inconsistent like that.
(By which I mean, you can get properties from them but can't pass them to other functions.)
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.
(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).
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);
    }
That sounds like a JS engine bug to me, then...  We should be getting the original object in our getter, not the proxy.
Assignee: nobody → general
Component: DOM → JavaScript Engine
Summary: Nodelist length getter seems to work even if the this object is not a nodelist → Nodelist length getter seems to work even if the this object is not a nodelist, because getters on a proxy
Summary: Nodelist length getter seems to work even if the this object is not a nodelist, because getters on a proxy → 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
Whiteboard: [js:p2]
Assignee: general → nobody
Flags: needinfo?(efaustbmo)
Jason, you've been working on proxy correctness stuff, right?
Flags: needinfo?(jorendorff)
Could GetPropertyOnPrototype be the problem? It looks like that just drops the receiver on the floor.
Yep, this makes it throw.
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.
Depends on: 1118360
Flags: needinfo?(jorendorff)
Flags: needinfo?(efaustbmo)
(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.
Component: JavaScript Engine → DOM
This got fixed by passing the actual receiver in GetPropertyOnPrototype.  That happened in bug 603201.

Need to add a test.
Depends on: 603201
Flags: in-testsuite?
Attached patch Add a test forSplinter Review
Attachment #8735455 - Flags: review?(Ms2ger)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
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.
Attachment #8735455 - Flags: review?(Ms2ger) → review+
https://hg.mozilla.org/mozilla-central/rev/2832f3510896
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.