Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Looking up a nodelist element through an XRayWrapper using a string index fails

RESOLVED FIXED in mozilla9

Status

()

Core
XPConnect
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Steve Sobel, Assigned: mrbkap)

Tracking

6 Branch
mozilla9
x86_64
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

6 years ago
User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:6.0) Gecko/20100101 Firefox/6.0
Build ID: 20110811165603

Steps to reproduce:

I develop a Greasemonkey script called Reddit Enhancement Suite.  When running the version of Reddit Enhancement Suite that can be found here:

http://reddit.honestbleeps.com/v3.4/reddit_enhancement_suite.user.js

I found that the script started getting errors upon upgrading to Firefox 6 (from Firefox 5)...


Actual results:

What happened is I'd get a javascript error that this.keyboardLinks[i] was undefined...  Why?  I'm not sure!  Here's where my bug report comes in...

I'm grabbing DIV elements on a page and putting them in an array for the purpose of keyboard navigation.  This is done using document.body.querySelectorAll('div.entry .title');

I'm then adding a class to one of those divs (I'm caching which index they were last on using keyboard navigation, and adding a class to it to outline it so the user can see it is "focused")

I'm then looping through those divs and also adding event listeners and index numbers (via a class: keyIndex="i")...

After upgrading to Firefox 6, the element at index n - where n was the one I was setting to "focused", would be undefined - as if it had vanished from the array!

The way I modified the class was using basic functions as follows:

function hasClass(ele,cls) {
	if ((typeof(ele) == 'undefined') || (ele == null)) {
		if (typeof(console) != 'undefined') console.log(arguments.callee.caller);
		return false;
	}
	return ele.className.match(new RegExp('(\\s|^)'+cls+'(\\s|$)'));
}
function addClass(ele,cls) {
	if (!hasClass(ele,cls)) ele.className += " "+cls;
}


Here's the thing: This same javascript is used for RES in Chrome, Safari, Opera and of course prior versions of Firefox, and this piece of the array was never magically becoming undefined.


Expected results:

What should have happened is the javascript should've run successfully - the element in that array should never have vanished...

I've tried to make a clean "test case only" sort of thing, but I couldn't replicate the bug, so I understand it may be related to some of the other craziness going on in my massive userscript.  Still, it doesn't quite make sense especially given that if it were a problem with my code, it'd likely have manifested itself in other browsers and/or versions of Firefox.
Component: General → Style System (CSS)
Product: Firefox → Core
QA Contact: general → style-system
I'm going to say that this could *possibly* be in CSS, but I'm not sure.
(Reporter)

Comment 2

6 years ago
@Joe: how could it be in CSS?  It doesn't disappear from view - sorry if I wasn't clear about that - it disappears as in if we call my keyFocus: function on keyboardLinks[2], it disappears from the keyboardLinks[] array, like so:

keyboardLinks[0] = { dom object }
keyboardLinks[1] = { dom object }
keyboardLinks[2] = undefined
keyboardLinks[3] = { dom object }
keyboardLinks[4] = { dom object }
keyboardLinks[5] = { dom object }
Sorry; I meant the bug probably lies in the Gecko CSS system, since you're using querySelectorAll.

Comment 4

6 years ago
Steve, sorry for the lag here.  I was on vacation...

The return value of querySelectorAll is immutable, so the behavior you're seeing is _very_ odd.  Could you please give me clear steps to follow to reproduce the problem (assume I have never used Greasemonkey, which is a good assumption)?
(Reporter)

Comment 5

6 years ago
Boris:

Sure, here's the steps:

1) Install Firefox 6.0 (I haven't tried in any higher version, like 7)

2) Install greasemonkey v0.9.8 (only because I haven't tried a higher version of GM) from the "All versions" page: https://addons.mozilla.org/en-US/firefox/addon/greasemonkey/versions/?page=1#version-0.9.8 - click "add to firefox"

3) Install Reddit Enhancement Suite by simply clicking this link once Greasemonkey is installed:  http://reddit.honestbleeps.com/v3.4/reddit_enhancement_suite.user.js - you should be prompted with a Greasemonkey dialog saying you're going to install this script.  Click "install" once the countdown finishes (3 seconds)...

4) Browse to http://www.reddit.com/ with your javascript error console open (ctrl-shift-j), and you should see the error right away.  If you do not, click near (but not on) one of the links so that you see a light blue box highlight the area around the link.. then refresh the page.

You should see an error that says something like:

this.keyBoardlinks[i] is undefined

That should replicate it for you.  If you need any additional info, just let me know!

Comment 6

6 years ago
OK, so I can definitely reproduce the problem.

