Closed Bug 717842 Opened 12 years ago Closed 12 years ago

New nodelist bindings break cross-window instanceof

Categories

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

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
Tracking Status
firefox9 --- unaffected
firefox10 - ---
firefox11 - ---
firefox12 - ---

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

Details

(Keywords: regression)

Attachments

(1 file)

There's a hasInstance hook that's supposed to make it work, but it checks the wrong object for the magic jsclass.  Ran into it while trying to document sInterfaceClass at Jason's request.

Patch coming up.  I think we should probably fix this on aurora and beta too...
This fixes the obvious bug, but I don't understand why we need the loop over prototypes.  When will that return true for cases when we have instanceIsProxy true but the |obj| doesn't have sInterfaceClass?  And aren't those cases broken for the cross-window case too?

Also, I don't understand what ListBase<LC>::hasInstance is doing and why....  It seems fishy to me.
Attachment #588287 - Flags: review?(peterv)
Web IDL says:

  Every platform object is associated with a global environment, just as the initial
  objects are. It is the responsibility of specifications using Web IDL to state
  which global environment (or, by proxy, which global object) each platform object
  is associated with.

I doubt any spec does this, though.

I would expect the subframe.document.getElementsByTagName() call in your test to return an object that is associated with the subframe's global environment, have subframe.HTMLCollection.prototype in its prototype chain, and thus would evaluate `list2 instanceof HTMLCollection` as false, since Web IDL doesn't have any special [[HasInstance]] behaviour.
If that's all we want, then we don't need the special hasInstance hook on the JSClass there.  We may not even need the JSClass.

That said, the behavior this patch restores it the pre-new-binding behavior.  It's the one all other DOM objects in Gecko have, I believe.  So it's possible the goal was to not change behavior when switching to the new bindings, which does not preclude changing it as needed, of course.

I did check, and none of Opera, Safari, Chrome return true for cross-window instanceof.
(In reply to Cameron McCormack (:heycam) from comment #2)
> since
> Web IDL doesn't have any special [[HasInstance]] behaviour.

It used to though (http://www.w3.org/TR/2010/WD-WebIDL-20101021/#es-interface-hasinstance), that's what our implementation followed (modulo the prototype/obj mixup). Since it seems gone from recent drafts I'm fine with redoing our implementation.
If we want to do that for nodelists, then it seems like we should be able to nix all the instanceof-related magic, right?  Both the proxy hook and the JSClass bit?
BTW, I'm a bit confused why |list instanceof HTMLCollection| would ever be true if WebIDL doesn't specify [[HasInstance]] behaviour for its interface objects.
(In reply to Peter Van der Beken [:peterv] from comment #6)
> BTW, I'm a bit confused why |list instanceof HTMLCollection| would ever be
> true if WebIDL doesn't specify [[HasInstance]] behaviour for its interface
> objects.

"Interface objects are always function objects." is what makes it work. Seems the drafts have changed quite a bit in this regard.
By the way, bug 14869 : "getElementsByTagName() returns HTMLCollection instead of NodeList", so
----
var list = document.getElementsByTagName("*");
ok(list instanceof HTMLCollection, "List should be an HTMLCollection");
----
It should to the extent that Firefox is not conforming to the spec.
(In reply to Cameron McCormack (:heycam) from comment #2)
> Web IDL says:
> 
>   Every platform object is associated with a global environment, just as the
> initial
>   objects are. It is the responsibility of specifications using Web IDL to
> state
>   which global environment (or, by proxy, which global object) each platform
> object
>   is associated with.
> 
> I doubt any spec does this, though.
Maybe WebIDL could say that each frame gets a fresh DOM environment, no?
Are there use cases where it can be useful for some specs to share script-facing objects across different global environments?
> It should to the extent that Firefox is not conforming to the spec.

The test is meant to catch changes from the behavior we have right now, yes.  That's the idea of regression tests.
Regression in a new feature, tracking for FF10.
(In reply to David Bruant from comment #9)
> Maybe WebIDL could say that each frame gets a fresh DOM environment, no?
> Are there use cases where it can be useful for some specs to share
> script-facing objects across different global environments?

I don't think Web IDL should be talking about "frames".  It could maybe have a hook that HTML could use to do this.  But the tricky thing is describing exactly which objects returned from operations and attributes are associated with the current window and which things aren't.  For example:

  <!DOCTYPE html>
  <iframe></iframe>
  <script>
  var f = document.getElementsByTagName("iframe")[0];
  var w = f.contentWindow;
  var e = document.createElement("span");  // belongs to this window
  w.document.body.appendChild(e);
  w.document.body.lastChild;  // belongs to this window, not f
  </script>

So we can't just say that all IDL attributes return objects that belong to the global environment that its interface comes from.
>  w.document.body.lastChild;  // belongs to this window, not f

I believe at that point it should belong to f.  That should generally happen on adoptNode calls.
OK.  I tested with instanceof to see if the node actually still belonged to this window, but I forgot instanceof will return true regardless for us currently. :)

But this general pattern is what I mean to point out; operations/attributes for objects in frame B that return objects created in frame A.
(In reply to Cameron McCormack (:heycam) from comment #12)
> (In reply to David Bruant from comment #9)
> > Maybe WebIDL could say that each frame gets a fresh DOM environment, no?
> > Are there use cases where it can be useful for some specs to share
> > script-facing objects across different global environments?
> 
> I don't think Web IDL should be talking about "frames".
I'm not expert on the HTML5 terminology. Would "browsing context" be more accurate for this case?

> It could maybe have
> a hook that HTML could use to do this.  But the tricky thing is describing
> exactly which objects returned from operations and attributes are associated
> with the current window and which things aren't.  For example:
> 
>   <!DOCTYPE html>
>   <iframe></iframe>
>   <script>
>   var f = document.getElementsByTagName("iframe")[0];
>   var w = f.contentWindow;
>   var e = document.createElement("span");  // belongs to this window
>   w.document.body.appendChild(e);
>   w.document.body.lastChild;  // belongs to this window, not f
>   </script>
> 
> So we can't just say that all IDL attributes return objects that belong to
> the global environment that its interface comes from.
My language was confusing, sorry. You're talking about belonging, but that's not what I meant (though I wrote "gets").

The way I see it, objects do not belong to anyone. However, each "browsing context" or "scripting context" is initialized with a fresh set of built-ins. If 2 scripting contexts can communicate synchronously, then they can access each others built-in, but it's not really that they are /sharing/ what /belongs/ to any of them. It's more that the sharing is de facto and so the notion of belonging is somewhat implicit.

Regarding nodes in DOM tree, the "belonging" is not at the language level, but at the " DOM library" level. The DOM "library" defines what a node, an element and a document are; it defines that some nodes belong to some documents, but WebIDL has no role to play here.
[Triage Comment]
No need to track this, we shipped 10 with this and it doesn't appear to consider significant user pain so letting it ride
Wontfixing this, per discussion.
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → WONTFIX
Whiteboard: [need review]
Attachment #588287 - Flags: review?(peterv)
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: