702847 - strange issue with array elements being undefined and testing typeof failing

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P2)

Description

[Steve Sobel]

User Agent: Mozilla/5.0 (Windows NT 6.0; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20111104165243

Steps to reproduce:

I develop Reddit Enhancement Suite, which can be downloded from Github here:

http://github.com/honestbleeps/Reddit-Enhancement-Suite

I can run the following steps:
- get code from github
- cfx xpi to compile
- go to www.reddit.com
- click any user's "tag" icon, and set a tag along with a color (be sure to set a color)...
- save the tag... then click the tag again to edit... you will get a javascript error.


Actual results:

I have a function in the code that takes in a SELECT form element and a value, and appropriately sets the selectedIndex based on that value -- like so:

	setSelectValue: function(obj, value) {
		console.log('set select value... ???!!?@#?!@#');
		for (var i=0, len=obj.length; i < len; i++) {
			if ((typeof(obj[i]) != 'undefined') && (obj[i].value == value)) {
				obj[i].selected = true;
			}
		}
	},

Note that I have added in checks for typeof(obj[i]) - which really shouldn't even have to be checked!  These should never be undefined.

However, they are showing up as undefined in Firefox (and no other browser)...

Furthermore, the check fails anyway, and the script fails!  However, if I add a console.log(typeof(obj[i])) prior to the test -- then the test passes...  this is consistent: remove that console.log, the script errors out again saying obj[i] is undefined.


Expected results:

What should have happened is that no elements in obj[] should be undefined ever! Even if they are, however, a check for typeof(obj[i]) should properly at least short circuit that if statement and not cause a javascript error.

Worse yet, throwing in a console.log statement prior to that check of:

console.log(typeof(obj[i])) suddenly at least avoids a javascript error.

removing the console.log statement makes that error come back.

I will try and create a more isolated test case if and when I have more time, but I'm not sure how soon I can get to that, so I wanted to link to the full project for you to at least be able to easily download and replicate the steps.

Thanks!

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Possible fix

Oh there is a known issue with html collections in content script and
Select is one of them, I didn't knew that!

Steve, can you try your addon with this patch applied?
This patch is against current master and won't work if you are using 1.2/1.3 releases.

Comment 2

[Alexandre Poirot [:ochameau]]

Without this patch I had an exception from this line:
https://github.com/honestbleeps/Reddit-Enhancement-Suite/blob/master/XPI/data/reddit_enhancement_suite.user.js#L6321

With it, I don't get any exception anymore, but color is still not updated in select ... can you confirm that there is no more issue around select?

Comment 3

[Steve Sobel]

Sorry for the slow response - I haven't yet had the chance to try out the patch, but I'm about to investigate getting current master and wanted to let you know I haven't abandoned tracking this bug, just been busy!

Comment 4

[Steve Sobel]

Update: It appears that that patch fixes it! I didn't have the same problem Alexandre had with the color not being selected... it works for me.  Thank you!

Comment 5

[Alexandre Poirot [:ochameau]]

Comment on attachment 574862 [details] [diff] [review]
Possible fix

Ok so if that fix this bug, let's review it!

In this patch, I just applied the same trick as with NodeList and HTMLCollection,
for an unknown reason there is an issue around lists.
You can access to each list elements only once, then it becomes undefined :x
The best would be to figure out what is really going on, but the only way to digg this is by debugging it through gdb and it will take lot of time :(
I already tried to reproduce this bug with simple test case without success. 
I don't think we can reproduce it without using Proxies.

Comment 6

[Alexandre Poirot [:ochameau]]

gabor: By any chance, would you have any guess why we have such issue ?
Do you know anything special around NodeList, HtmlCollection & all ?

Comment 7

[Gabor Krizsanits (INACTIVE)]

(In reply to Alexandre Poirot (:ochameau) from comment #6)
> gabor: By any chance, would you have any guess why we have such issue ?
> Do you know anything special around NodeList, HtmlCollection & all ?

No, I don't know much about this part, but me or Eddy will take a look at it hopefully soon. Shall we add this bug or it's meta bug to bug 697775 ?

Comment 8

[Alexandre Poirot [:ochameau]]

I'm not 100% sure there is something wrong in platform code,
it may just be something wrong in my proxy code.
But in order to be sure, we will have to see what gdb says about these undefined values.

Comment 9

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

Comment on attachment 574862 [details] [diff] [review]
Possible fix

Review of attachment 574862 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 10

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

(In reply to Alexandre Poirot (:ochameau) from comment #6)
> gabor: By any chance, would you have any guess why we have such issue ?
> Do you know anything special around NodeList, HtmlCollection & all ?

I think they all are lazily populated, also jsdom project needed proxies for that.

Comment 11

[Eddy Bruel [:ejpbruel]]

I can reproduce this bug on Firefox 8, but not on Firefox Nightly, Aurora or Beta. I think it's safe to assume that this issue has been fixed, even though we do not know what the cause of the problem was.

Comment 12

[Alexandre Poirot [:ochameau]]

Interesting, I give it a try too and it appears that the whole class of similar issue. i.e. all issues around Collection have been fixed.
So that we should follow this in order to remove workarounds when we do no support buggy firefox versions!
So Beta means the next one, 9, right?
So we will be able to remove them when we drop 8 support.
It would be handy to know on which jetpack release it will happened so that we can drop some notes somewhere?

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Firefox 9 comes out on December 20, so if I understand our support and release plans, master branch will switch to supporting Firefox 9 through Nightly 12 three weeks after December 20. So January 10?

Comment 14

[Myk Melez [:myk] [@mykmelez]]

Hmm, I thought we decided that master's minVersion will get bumped in conjunction with the Firefox release, so next week.

Comment 15

[Jordan Santell [:jsantell] [@jsantell]]

I think this is no longer an issue and the above has landed; closing it up!