339649 - incorrect script-global used in nsXBLPrototypeHandler::ExecuteHandler

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Mark Hammond [:markh] [:mhammond]]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

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...

Comment 2

[Mark Hammond [:markh] [:mhammond]]

See also bug 347480 for another potentially incorrect scope.

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

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.

Comment 4

[Boris Zbarsky [:bzbarsky]]

ccing some folks who might know what this code is trying to do....

Comment 5

[[e-mail]]

(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">]

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

So would it be safe to simply remove the focused-window stuff then?

Comment 7

[[e-mail]]

(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.

Comment 8

[Olli Pettay [:smaug][[e-mail]]]

Attachment: remove focusedWin

Comment 9

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 269266 [details] [diff] [review]
remove focusedWin

oops, wrong patch

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Attachment: remove also winroot

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.

Comment 11

[Mike Connor [:mconnor]]

punting remaining a6 bugs to b1, all of these shipped in a5, so we're at least no worse off by doing so.

Comment 12

[Olli Pettay [:smaug][[e-mail]]]

(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.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

Attachment: proposed patch

So for platform bindings use the context from the topmost window.

Comment 14

[[e-mail]]

(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.

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(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)

 

Comment 16

[[e-mail]]

(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>.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

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...

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

s/fox/fix/

Comment 19

[Robert Kaiser]

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.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Oops, sorry. And thank Robert.
Looks like missing =0 in nsPIWindowRoot

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Attachment: Remove XXX

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.