Last Comment Bug 780993 - Setting indexed properties on HTMLSelectElement to null no longer works
: Setting indexed properties on HTMLSelectElement to null no longer works
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: :Ms2ger (⌚ UTC+1/+2)
: Paul Silaghi, QA [:pauly]
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 752226
  Show dependency treegraph
 
Reported: 2012-08-07 13:50 PDT by john.o.nye
Modified: 2012-09-19 04:06 PDT (History)
7 users (show)
Ms2ger: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
+
verified
+
verified
+
verified
unaffected


Attachments
Archive.zip (10.08 KB, application/octet-stream)
2012-08-07 13:50 PDT, john.o.nye
no flags Details
Testcase from comment 3 (2.46 KB, text/html)
2012-08-08 09:08 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Minimal testcase. The select should say "Correct" (181 bytes, text/html)
2012-08-08 09:31 PDT, Boris Zbarsky [:bz] (still a bit busy)
no flags Details
Patch v1 (3.09 KB, patch)
2012-08-08 09:51 PDT, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
Details | Diff | Splinter Review
Patch v2 (3.52 KB, patch)
2012-08-08 13:33 PDT, :Ms2ger (⌚ UTC+1/+2)
bzbarsky: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
lukasblakk+bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description john.o.nye 2012-08-07 13:50:40 PDT
Created attachment 649789 [details]
Archive.zip

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.8; rv:15.0) Gecko/20100101 Firefox/15.0
Build ID: 20120717110313

Steps to reproduce:

I am writing routines to fill a form and want to clear a select that may contain residue from prior use. The routine contains a while statement to empty the select options. Today Firefox loaded an update and the function does not complete and no error is shown in the Firebug console. 


Actual results:

The form should have been cleared and then populatd with the entry to be edited. It cleared the text box, but did not clear the list and exited without running functions to display control buttons


Expected results:

The clear should complete, the form should be repopulated and the buttons displayed. The attached ZIP contains the html, css and js files. The steps taken: select text response creation. Save a completed entry. highlight the new entry in righthand list and click edit. population does not complete.
Comment 1 :Benjamin Peterson 2012-08-07 16:27:08 PDT
The title claims that "javascript while statement exits function". Do you have a smaller example that exhibits the problem?
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2012-08-07 16:40:29 PDT
Actually, the given testcase might be OK, but what's really needed are actual steps to reproduce (and actual and expected results) using that testcase.  That is, exact instructions on what to click in what order and how someone not familiar with what the page is _trying_ to do would tell whether things were cleared correctly.
Comment 3 john.o.nye 2012-08-08 09:04:23 PDT
I have condensed the conditions producing the error into a small html file. It has one button that performs the actions leading to the error. - It is copied below:

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">
<html>
<head>
<meta http-equiv="Content-type" Content="text/html; charset=UTF-8">
<title>Bug Page</title>
<style>
body, select, text, textarea, input, select { font: normal normal 11px Verdana, Geneva, Arial, Helvetica, sans-serif; }
span.reqset { color: #dc143c; font-size: 9px; }
span.clickme { cursor:pointer }
th.stmt { width: 200px; }
th.rate { width: 100px; }
tr.sepa { height: 2px; }
td.sepa { height: 2px; background: lightgray; }
</style>
<script language=javascript>

var toedit = 'Page-A:-:Text:-:New Text:-:New One:=:New Two';

function checkSize (tel, size) {
	var fm = document.forms[0];
	var thisCont = fm.elements[tel].value;
	var thisLen = thisCont.length;
	if (thisLen > size) {
		thisCont = thisCont.substr(0, size);
		thisLen = size;
		fm.elements[tel].value = thisCont;
	}
	var dsize = size - thisLen;
	document.getElementById(tel + 'X').innerHTML = dsize;
}
function setSelSize (tel) {
	var fm = document.forms[0];
	var theSize = fm.elements[tel].length;
	document.getElementById(tel + 'X').innerHTML = theSize;
}
function edpage() {
	alert('Starting form stup');
	document.forms[0].tptc.value = '';
	alert('Clearing stuff');
	while(document.forms[0].tell.length > 0) { document.forms[0].tell[0] = null; }
	alert('Have cleared the list');
	if (toedit != '') {
		alert('Have to prime form');
		var tArr = toedit.split(':-:');
		document.forms[0].tptc.value = tArr[2].replace(/<br>/g,'\n');
		var tLi = tArr[3].split(':=:');
		for (var ix = 0; ix < tLi.length; ix++) {
			document.forms[0].tell.add(new Option(tLi[ix], tLi[ix]), null);
		}
		document.forms[0].tell.size = 2;
		if (document.forms[0].tell.length > 2 && document.forms[0].tell.length < 10) document.forms[0].tell.size = document.forms[0].tell.length;
		if (document.forms[0].tell.length > 9) document.forms[0].tell.size = 9;
		
		alert('Set Form Sizes');
		checkSize('tptc',500);
		alert('Set Header Size');
		setSelSize('tell');
	}
}
function sendA(msg) {
	alert(msg);
}

</script>
</head>
<body>
<form>
<textarea wrap=logical name=tptc cols=100 rows=5 onKeyUp="sendA('KeyUp'); checkSize('tptc',500);">Residue text</textarea><span id=tptcX>488</span><br>
<select style="width:550px" size=2 name=tell>
<option value="Old One">Old One</option>
<option value="Old Two">Old Two</option>
</select><span id=tellX>2</span><br>
<input type=button value="Reload" onClick="sendA('Reload'); edpage();">
</form>
</body>
</html>
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2012-08-08 09:08:08 PDT
Created attachment 650160 [details]
Testcase from comment 3
Comment 5 Boris Zbarsky [:bz] (still a bit busy) 2012-08-08 09:14:14 PDT
Running that testcase and clicking the "Reload" button (which I assume is what one is supposed to do), I get an exception thrown on this line:

  while(document.forms[0].tell.length > 0) { document.forms[0].tell[0] = null; }
Comment 6 john.o.nye 2012-08-08 09:19:43 PDT
Yes, that is the problem I am reporting. I see this in Firefox on OSX Build 15, with Web Console reporting an NS_ERROR_UNEXPECTED: Unexpected error

When I run in Chrome or Safari - no error is experienced.

The code has been static in this area for some time, and the error appeared a few days ago with a Firefox Automatic Update.
Comment 7 Boris Zbarsky [:bz] (still a bit busy) 2012-08-08 09:27:09 PDT
Regression range:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=c758cc9b60e5&tochange=ac968ff4fe41
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2012-08-08 09:28:02 PDT
This has got to be a regression from bug 752226.  Still looking for the exact codepath that was changed incorrectly there.
Comment 9 Boris Zbarsky [:bz] (still a bit busy) 2012-08-08 09:31:13 PDT
Created attachment 650174 [details]
Minimal testcase.  The select should say "Correct"
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2012-08-08 09:35:09 PDT
nsHTMLSelectElementSH::SetOption is what used to accept null and no longer does.

The good news is that this is only on beta so far.  John, thank you very much for running betas and for reporting the bug!

Ms2ger is going to take this.
Comment 11 john.o.nye 2012-08-08 09:37:46 PDT
I am glad to be able to help. I am truly impressed with your response time. I am used to dealing with Oracle Support which tends to be slow to react.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2012-08-08 09:41:31 PDT
Yeah, we do try to be better than Oracle.  ;)
Comment 13 :Ms2ger (⌚ UTC+1/+2) 2012-08-08 09:51:09 PDT
Created attachment 650186 [details] [diff] [review]
Patch v1

