559792 - XUL menulist widget broken in content pages

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Wladimir Palant]

Apparently, menulist widget doesn't work in Fennec 1.1a2pre build 20100416014611 (testing on Windows 7), at least not in content pages.

Steps to reproduce:

* Install latest Adblock Plus development build (00latest.xpi from https://adblockplus.org/devbuilds/adblockplus/)
* "Add Adblock Plus filter subscription" page will open after a restart. If it doesn't, go to chrome://adblockplus/content/ui/subscriptionSelection.xul manually.
* Click the list to open the dropdown menu

Expected results:

* The dropdown menu opens and entries can be selected

Actual results:

* Error "this._control.menupopup is undefined" from browser-ui.js line 1763. This is MenulistWrapper.children getter.
* The page can no longer be scrolled
* Attempts to close the tab or to switch to a different tab result in a new error message: "this._panel is null" in browser-ui.js line 1939 (SelectHelper.close() method).

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: wip

This correct the bug by using wrappedJsObject when this is necessary but I want to have mrbkap's input on that.

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 439606 [details] [diff] [review]
wip


> function MenulistWrapper(aControl) {
>   this._control = aControl;
> }

Why not do:
this._control = aControl.wrappedJSObject || aControl;

and skip doing this everywhere else

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 439606 [details] [diff] [review]
wip

22:21 < gavin> why aren'tyou able to get the properties from the wrapper?
22:22 < gavin> those are all IDL-definedproperties,right?
22:22 < mrbkap> vingtetun: est-ce que c'est pour mozilla-central ?
22:22 < gavin> I guess I should look at the bug!
22:23 < gavin> oic
22:23 <@mfinkle> mrbkap: and 192
22:23 < gavin> they aren't IDL-defined, just XBL
22:24 -!- mak|afk is now known as mak
22:26 < mrbkap> mfinkle, vingtetun: on mozilla-central there's XPCNativeWrapper.unwrap, which does the .wrappedJSObject test for you.
22:26 < mrbkap> mfinkle, vingtetun: on 1.9.2, that looks OK to me.
22:26 < vingtetun> we're stuck on 1.9.2 for release
22:26 < mrbkap> Yeah :-/

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #2)
> (From update of attachment 439606 [details] [diff] [review])
> 
> > function MenulistWrapper(aControl) {
> >   this._control = aControl;
> > }
> 
> Why not do:
> this._control = aControl.wrappedJSObject || aControl;
> 
> and skip doing this everywhere else

That's what we used to do before but if we do that a malicious website can replace the dispatchEvent/select/ calls by something else

Comment 5

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #4)

> That's what we used to do before but if we do that a malicious website can
> replace the dispatchEvent/select/ calls by something else

OK, but I have no idea why this way is any better. You are only _not_ using wrappedJSObject in fireOnChange.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

Wouldn't the same malicious website be able to do evil things in the other calls?

Comment 7

[:Gavin Sharp [email: [e-mail]]]

To be clear, the only "malicious things" the website can do is return unexpected data. So you need to be aware that any time you're using .wrappedJSObject, you're potentially getting content-defined values that may be entirely bogus (applies to both properties and functions).

What you *don't* need to worry about privilege escalation attacks, since those should be prevented (by XPCSafeJSObjectWrapper).

https://developer.mozilla.org/En/XPConnect_wrappers explains in more detail.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #5)
> (In reply to comment #4)
> 
> > That's what we used to do before but if we do that a malicious website can
> > replace the dispatchEvent/select/ calls by something else
> 
> OK, but I have no idea why this way is any better. You are only _not_ using
> wrappedJSObject in fireOnChange.

No, also in a a few other places. I guess this is not _better_ but you prevent us to use methods of the web page element that can be different than what we are expecting.

My main concern come from a test page build by mrbkap:

http://people.mozilla.com/~mrbkap/fennec.html (bug 538442)

Here the setOptionsSelectedByIndex method is overridden and prevent us to do what the user expect to see.

Comment 9

[Wladimir Palant]

A webpage can always mess up an element that it contains. Something as trivial as onselect="alert('Hi!');this.selectedIndex = -1;" will have the same effect, at least from the user's perspective. Not to mention that both in Fennec and Firefox it can easily replace the default XBL binding by it own that works in an unexpected way. So I wouldn't worry too much about selectedIndex not working as expected (other than converting the result to an integer). I would however get the menupopup element "manually" instead of using wrappedJSObject and the XBL getter, that's simple enough.

Comment 10

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to comment #9)
> A webpage can always mess up an element that it contains. Something as trivial
> as onselect="alert('Hi!');this.selectedIndex = -1;" will have the same effect,
> at least from the user's perspective. Not to mention that both in Fennec and
> Firefox it can easily replace the default XBL binding by it own that works in
> an unexpected way. 

Right, but the "onselect" will fire only after we have say the web content to do it, which mean once we've done what it supposed to be.

But I guess you're right for the custom xbl here.

> So I wouldn't worry too much about selectedIndex not working
> as expected (other than converting the result to an integer). I would however
> get the menupopup element "manually" instead of using wrappedJSObject and the
> XBL getter, that's simple enough.

I don't really want to do that. If we can abstract the inner structure of the lement and to duplicate the xlb's code it's worth the wrappedJSObject for me.

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 439606 [details] [diff] [review]
wip

Sounds like this will work as we intend.

Comment 12

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/a2e32d6b520e

Comment 13

[Wladimir Palant]

Comment on attachment 439606 [details] [diff] [review]
wip

> MenulistWrapper.prototype = {
>-  get selectedIndex() { return this._control.selectedIndex; },
>+  get selectedIndex() {
>+    let control = this._control.wrappedJSObject || this._control;
>+    return this._control.selectedIndex;

I think you mean to use the control variable here, not this._control. And I would really do something like this:

> let result = parseInt(control.selectedIndex);
> return (isNaN(result) ? -1 : result);

I don't know what code is using this, I wouldn't risk returning something entirely unexpected (string? object?) to it. This was the result will always be an integer.

Would it make sense to catch exceptions in selectedIndex getter/setter? The calling code might not expect an exception and break badly.

Comment 14

[Wladimir Palant]

Reopening, see comment above.

Comment 15

[Wladimir Palant]

Actually, better:

> let result = control.selectedIndex;
> return (typeof result == "number" && !isNaN(result) ? result : -1);

Reason: parseInt() will call toString() on an object (which is already undesirable in itself) and might throw (e.g. if the object is {__proto__: null}). Better simply reject anything that isn't a number.

Comment 16

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #13)
> (From update of attachment 439606 [details] [diff] [review])
> > MenulistWrapper.prototype = {
> >-  get selectedIndex() { return this._control.selectedIndex; },
> >+  get selectedIndex() {
> >+    let control = this._control.wrappedJSObject || this._control;
> >+    return this._control.selectedIndex;
> 
> I think you mean to use the control variable here, not this._control.

Oops!

(In reply to comment #15)
> Actually, better:
> 
> > let result = control.selectedIndex;
> > return (typeof result == "number" && !isNaN(result) ? result : -1);
> 
> Reason: parseInt() will call toString() on an object (which is already
> undesirable in itself) and might throw (e.g. if the object is {__proto__:
> null}). Better simply reject anything that isn't a number.

Looks good

Comment 17

[Mark Finkle (:mfinkle) (use needinfo?)]

Attachment: followup

followup with suggested fixes

Comment 18

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 439983 [details] [diff] [review]
followup

Thanks Wladimir for yout time spent of this bug and thanks Mark for providing a followup.

Comment 19

[Mark Finkle (:mfinkle) (use needinfo?)]

pushed:
http://hg.mozilla.org/mobile-browser/rev/68ce334c00d1

Comment 20

[Aakash Desai [:aakashd]]

verified FIXED on builds:

Mozilla/5.0 (X11; U; Linux armv7l; Nokia N900; en-US; rv:1.9.2.5pre) Gecko/20100420 Namoroka/3.6.5pre Fennec/1.1a2pre

and

Mozilla/5.0 (X11; U; Linux armv71; Nokia N900; en-US; rv:1.9.3a5pre) Gecko/20100420 Namoroka/3.7a5pre Fennec/1.1a2pre