315859 - Crash when using any arrow key on a radio input enclosed in a fieldset, and radio name is the same than fieldset id [@ nsHTMLFormElement::GetNextRadioButton]

Status: VERIFIED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Hadrien Nilsson]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; fr; rv:1.8) Gecko/20051025 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; fr; rv:1.8) Gecko/20051025 Firefox/1.5

Crash when pressing any arrow key on a radio input who is having the focus, and whose name is the same than the id of a fieldset enclosing that radio input.

Reproducible: Always

Steps to Reproduce:
1.Open the test page
2.Click on the radio with the mouse in order to give the focus to it
3.Push any of the four arrow key on your keyboard

Actual Results:  
Crash (null address)

Expected Results:  
No crash :)

Comment 1

[Hadrien Nilsson]

Attachment: Test case to illustrate the crash

Comment 2

[Hadrien Nilsson]

Additional information to reproduce the crash :

The fieldset element must be enclosed in a form element, otherwise the crash does not occur.

Crashes Firefox 1.5 rc2, and also the latest nightly build of Deer Park :

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20051109 Firefox/1.6a1

Comment 3

[Martijn Wargers (dead)]

Confirmed, using: 
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20051109 Firefox/1.6a1

Talkback ID: TB11676318W

Comment 4

[Martijn Wargers (dead)]

Attachment: backtrace from debug build

This is a backtrace from the assertions I get before the crash and from the sigsegv itself.

Assertions:
###!!! ASSERTION: mRadioButtons holding a non-radio button: 'radio', file c:/moz
illa/mozilla/content/html/content/src/nsHTMLFormElement.cpp, line 1647

###!!! ASSERTION: You can't dereference a NULL nsCOMPtr with operator->().: 'mRa
wPtr != 0', file ../../dist/include/xpcom/nsCOMPtr.h, line 849

Sigsegv:
Program received signal SIGSEGV, Segmentation fault.
0x0549ee23 in nsHTMLFormElement::GetNextRadioButton(nsAString_internal const&, i
nt, nsIDOMHTMLInputElement*, nsIDOMHTMLInputElement**) (this=0xfaef890,

Comment 5

[Martijn Wargers (dead)]

Looking at blame:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/content/src/nsHTMLFormElement.cpp#1636
I think this could be a regression from bug 17602.

Comment 6

[[e-mail]]

nsHTMLFormElement's idea of what a radio group is buggy - it includes all form controls with the same name, whether or not they are radio elements, and then GetNextRadioButton assumes that they're at least all <INPUT> elements.

Comment 7

[Alfred Peng]

Attachment: Patch v1

aaron, is this a proper way to go?

Comment 8

[Mats Palmgren (inactive)]

Comment on attachment 226647 [details] [diff] [review]
Patch v1

Since the GetDisabled() call is now conditional you also need this change:
-  PRBool disabled;
+  PRBool disabled = PR_TRUE;

With that, r=mats

The first comment line inside GetNextRadioButton() is wrong, I think you
should just remove that line while we're here...

Comment 9

[Alfred Peng]

Attachment: Patch v2

Update the patch to address the last comments.

mats, thanks for your review.

Comment 10

[[e-mail]]

Comment on attachment 226755 [details] [diff] [review]
Patch v2

>-  PRBool disabled;
>+  PRBool disabled = PR_TRUE;
Actually you shouldn't need this change because we only use disabled immediately after the call to GetDisabled.

>     radioGroup->Item(index, getter_AddRefs(radioDOMNode));
>     radio = do_QueryInterface(radioDOMNode);
>-    NS_ASSERTION(radio, "mRadioButtons holding a non-radio button");
>+    if (!radio)
>+      continue;
>+
>+    formControl = do_QueryInterface(radio);
>+    if (formControl->GetType() != NS_FORM_INPUT_RADIO)
>+      continue;
>+
>     radio->GetDisabled(&disabled);
>   } while (disabled && radio != currentRadio);
Should we do the check for a form control first?

Comment 11

[Mats Palmgren (inactive)]