https://tbpl.mozilla.org/?tree=Try&rev=826453a14ec5
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2012-08-08 09:59:51 PDT
Comment on attachment 650186 [details] [diff] [review]
Patch v1

r=me
Comment 15 :Ms2ger (⌚ UTC+1/+2) 2012-08-08 13:33:56 PDT
Created attachment 650277 [details] [diff] [review]
Patch v2

This one doesn't crash... do_QueryWrapper isn't null-safe, unlike all other do_*s.
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2012-08-08 15:21:18 PDT
Comment on attachment 650277 [details] [diff] [review]
Patch v2

<sigh>.

r=me
Comment 17 :Ms2ger (⌚ UTC+1/+2) 2012-08-09 04:03:54 PDT
https://hg.mozilla.org/mozilla-central/rev/cefda2b7296b
Comment 18 :Ms2ger (⌚ UTC+1/+2) 2012-08-09 04:05:53 PDT
Comment on attachment 650277 [details] [diff] [review]
Patch v2

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 752226
User impact if declined: exceptions are thrown on some web pages
Testing completed (on m-c, etc.): On m-c; passes tests.
Risk to taking this patch (and alternatives if risky): Little risk
String or UUID changes made by this patch: None
Comment 19 Lukas Blakk [:lsblakk] use ?needinfo 2012-08-09 10:18:18 PDT
Comment on attachment 650277 [details] [diff] [review]
Patch v2

Since this has tests and first appears in 15, we'll take this for branches.
Comment 21 Paul Silaghi, QA [:pauly] 2012-08-17 04:49:00 PDT
Able to see the issue on FF 15b4.
No errors on FF 15b5 in the web console using the STR in comment 5. Also, the "correct" value is displayed loading the testcase in comment 9.
Verified fixed on FF 15b5 on Win 7 x64, Ubuntu 12.04 and Mac OS X 10.6.
Comment 22 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2012-09-18 16:06:45 PDT
Paul, please verify this against the latest Firefox 16 beta and Firefox 17 aurora, thanks.
Comment 23 Paul Silaghi, QA [:pauly] 2012-09-19 04:06:18 PDT
Verified fixed on FF 16b3 and FF 17.0a2 (2012-09-18) on Win 7 x64, Ubuntu 12.04 and Mac OS X 10.7.4

Note You need to log in before you can comment on or make changes to this bug.