377413 - Dynamic delete of form elements leaves references sometimes - add tests for this old bug to guard against regressions

Status: RESOLVED FIXED

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

Description

[Les Barstow]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1.3) Gecko/20070309 Firefox/2.0.0.3

The URL above is a somewhat simple application that inserts and deletes rows in a table using JavaScript.  Rows all contain form input fields.  Any reference to an existing form element - statically created, or dynamically created using innerHTML or document.createElement("input") - causes the element to become somewhat indelible.  Deleting the table row containing the input field (or even deleting the input element itself and then deleting the row) leaves a residual reference for any element that had been referenced while it existed.

The referenced URL document contains several sample tests that can be run.  URL contains all necessary JavaScript and HTML.

Reproducible: Always

Steps to Reproduce:
1. See referenced URL - steps to reproduce are contained within the document.
Actual Results:  
If, after creating an input element (using innerHTML or createElement), you reference it by testing for existence or assigning a value, then subsequent deletion (via table deleteRow or via getElementById(elementid).parentNode.removeChild(getElementById(elementid)) ) does not completely delete all references to element; a test still yields a true result for document.formname.elementname, though you can no longer getElementById(elementid), and the element does not appear anywhere in the DOM Inspector.

Expected Results:  
No references should remain for the deleted element.  IE7 correctly renders this result.

Tested on Firefox 2.0.0.3 on WinXP and Mac OS X, as well as Firefox 1.5.0.11 WinXP.

Comment 1

[Steve England [:stevee]]

Can you please test on a trunk build of firefox?
- http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2007-04-13-04-trunk/

Comment 2

[Les Barstow]

Tested.  Still exists.

Comment 3

[Gérard Talbot]

Bug 307415 is very similar to this bug.

Comment 4

[Les Barstow]

In this instance, the form property was no longer visible in the DOM Inspector after removal of the element.  However, the effect was almost identical - the old element still appeared to have a property the form when doing programmatic testing.  I agree, #307415 does appear to be almost identical.

Comment 5

[Les Barstow]

Added test case back to web server (was accidentally deleted during cleanup).

Comment 6

[Hallvord R. M. Steen [:hallvors]]

Attachment: 377413.htm - tc

Comment 7

[Hallvord R. M. Steen [:hallvors]]

Original test is gone but I wrote a small test case and this seems to have been fixed at some point.

Comment 8

[Hallvord R. M. Steen [:hallvors]]

Attachment: bug_377413_test.patch

Attaching a patch for adding mochitests for this bug

Comment 9

[Hallvord R. M. Steen [:hallvors]]

I've tried to add some tests. I'm not sure if existing tests cover this (might quite possibly do) (at least it was a useful learning exercise for me to figure out how to add tests to the codebase).

Could someone - maybe billm, bz or jmaher - take a look and decide whether it's useful to add this patch to mozilla-central? TIA.

Comment 10

[Joel Maher ( :jmaher ) (UTC -8)]

A few things to consider:
1) is this going to run reliably on desktop, android, and b2g?
2) do we have similar coverage in other tests (most likely in the same directory)

I am not sure about either of these questions, but if this test provides us coverage which we do not have, lets get it added.  We can always use the manifest to turn it off on platforms where it doesn't run (although I suspect this specific test case will run fine on all platforms).

Comment 11

[Hallvord R. M. Steen [:hallvors]]

(In reply to Joel Maher (:jmaher) from comment #10)
> 1) is this going to run reliably on desktop, android, and b2g?

I can think of no reasons it won't, (and if it doesn't that might indicate a bug..) it tests core DOM stuff that should work consistently.

> 2) do we have similar coverage in other tests (most likely in the same
> directory)

I was hoping somebody with knowledge of those tests could answer that so I would not have to read everything ;-)

Comment 12

[Hallvord R. M. Steen [:hallvors]]

