552560 - Remove __parent__

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

It exposes a somewhat obscure implementation-internal value, it induces special-case logic in the emitter, and wrappers possibly have to be specifically wary of it.  It's also not so great for secure-JS variants like fbjs and cajmumble.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Proto-patch

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Patch

KILLPARENT^H^H^H^H^H^H^H^H^H^HEr, I mean, this patch tests well, doesn't seem to break anything.

Comment 3

[Brendan Eich [:brendan]]

Nice work, Oedipus! :-P

The ratio of boilerplate to substance in nsDOMWindowUtils::GetParent makes me sad.

/be

Comment 4

[nanto_vi (TOYAMA Nao)]

I use __parent__ in a JavaScript module [1].  Could you move getParent() from nsIDOMWindowUtils to Components.utils so that it can be used in JS modules, JS components or xpcshell?

[1] http://github.com/hatena/hatena-bookmark-xul/blob/master/resources/modules/10-event.jsm#L64
In this code, I use __parent__ to get the window where a listener object is created; and to remove the listener when the window is to unload.

Comment 5

[Jeff Walden [:Waldo]]

The entire concept of every object/function having a parent is problematic.  It's very easy to get parentage wrong when constructing objects/functions, in ways that lead to security vulnerabilities.  The only reason I added functions for accessing it is so testing code can verify assertions about it.  I think it unlikely we'll add anything beyond what's already in the patch, or provide more publicly visible access to the removed functionality.

Your use case doesn't require parent information, though, just global object information.  I think we might be willing to expose JS_GetGlobalForObject on Components.utils or somewhere else, it being less problematic and obscure than parentage, or so I understand.  Thoughts from other SpiderMonkey hackers appreciated on this idea here, will file new bug if people think this sounds reasonable.  (That said, you're aware that walking up the JS stack like that is an easy way to slow SpiderMonkey down, right?  It throws you out of the JIT at the moment and back into the interpreter.)

That said, why do you need this information anyway?  My understanding has always been that manual event listener addition/removal like this is only necessary in old browsers like IE7 (or 6?) and Firefox 2 (or 1.x?), and that it's a waste of time elsewhere.

Comment 6

[Brendan Eich [:brendan]]

To borrow from the Emperor in "Amadeus", too many words :-P.

I think Nanto has a point, independent of his use-case's particulars -- I wish I had seen it sooner. Components.utils is better than nsWindowUtils.

/be

Comment 7

[Igor Bukanov]