(In reply to comment #10)
> (From update of attachment 226755 [details] [diff] [review] [edit])
> >-  PRBool disabled;
> >+  PRBool disabled = PR_TRUE;
> Actually you shouldn't need this change because we only use disabled
> immediately after the call to GetDisabled.
> 

If either of the "continue" is executed the first time through the loop
we do need it, otherwise we use an uninitialized "disabled".
Not that it matters much, but I was thinking that tools like Coverity
will probably complain about it...

Comment 12

[Alfred Peng]

(In reply to comment #11)
> If either of the "continue" is executed the first time through the loop
> we do need it, otherwise we use an uninitialized "disabled".
> Not that it matters much, but I was thinking that tools like Coverity
> will probably complain about it...
> 

I think neil's point is that the statement "radio->GetDisabled(&disabled);" will initialize "disabled" before the check.

Comment 13

[Mats Palmgren (inactive)]

But you won't reach that statement if you execute a "continue".
(A "continue" inside a do-while jumps to the "while", not the "do".)

Comment 14

[Alfred Peng]

Attachment: Patch v3

Updated patch to address neil's review.

> >+    formControl = do_QueryInterface(radio);
> >+    if (formControl->GetType() != NS_FORM_INPUT_RADIO)
> >+      continue;
> >+
> >     radio->GetDisabled(&disabled);
> >   } while (disabled && radio != currentRadio);
> Should we do the check for a form control first?

Since the radio has been checked, can we deem formControl to be not null? Or it's still a necessary sanity check?

Comment 15

[Alfred Peng]

Attachment: Patch v4

(In reply to comment #13)
> But you won't reach that statement if you execute a "continue".
> (A "continue" inside a do-while jumps to the "while", not the "do".)
> 

oops, that's right. New patch for neil.

Comment 16

[[e-mail]]

(In reply to comment #13)
>But you won't reach that statement if you execute a "continue".
Ah, so it's a badly written loop, or at least it's not obvious enough :-P
Maybe it should be written as a for (;;) loop:
for (;;) {
  // update index
  // get next item
  // continue if it's not an input
  // break if it's the starting radio
  // continue if it's not a radio
  // break if it's not disabled
}

Comment 17

[Alfred Peng]

Attachment: Patch v1 for mozilla 1.8.1

Firefox 2.0 also crashes by the testcase. Make a corresponding patch for it.

Comment 18

[Mike Schroepfer]

Please land on trunk and let it bake a day or so before we approve for 1.8.1

Comment 19

[Leon Sha]

Checking in public/nsIRadioGroupContainer.h;
/cvsroot/mozilla/content/html/content/public/nsIRadioGroupContainer.h,v  <--  nsIRadioGroupContainer.h
new revision: 1.8; previous revision: 1.7
done
Checking in src/nsHTMLFormElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLFormElement.cpp,v  <--  nsHTMLFormElement.cpp
new revision: 1.203; previous revision: 1.202
done

Comment 20

[Alfred Peng]

=> Land to trunk

Comment 21

[Stephen Donner [:stephend] Not actively reading bugmail]

The crash is verified FIXED using https://bugzilla.mozilla.org/attachment.cgi?id=202507 as the testcase with trunk SeaMonkey 1.5a;Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060630 SeaMonkey/1.5a

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 227410 [details] [diff] [review]
Patch v1 for mozilla 1.8.1

a=dbaron on behalf of drivers.  Please land on MOZILLA_1_8_BRANCH and mark fixed 1.8.1 when you have.

Also, please make sure there's a bug filed on comment 6.

Comment 23

[Leon Sha]

Checking in public/nsIRadioGroupContainer.h;
/cvsroot/mozilla/content/html/content/public/nsIRadioGroupContainer.h,v  <--  nsIRadioGroupContainer.h
new revision: 1.5.8.1; previous revision: 1.5
done
Checking in src/nsHTMLFormElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLFormElement.cpp,v  <--  nsHTMLFormElement.cpp
new revision: 1.186.4.2; previous revision: 1.186.4.1
done

Comment 24

[Alfred Peng]

=> Land on MOZILLA_1_8_BRANCH

New bug 343444 filed to address comment 6.

Comment 25

[Daniel Veditz [:dveditz]]

Comment on attachment 227410 [details] [diff] [review]
Patch v1 for mozilla 1.8.1

approved for 1.8.0 branch, a=dveditz for drivers

Comment 26

[Alfred Peng]

Checking in content/html/content/src/nsHTMLFormElement.cpp;
/cvsroot/mozilla/content/html/content/src/nsHTMLFormElement.cpp,v  <--  nsHTMLFormElement.cpp
new revision: 1.186.10.1; previous revision: 1.186
done
Checking in content/html/content/public/nsIRadioGroupContainer.h;
/cvsroot/mozilla/content/html/content/public/nsIRadioGroupContainer.h,v  <--  nsIRadioGroupContainer.h
new revision: 1.5.16.1; previous revision: 1.5
done


=> Land on 1.8.0 branch.

Comment 27

[Adam Guthrie]

Verified FIXED using 1.8 and 1.8.0 on Linux (X11; U; Linux i686; en-US;). I no longer crash using the testcase in attachment 202507 [details].

Comment 28

[Erik Quaeghebeur]

(In reply to Adam Guthrie from comment #27)
> Verified FIXED using 1.8 and 1.8.0 on Linux (X11; U; Linux i686; en-US;). I
> no longer crash using the testcase in attachment 202507 [details].

I encounter this bug with Firefox 31.3.0 (ESR) on Linux. The testcase crashes my browser. I am prepared to provide further details if given clear instructions.

Comment 29

[Martijn Wargers (dead)]

(In reply to Erik Quaeghebeur from comment #28)
> I encounter this bug with Firefox 31.3.0 (ESR) on Linux. The testcase
> crashes my browser. I am prepared to provide further details if given clear
> instructions.

Erik, could you please file a new bug on this and mention the bug number here? Thanks.

Comment 30

[Erik Quaeghebeur]

(In reply to Martijn Wargers [:mwargers] (QA) from comment #29)
>
> Erik, could you please file a new bug on this and mention the bug number
> here? Thanks.

Bug 1115981 (But why make me do the seemlessly unnecessary extra administrative effort and not just reopen this bug?)

Comment 31

[Martijn Wargers (dead)]

(In reply to Erik Quaeghebeur from comment #30)
> (In reply to Martijn Wargers [:mwargers] (QA) from comment #29)
> >
> > Erik, could you please file a new bug on this and mention the bug number
> > here? Thanks.
> 
> Bug 1115981 (But why make me do the seemlessly unnecessary extra
> administrative effort and not just reopen this bug?)

Because it's a different bug. This was fixed a long time ago, already.