715156 - JS code no longer work for select options as array in a Greasemonkey script (index and name setters don't really work via XrayWrapper)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nam H]

User Agent: Mozilla/5.0 (Ubuntu; X11; Linux x86_64; rv:10.0) Gecko/20100101 Firefox/10.0
Build ID: 20120102221218

Steps to reproduce:

This code used to work:

myselect.options[myselect.options.length] = option;

It stopped working lately.  I don't know what the standard is, but I would suppose this will break lots of existing code.  So this should be very high important bug.

The following code work, and can be used as a work around:

myselect.options.add(option);



Actual results:

No error, the select didn't populate any option.


Expected results:

The options should be added and shown.

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Regression from bug 648801?

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: Attempt at testcase; does NOT show the bug

This is a attempt to create a testcase based on comment 0, but it works just fine for me, both with a nightly and with a Firefox 10 beta.

Reporter, can you please attach an actual testcase that demonstrates your problem?

Comment 3

[Nam H]

So it maybe more limited than I thought.  This is version 10 specific.  Also, it's actually a code from GreaseMonkey script.

To test, you install it as a GreaseMonkey script.  Line 30 doesn't work.  Line 32 works, and can be tested by commented out line 30, and uncomment line 32:
[code]
// ==UserScript==
// @name          TestSelect
// @description   Test Select option FF10
// @include       http://*/*
// @include       https://*/*
// ==/UserScript==

TestSelect = function(browserWin) {

    this.initialize = function(evt) {
       if (evt != null) {
            evt.preventDefault();
            evt.stopPropagation();
        }
        this.addList();
    }

    this.addList = function() {
        var body = browserWin.document.getElementsByTagName("BODY");
        if (body && body.length > 0) {
            body = body[0];
        } else {
            return;
        }
                var selectList = newNode("select", body, browserWin.document);

                for (var i = 0; i < 10; i++) {
                    var option = new Option("something" + i, "something" + i, false, false);
			//code that doesn't do anything:
                    selectList.options[selectList.options.length] = option;
			//code that works:
			//selectList.options.add(option);
                }
    }

    this.initialize(null);
}

newNode = function(tag, on, doc) {
    var e = doc.createElement(tag);
    if (on) on.appendChild(e);
    return e;
}

try {
    var isGreaseMonkeyScript = false;
    try {
        if (typeof GM_getValue != "undefined") {
            isGreaseMonkeyScript = true;
        }
    } catch (err) {
    }
    if (isGreaseMonkeyScript) {
        window.addEventListener("load", function() {
            new TestSelect(window);
        }, true);
    }
    if (window.opera) {
        new TestSelect(window);
    }

    var chrome_browser = navigator.userAgent.toLowerCase().indexOf('chrome') > -1;
    if (chrome_browser) {
        new TestSelect(window);
    }
} catch (e) {
}

[/code]

Comment 4

[Boris Zbarsky [:bzbarsky]]

> This is version 10 specific. 

I did test version 10....

> Also, it's actually a code from GreaseMonkey script.

Ah, that would be very relevant.  Those run in a totally different environment from normal web code!

Peter, Bobby, what's happening here is that we land in ListBase<LC>::getOwnPropertyDescriptor via the XrayProxy code from set().  We have an index setter, so we call FillPropertyDescriptor, but that sets up no setter for the prop. Then xpc::XrayProxy::getOwnPropertyDescriptor sets the obj on the descriptor to itself.    Then we end up in xpc::XrayProxy::defineProperty (via js::ProxyHandler::set), which just defines the property on the holder.  That's broken for indexsetter/namesetter cases, of course.

I see all that on trunk too, not just in 10.

bobby, I know you were in that code recently; do your patches fix this by any chance?  I'm guessing not...

Comment 5

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #4)
> bobby, I know you were in that code recently; do your patches fix this by
> any chance?  I'm guessing not...

My code (bug 706301) already landed, so if you're seeing it on trunk too then presumably not.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Ah, yes.  OK.

So yeah, name/index setters via XrayProxy just seem completely broken...

Comment 7

[Alex Keybl [:akeybl]]

Tracking for Firefox 10/11 considering the concern that this may break many scripts.

Comment 8

[Alex Keybl [:akeybl]]

Johnny/Peter - could you find somebody to take a look at this? We want to make sure that all tracked bugs are assigned to the person with next action, or closed. Thanks!

Comment 9

[Nam H]

I also noticed that in Weblogic server 11, the Shutdown drop down selection list (in the Admin console->Servers->Control) no longer work in FF10.  I don't know if it's the same issue, but it's rather strange that it's happening in normal environment (non-grease monkey/plugins).

Comment 10

[Josh Matthews [:jdm]]

vy.huy.ho, that sounds like something that should be filed as a new bug. Would you mind doing that, please, and giving a link to the Weblogic server on which you are experiencing the issue?

Comment 11

[Nam H]

it turns out the Weblogic problem is with the web browser version 9.0.1 and it's an unrelated issue.

Comment 12

[Johnny Stenback (:jst)]

Peter says he'll look into this.

Comment 13

[Alex Keybl [:akeybl]]

Any updates?

Comment 14

[Alex Keybl [:akeybl]]

Spoke with jst about this - our expectation is that we would have had more reports post-FF10 if this was of major concern. In that case, untracking for FF11.

Comment 15

[janekptacijarabaci]

FF 10.0 - 15.0.1
It is shame.

Comment 16

[Anthony Lieuallen]

https://github.com/greasemonkey/greasemonkey/issues/1510#issuecomment-10840641

Users are still complaining about this bug.  Is it ever going to be fixed?

Comment 17

[Boris Zbarsky [:bzbarsky]]

The first problem is figuring out how to fix it while remaining secure...  No one has had a chance to look into that yet.

Comment 18

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 19

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 701031 [details] [diff] [review]
v1

r=me, I guess, but this will make setting expandos on a Document via an Xray pretty dicey, no?  In particular, if your expando happens to collide with an <img> or whatnot...

Maybe we can just make Document pretend it's not [OverrideBuiltins] when touched via an Xray.

Comment 20

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky (:bz) from comment #19)
> this will make setting expandos on a Document via an Xray
> pretty dicey, no?  In particular, if your expando happens to collide with an
> <img> or whatnot...

Yeah, it's going to be pretty complicated. Technically I think it should throw, but I don't think we want content script to be able to control when the Xray throws. I haven't thought about it a lot, but we might be able to deal with this in getOwnPropertyDescriptor.

Comment 21

[Peter Van der Beken [:peterv]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3c145bd1dd2

Comment 22

[Ed Morley [:emorley]]

Push backed out for OS X startup failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=f3c145bd1dd2

https://hg.mozilla.org/integration/mozilla-inbound/rev/18b2fd105563

Comment 23

[Peter Van der Beken [:peterv]]

(In reply to Ed Morley [:edmorley UTC+0] from comment #22)
> Push backed out for OS X startup failures:

Which weren't actually related to this parch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/68e0432939c9

Comment 24

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/68e0432939c9