Closed Bug 339649 Opened 14 years ago Closed 13 years ago
incorrect script-global used in ns
XBLPrototype Handler::Execute Handler
nsXBLPrototypeHandler::ExecuteHandler (nsXBLPrototypeHandler.cpp), in revision 1.110, line 465 has the comment 'XXX: Don't use the global object!' - which it then proceeds to do! This needs investigation and bz asked me to open a bug to track it. Note that after the DOM_AGNOSTIC3_BRANCH lands, the surrounding code will have changed and the comment changed to 'XXX - apparently we should not be using the global as the scope - what should we use?' The comment was originally introduced in revision 1.27 (http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&root=/cvsroot&subdir=mozilla/content/xbl/src&command=DIFF_FRAMESET&file=nsXBLPrototypeHandler.cpp&rev2=1.27&rev1=1.26 ) but that diff doesn't leave me any wiser as to what should be used. bonsai shows jst made the checkin back in 2001, but credits many others (including myself!) for working on the patch, so I doubt he recalls - but adding to CC anyway.
So what _is_ this code doing, exactly? It's trying to get a JSObject for aReceiver, right? And it's using for the scope the focused window if there is one, then the window that aReceiver belongs to (but the owner doc, not the current doc). And then it uses the outer window from the corresponding JS context as the scope for WrapNative... I guess this only works because WrapNative ignores the passed-in scope for nodes...
See also bug 347480 for another potentially incorrect scope.
Is this as simple as removing the code that grabs the focused window and simply always use the window from the node? (btw, using GetOwnerDoc() seems like the right thing for that purpose). If so we might as well fix this since it seems scary. Tentatively marking as blocking.
Flags: blocking1.9? → blocking1.9+
Assignee: general → jonas
ccing some folks who might know what this code is trying to do....
(In reply to comment #1) >So what _is_ this code doing, exactly? It's trying to get a JSObject for >aReceiver, right? And it's using for the scope the focused window if there is >one, then the window that aReceiver belongs to (but the owner doc, not the >current doc). And then it uses the outer window from the corresponding JS >context as the scope for WrapNative... The focused window is a red herring. XBL never binds directly to a window except for <key>s which happen to have already been taken care of above. INHO the code is simply trying to execute the code as if it was an inline event handler on the bound element e.g. <radio onclick="if (event.button == 0 && !this.disabled) this.radioGroup.selectedItem = this;"/> [the XPFE version doesn't seem to have been converted to <handler event="click" button="0">]
So would it be safe to simply remove the focused-window stuff then?
(In reply to comment #5) >the XPFE version doesn't seem to have been converted Whoops, I was looking at an old version :-[ (In reply to comment #6) >So would it be safe to simply remove the focused-window stuff then? Although possibly irrelevant to this bug, it looks safe to me.
Target Milestone: --- → mozilla1.9alpha6
Comment on attachment 269266 [details] [diff] [review] remove focusedWin oops, wrong patch
As far as I see winRoot is used only when handling xul keys, and those are handled in the method already before. But I need to test this with some editor code too, so not asking review yet.
Attachment #269269 - Attachment is obsolete: true
punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.
Target Milestone: mozilla1.9alpha6 → mozilla1.9beta1
(In reply to comment #5) > XBL never binds directly to a window > except for <key>s which happen to have already been taken care of above. > Platform bindings are bound to chrome handlers, which for example window root is. Currently platform bindings seem to use command handlers, but one might add some JS code there too. Which context should be used then? I'd say the context should be taken from the mWindow of nsWindowRoot. That would make things consistent; trying to get the "nearest" DOMWindow. I'll make a patch to test whether that breaks anything.
So for platform bindings use the context from the topmost window.
(In reply to comment #12) >Currently platform bindings seem to use command handlers, but one might add >some JS code there too. One of my less successful patches added JS to the input binding... I seem to remember that several reasons were given as to why that was a bad idea.
(In reply to comment #14) > I seem to remember that several reasons were given as to why that was > a bad idea. Do you have the bug number or do you remember why it was bad idea (well, with the current "use focused window context" it might be a bad idea)
Attachment #270370 - Flags: review?(jonas) → review+
(In reply to comment #14) >One of my less successful patches added JS to the input binding... I seem to >remember that several reasons were given as to why that was a bad idea. Apparently at the time there were JS proto chain issues, however that might have been because I used an <implementation>.
Attachment #270370 - Flags: superreview?(jst) → superreview+
Comment on attachment 270370 [details] [diff] [review] proposed patch Checked in this "focused win removal patch". Is there anything else we'd like to fox here? I'll (re-)compare this code to ELM's listener registration...
I just backed this out as this has broken multiple tinderboxen and smaug was not available on IRC to fix that. Pike said in #developers he tried to contact him but had no chance and no idea himself how to fix it.
Oops, sorry. And thank Robert. Looks like missing =0 in nsPIWindowRoot
I re-read ELM and XBLProtoHandler and now XBL (script) event handlers should always work like content's event attributes <element onevent="something()">. But the XXX is about scope. Why should XBL use different scope than event attributes?
I again reread all the code and checked how BindCompiledEventHandler is used, and couldn't see anything wrong with the current code in XBL. So removing XXX now that the boundGlobal has always some reasonable value.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.