Closed Bug 715156 Opened 11 years ago Closed 10 years ago

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

Categories

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

10 Branch
x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla21
Tracking Status
firefox10 + ---
firefox11 - ---

People

(Reporter: vy.huy.ho, Assigned: peterv)

References

Details

(Keywords: addon-compat, regression)

Attachments

(2 files)

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.
Severity: normal → major
Regression from bug 648801?
Severity: major → normal
Component: General → DOM: Core & HTML
Product: Firefox → Core
QA Contact: general → general
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?
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]
> 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...
Blocks: 648801
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: JS code no longer work for select options as array → JS code no longer work for select options as array in a Greasemonkey script (index and name setters don't really work via XrayWrapper)
(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.
Ah, yes.  OK.

So yeah, name/index setters via XrayProxy just seem completely broken...
Tracking for Firefox 10/11 considering the concern that this may break many scripts.
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!
Assignee: nobody → jst
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).
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?
See Also: → 716571
it turns out the Weblogic problem is with the web browser version 9.0.1 and it's an unrelated issue.
See Also: 716571
Peter says he'll look into this.
Assignee: jst → peterv
Any updates?
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.
FF 10.0 - 15.0.1
It is shame.
https://github.com/greasemonkey/greasemonkey/issues/1510#issuecomment-10840641

Users are still complaining about this bug.  Is it ever going to be fixed?
The first problem is figuring out how to fix it while remaining secure...  No one has had a chance to look into that yet.
Attached patch v1Splinter Review
Attachment #701031 - Flags: review?(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.
Attachment #701031 - Flags: review?(bzbarsky) → review+
(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.
(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
https://hg.mozilla.org/mozilla-central/rev/68e0432939c9
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
You need to log in before you can comment on or make changes to this bug.