(That said, having looked at all file names and opened some of them I didn't find equivalent tests)

Comment 13

[Joel Maher ( :jmaher ) (UTC -8)]

I would ask for a review from someone who has reviewed similar tests.  Also push to try and ensure it works as you expect.

Comment 14

[Hallvord R. M. Steen [:hallvors]]

Try server output: https://tbpl.mozilla.org/?tree=Try&rev=0b7b1e16c29c
There is one entry here saying the test ran ok on Fx Android: https://tbpl.mozilla.org/php/getParsedLog.php?id=38313881&tree=Try&full=1 but I don't find an easy way to find all results for this test across builds.

Comment 15

[Hallvord R. M. Steen [:hallvors]]

Ms2ger: Sorry for needinfo'ing you without comment - I meant to ask for a review since you've reviewed some of the other tests here.

Comment 16

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I'd much rather you wrote testharness.js tests, fwiw ;)

Comment 17

[Hallvord R. M. Steen [:hallvors]]

Well, given that I know testharness.js much better than this mochitest thingy I'm perfectly happy to do so :-p - how/where? Do testharness.js test suites live in mozilla-central repo?

Comment 18

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Just include /resources/testharness.js and /resources/testharnessreport.js, and put the test in a mochitest-plain manifest (like you did here).

Comment 19

[Hallvord R. M. Steen [:hallvors]]

Attachment: bug_377413_test_2.patch

Does that help?

Comment 20

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 8419906 [details] [diff] [review]
bug_377413_test_2.patch

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

LGTM, thanks, and sorry for forgetting about your needinfo.

::: content/html/content/test/forms/test_377413.html
@@ +29,5 @@
> +test(function(){
> +    tb.innerHTML = '<tr><td><input name="fooboo"></td></tr>';
> +    document.forms[0].fooboo.value = 'testme';
> +    document.getElementsByTagName('table')[0].deleteRow(0);
> +    assert_true(typeof document.forms[0].fooboo === 'undefined')

assert_equals(document.forms[0].fooboo, undefined);

@@ +44,5 @@
> +}, 'element name has created property on form');
> +
> +test(function(){
> +    tb.innerHTML = '';
> +    assert_true(! ( 'boofoo' in document.forms[0]))

assert_false

Comment 21

[Hallvord R. M. Steen [:hallvors]]

Attachment: bug_377413_test_2.patch

Review comments addressed now :)

Comment 22

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Gah, looks like I missed the fact that you didn't add the test to the mochitest.ini in that dir. Please fix that and update the commit message.

Comment 23

[Hallvord R. M. Steen [:hallvors]]

Attachment: bug_377413_test_2.patch

D'oh

Comment 24

[Hallvord R. M. Steen [:hallvors]]

Attachment: bug_377413_test_2.patch

and now with a better description..

Comment 25

[Hallvord R. M. Steen [:hallvors]]

Should I reopen this bug to make sure people see the checkin-needed keyword?

Comment 26

[rsx11m]

As a minimum, you shouldn't resolve a bug as WFM when there is actually work done. Maybe you should modify the summary to reflect that you are just adding tests for this without modifying the code?

Comment 27

[Hallvord R. M. Steen [:hallvors]]

I guess it would have been better to open another bug saying "add tests.."? 

Anyway, I had no idea contributing a simple test here would take so much time when I decided to use this bug to learn how to add tests. Hence I didn't re-open or report a new one. The real lesson I take away is: don't give Gecko tests, add them to W3C's web-platform-tests instead :-p

Comment 28

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

I've got it in my queue.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Based on comment 28, I'm assuming Ms2ger is going to take care of pushing this. Clearing checkin-needed to get it off our radar.

Comment 30

[rsx11m]

(In reply to Hallvord R. M. Steen from comment #27)
> I guess it would have been better to open another bug saying "add tests.."? 

In general it's ok to retarget a bug in this way, thus no problem. However, you were sending mixed messages by resolving the bug, basically saying it's not a problem, and then you kept going.

Comment 31

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/999ef6a46be8

Comment 32

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/999ef6a46be8