(In reply to comment #6)
> To borrow from the Emperor in "Amadeus", too many words :-P.
> 
> I think Nanto has a point, independent of his use-case's particulars -- I wish
> I had seen it sooner. Components.utils is better than nsWindowUtils.

The sample code applies __parent__ to a function or a global object. So perhaps we should just provide access to __parent__ only for functions, global objects and DOM nodes that could also present on the scope chain for event listeners?

Comment 8

[nanto_vi (TOYAMA Nao)]

(In reply to comment #5)
> Your use case doesn't require parent information, though, just global object
> information.  I think we might be willing to expose JS_GetGlobalForObject on
> Components.utils or somewhere else, it being less problematic and obscure than
> parentage, or so I understand.

You're right.  What I really want is a way to get the global object from an object.  It's better if we can get global directly.

> (That said, you're aware that walking up the JS stack like that is
> an easy way to slow SpiderMonkey down, right?  It throws you out of the JIT at
> the moment and back into the interpreter.)

The code referred to in comment 4 is an older version.  I'm writing newer version of that but it's not yet public.  In newer version, I try to get global from a listener object, not from a function in which the listener is added, so arguments.callee.caller is no longer used.

> That said, why do you need this information anyway?  My understanding has
> always been that manual event listener addition/removal like this is only
> necessary in old browsers like IE7 (or 6?) and Firefox 2 (or 1.x?), and that
> it's a waste of time elsewhere.

It's true for DOM EventTarget objects and its listeners, but I implemented my own event model that allows pure JavaScript object to behave as an EventTarget-like object.  Some EventTarget-like objcts live as long as Mozilla application does though some listener objects for the EventTarge-like are only useful while a window, which the listeners belong, lives.  Manual listener removal is required to prevent such listeners from being invoked after the window is closed.  If EventTarget-like can know the global for a listener and then remove the listener on the global's unload, those who want to add listeners would get no worry about removing the listener.

Comment 9

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Would 
  obj.eval.call(obj, "this");
work, now that indirect eval is global eval?  I haven't tested, I confess.

Comment 10

[Jeff Walden [:Waldo]]

eval is no longer a property of Object.prototype, only of the global object.  Or do I misunderstand your idea?

Comment 11

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Nope, I forgot that we'd fixed that too.

Comment 12

[Blake Kaplan (:mrbkap) (inactive)]

One way to get an object's global object was |obj.valueOf.call(null)|. I think jorendorff has a patch though to make that return the caller's global instead valueOf's global.

Comment 13

[Brendan Eich [:brendan]]

(In reply to comment #12)
> One way to get an object's global object was |obj.valueOf.call(null)|. I think
> jorendorff has a patch though to make that return the caller's global instead
> valueOf's global.

What bug is that?

Why is caller's global the right answer?

/be

Comment 14

[Jeff Walden [:Waldo]]

It does kind of seem to me like valueOf should return "its" own global, actually, contorted tho the idea is to reason through, and ugly to boot.

Comment 15

[Blake Kaplan (:mrbkap) (inactive)]

From reading jorendorff's patch over in bug 556277, it appears I misread a comment he made somewhere... never mind me!

Comment 16

[Jeff Walden [:Waldo]]

Attachment: Followup test fixes since parent() is shell-only, not in browser

I ran jstests but not browser jstests, so missed these.  If you follow the bug reports in, one requires a parent-alike and a with, but since neither are available in unprivileged script it's easiest to just remove the test entirely.  Another is an assertion typo that probably (bug is unclear) depended on access of a "special" property, but parent() isn't, so it's useless too.  A third relies on Block objects not being censored, but we've done that for ages.  The fourth, I just if-guarded the parent checks -- and because those were comparing parents of distinct objects, a checkParent(obj, parent, msg) function wouldn't work anyway, only a checkParents(obj1, obj2, msg) -- but it seemed like overkill for only a single function, so not worth it.

Comment 17

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/60f821e679cd
http://hg.mozilla.org/tracemonkey/rev/daf02e36d328
http://hg.mozilla.org/tracemonkey/rev/109d400f1a3f
http://hg.mozilla.org/tracemonkey/rev/9be9a6eb0890
http://hg.mozilla.org/tracemonkey/rev/b67b43bbfbd2

The last change effectively disabled a chrome-workers ctypes test (new since the test was written?  wasn't failing when I last ran tests), in the interest of quelling an orange to permit more careful consideration of how to fix the problem (namely, that there's no parent() in chrome workers at present -- which change seems like a bad idea for workers).  Still deciding what to do here, exactly; will mark f-i-t once I decide that issue.

Comment 18

[Jeff Walden [:Waldo]]

Attachment: Potential patch for ctypes bustage

This reenables the ctypes test and makes parent checks conditional on the presence of parent().  I haven't tested yet -- need to rebuild my tree, which will take a litle time -- but I'm pretty sure it's good, so optimistically jumping the gun on the request.

Comment 19

[[e-mail]]

Comment on attachment 443729 [details] [diff] [review]
Potential patch for ctypes bustage

Looks good.

Comment 20

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/34c406d3fb4d

for the ctypes parent-check-guarding fix -- so we're all set here now!  \o/  Onward and upward...

Comment 21

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/60f821e679cd

Comment 22

[Jeff Walden [:Waldo]]

http://whereswalden.com/2010/05/07/spidermonkey-change-du-jour-the-special-__parent__-property-has-been-removed/

I've marked it obsolete in documentation I've found, but it still needs a note in some sort of overview document.