Closed
Bug 715156
Opened 13 years ago
Closed 12 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)
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: vy.huy.ho, Assigned: peterv)
References
Details
(Keywords: addon-compat, regression)
Attachments
(2 files)
206 bytes,
text/html
|
Details | |
8.41 KB,
patch
|
bzbarsky
:
review+
|
Details | Diff | Splinter Review |
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•13 years ago
|
||
Regression from bug 648801?
Severity: major → normal
Component: General → DOM: Core & HTML
Product: Firefox → Core
QA Contact: general → general
Comment 2•13 years ago
|
||
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]
Comment 4•13 years ago
|
||
> 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
tracking-firefox11:
--- → ?
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)
Updated•13 years ago
|
tracking-firefox10:
--- → ?
Keywords: addon-compat,
regression
Comment 5•13 years ago
|
||
(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•13 years ago
|
||
Ah, yes. OK.
So yeah, name/index setters via XrayProxy just seem completely broken...
Comment 7•13 years ago
|
||
Tracking for Firefox 10/11 considering the concern that this may break many scripts.
Comment 8•13 years ago
|
||
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).
Comment 10•13 years ago
|
||
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?
Reporter | ||
Comment 11•13 years ago
|
||
it turns out the Weblogic problem is with the web browser version 9.0.1 and it's an unrelated issue.
Comment 13•13 years ago
|
||
Any updates?
Comment 14•13 years ago
|
||
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•12 years ago
|
||
FF 10.0 - 15.0.1
It is shame.
Comment 16•12 years ago
|
||
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•12 years ago
|
||
The first problem is figuring out how to fix it while remaining secure... No one has had a chance to look into that yet.
Assignee | ||
Comment 18•12 years ago
|
||
Assignee | ||
Updated•12 years ago
|
Attachment #701031 -
Flags: review?(bzbarsky)
Comment 19•12 years ago
|
||
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+
Assignee | ||
Comment 20•12 years ago
|
||
(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.
Assignee | ||
Comment 21•12 years ago
|
||
Comment 22•12 years ago
|
||
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
Assignee | ||
Comment 23•12 years ago
|
||
(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•12 years ago
|
||
Status: NEW → RESOLVED
Closed: 12 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.
Description
•