Closed Bug 339649 Opened 14 years ago Closed 13 years ago

incorrect script-global used in nsXBLPrototypeHandler::ExecuteHandler

Categories

(Core :: XBL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9alpha8

People

(Reporter: markh, Assigned: smaug)

References

Details

Attachments

(2 files, 2 obsolete files)

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.
Flags: blocking1.9?
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+
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
Blocks: 292189
Attached patch remove focusedWin (obsolete) — Splinter Review
Attachment #269266 - Flags: review?(jonas)
Comment on attachment 269266 [details] [diff] [review]
remove focusedWin

oops, wrong patch
Attachment #269266 - Attachment is obsolete: true
Attachment #269266 - Flags: review?(jonas)
Attached patch remove also winroot (obsolete) — Splinter Review
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.
Attached patch proposed patchSplinter Review
So for platform bindings use the context from the topmost window.
Assignee: jonas → Olli.Pettay
Status: NEW → ASSIGNED
Attachment #270370 - Flags: superreview?(jst)
Attachment #270370 - Flags: review?(jonas)
(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)

 

(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...
s/fox/fix/
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?
Attached patch Remove XXXSplinter Review
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.
Attachment #275390 - Flags: superreview?(jst)
Attachment #275390 - Flags: review?(jst)
Attachment #275390 - Flags: superreview?(jst)
Attachment #275390 - Flags: superreview+
Attachment #275390 - Flags: review?(jst)
Attachment #275390 - Flags: review+
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
You need to log in before you can comment on or make changes to this bug.