XUL menulist widget broken in content pages

VERIFIED FIXED

Status

Firefox for Android Graveyard
General
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: Wladimir Palant, Assigned: vingtetun)

Tracking

(Blocks: 1 bug)

Trunk
x86
Windows 7

Details

Attachments

(2 attachments)

(Reporter)

Description

8 years ago
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).
Created attachment 439606 [details] [diff] [review]
wip

This correct the bug by using wrappedJsObject when this is necessary but I want to have mrbkap's input on that.
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 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 :-/
Attachment #439606 - Flags: review?(gavin.sharp)
(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
(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.
Wouldn't the same malicious website be able to do evil things in the other calls?
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.
(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.
(Reporter)

Comment 9

8 years ago
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.
(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 on attachment 439606 [details] [diff] [review]
wip

Sounds like this will work as we intend.
Attachment #439606 - Flags: review?(gavin.sharp) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/a2e32d6b520e
Assignee: nobody → 21
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
(Reporter)

Comment 13

8 years ago
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.
(Reporter)

Comment 14

8 years ago
Reopening, see comment above.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Reporter)

Comment 15

8 years ago
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.
(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
Created attachment 439983 [details] [diff] [review]
followup

followup with suggested fixes
Attachment #439983 - Flags: review?(21)
Comment on attachment 439983 [details] [diff] [review]
followup

Thanks Wladimir for yout time spent of this bug and thanks Mark for providing a followup.
Attachment #439983 - Flags: review?(21) → review+
pushed:
http://hg.mozilla.org/mobile-browser/rev/68ce334c00d1
Status: REOPENED → RESOLVED
Last Resolved: 8 years ago8 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.