258833 - [FIX]Bug 230816 fix changed scope chain for XBL constructors/destructors

Status: RESOLVED FIXED

(Core :: XBL, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

"this" used to be in the scope chain (since the function used to be an event
handler).

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: This should fix the scope chain

Comment 2

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 158516 [details] [diff] [review]
This should fix the scope chain

This should fix that bug with the autocomplete dropdown you were seeing in
Firefox, Brian...

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 158516 [details] [diff] [review]
This should fix the scope chain

shaver, could you sr?

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 158516 [details] [diff] [review]
This should fix the scope chain

sr=jst

Comment 5

[[e-mail]]

Is it actually a good idea for "this" to be in the scope chain?

Comment 6

[Boris Zbarsky [:bzbarsky]]

Short-term, probably -- as it is, at least two toolkit bindings (autocomplete
and toolbar) are broken....  If we feel it's acceptable to break backwards
compat here, that's fine with me; we just need to fix the bindings then.

Comment 7

[Brendan Eich [:brendan]]

Comment on attachment 158516 [details] [diff] [review]
This should fix the scope chain

I was on IRC with bz last night when we realized what broke here -- sorry I
didn't see it when sr'ing bz's patch to make constructors and destructors
anonymous functions.  I too think deep scope chains were a mistake (mine, in
Navigator 2.0), made to enable greater brevity in onclick="..." handlers.

For "XBL 2"'s JS bindings, we can change things.  For now, I think we must keep
compatibility.

/be

Comment 8

[Boris Zbarsky [:bzbarsky]]

Fixed.

Comment 9

[Stephen Walker]

*** Bug 258890 has been marked as a duplicate of this bug. ***

Comment 10

[Wladimir Palant]

Took me a while to find out that the bug I was seeing ("top is a string? what?") was caused by a magically appearing local variable, and then trace it back to this bug. This bug is three years old, can we maybe reconsider polluting the namespace? From what I can see, this "feature" is not documented anywhere.

Comment 11

[Boris Zbarsky [:bzbarsky]]

If bindings no longer rely on this, we can do whatever.  I doubt that's the case, however.

Comment 12

[Wladimir Palant]

Looking through the bindings, the only ones that are affected by this seem to be still autocomplete and toolbar. And that's obvious code bugs that should be fixed. I can try running the tests to see whether they turn up anything else.

Comment 13

[Boris Zbarsky [:bzbarsky]]

I'm not talking about our in-tree bindings only.  I'm talking about bindings in general, in the wild.

I suspect that Brendan is right and that we shouldn't touch this in XBL1.  If we do, I think it needs to be early in an alpha cycle..

Comment 14

[Wladimir Palant]

All tests did fine. As to bindings in the wild - that should be mostly extensions. Hard to tell how many extension have bugs similar to autocomplete, but I am pretty sure that those are much fewer than extensions assuming that bindings will stay attached after the node was removed from a document. So I will take the liberty and reopen this bug - I think this really needs to be addressed properly, causing subtle bugs with undocumented and very non-obvious behavior is very bad IMO.

Comment 15

[Wladimir Palant]

Attachment: Proposed namespace pollution patch

Comment 16

[Boris Zbarsky [:bzbarsky]]

It's not just extensions.  Akamai uses XBL, for example.

If you want to back this patch out, please file a new bug for that.  One bug per problem.

Note that this behavior SHOULD be documented.  If it's not, we need to fix that. But it's been in place ever since we've been shipping XBL, so changing it is not something to take lightly...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 289711 [details] [diff] [review]
Proposed namespace pollution patch

I'm not going to review this patch, sorry.  Most of it I can't even review if I wanted to.

Comment 18

[Boris Zbarsky [:bzbarsky]]

Oh, and as for tests... the number we have is tiny.  They don't have very good coverage, certainly not in this area.  Not breaking them is good as a minimum requirement, but doesn't produce any confidence in the patch, generally.

Comment 19

[[e-mail]]

Comment on attachment 289711 [details] [diff] [review]
Proposed namespace pollution patch

>       <constructor><![CDATA[
>-        mController = Components.classes["@mozilla.org/autocomplete/controller;1"].
>+        this.mController = Components.classes["@mozilla.org/autocomplete/controller;1"].
>                         getService(Components.interfaces.nsIAutoCompleteController);
>       ]]></constructor>
I wonder why this was never made a field...

>           // Look to see if there is a toolbarset.
>           this.toolbarset = this.firstChild;
>           while (this.toolbarset && this.toolbarset.localName != "toolbarset")
>-            this.toolbarset = toolbarset.nextSibling;
>+            this.toolbarset = this.toolbarset.nextSibling;
>           
>           if (this.toolbarset) {
>             // Create each toolbar described by the toolbarset.
>             var index = 0;
>-            while (toolbarset.hasAttribute("toolbar"+(++index))) {
>-              var toolbarInfo = toolbarset.getAttribute("toolbar"+index);
>+            while (this.toolbarset.hasAttribute("toolbar"+(++index))) {
>+              var toolbarInfo = this.toolbarset.getAttribute("toolbar"+index);
>               var infoSplit = toolbarInfo.split(":");
>               this.appendCustomToolbar(infoSplit[0], infoSplit[1]);
>             }
>           }
Even without the C++ changes I think it might be worth getting someone like gavin to review this for code correctness reasons.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Yes.  But not in this bug, please.  Stop the bugmail!