When I hit it, this.keyboardLinks is a NodeList, i == 3, length == 25, and if I call this.keyboardLinks.item(3) I get back an HTMLDivElement.

I tracked this down a bit, and one interesting thing is that we get a call to nsGenericArraySH::NewResolve with the id == 3 and with JSRESOLVE_ASSIGNING!  So basically, undefiend gets assigned to [3] on the nodelist (sort of; see below) and then after that we just get that back.

I believe this assignment happens when [3] is actually being _gotten_ like so:

  this.keyFocus(this.keyboardLinks[this.activeIndex]);

Here this.keyboardLinks is an XRayWrapper.  This lands us in js::proxy_GetProperty which calls xpc::XrayWrapper<JSCrossCompartmentWrapper>::get which calls xpc::XrayWrapper<JSCrossCompartmentWrapper>::getPropertyDescriptor which tries to resolveOwnProperty.  That triggers nsGenericArraySH::NewResolve and passes the _proxy_ as obj.

Now at the end of nsGenericArraySH::NewResolve we call ::JS_DefineElement to actually define the property.  This lands us under js::JSProxy::defineProperty which calls xpc::XrayWrapper<JSCrossCompartmentWrapper>::defineProperty which calls back into nsGenericArraySH::NewResolve but now with the JSRESOLVE_ASSIGNING flag.  This of course calls back into the proxy code, this time IsResolving() is true, so we just go ahead and define the property (on the holder?).  Then we unwind.  Something about all this ends up broken...
Component: Style System (CSS) → XPConnect
QA Contact: style-system → xpconnect

Comment 7

6 years ago
I think the key thing here is that the first id lookup there is for 3 as a _double_ and IsResolving uses pure id equality.
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 8

6 years ago
Actually, no.  The first lookup is for the _string_ "3".  Then this gets turned into a lookup for the _integer_ 3.  And of course IsResolving treats them differently...

I suspect this particular manifestation of the issue may go away with Peter's new nodelists, but this still seems like a bug in the xraywrapper.

Comment 9

6 years ago
Resummarizing to describe the actual problem; the class changes are, I suspect, completely unrelated to what's going on; the key part is that this.activeIndex is a string (presumably because that's what RESStorage.getItem('RESmodules.keyboardNavLastIndex.'+location.href) returns).
Summary: Element disappearing from javascript array when class modified → Looking up a nodelist element through an XRayWrapper using a string index fails

Comment 10

6 years ago
Created attachment 556128 [details] [diff] [review]
Hack that makes the problem go away but is probably totally wrong
Attachment #556128 - Flags: feedback?(mrbkap)
(Assignee)

Comment 11

6 years ago
Created attachment 559901 [details] [diff] [review]
Possible fix

Boris,

My super-simple testcase attached here doesn't seem to show the bug, but this patch should fix the problem you found in comment 6. Can you confirm (and/or do you see why my testcase works, even without my patch)?
Assignee: nobody → mrbkap
Attachment #556128 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #556128 - Flags: feedback?(mrbkap)
Attachment #559901 - Flags: feedback?(bzbarsky)

Comment 12

6 years ago
> My super-simple testcase attached here doesn't seem to show the bug

To trigger the bug the testcase should look more like this:

    var list = doc.getElementsByTagName("body");
    is(typeof list["0"], "object",
       "can lookup nodelist items by string");
    is(typeof list[0], "object",
       "can lookup nodelist items by integer after the lookup by string");

and the second test assertion there would fail.

If you do that, do you see it fail without the patch?
(Assignee)

Comment 13

6 years ago
Thanks! That code as written actually passes because the JS engine optimizes list["0"] into list.0 (that's pseudo-code). However, making the first test be:

var zeroAsString = "0";
is(typeof list[zeroAsString], "object", ...);

makes it show the bug.
(Assignee)

Comment 14

6 years ago
Created attachment 559965 [details] [diff] [review]
Proposed fix
Attachment #559901 - Attachment is obsolete: true
Attachment #559901 - Flags: feedback?(bzbarsky)
Attachment #559965 - Flags: review?(gal)

Comment 15

6 years ago
Comment on attachment 559965 [details] [diff] [review]
Proposed fix

For the test, are we worried about our constant-folding and stuff getting enough smarter sometime to optimize this pattern?  We can obfuscate the "0" a bit more if we have to, if so...
(Assignee)

Comment 16

6 years ago
Comment on attachment 559965 [details] [diff] [review]
Proposed fix

Andreas says r=him, looking over my shoulder.
Attachment #559965 - Flags: review?(gal) → review+
(Assignee)

Comment 17

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/4801c2f7bef8
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
https://hg.mozilla.org/mozilla-central/rev/4801c2f7bef8
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
You need to log in before you can comment on or make changes to this bug.