1059030 - crash navigating forms with <input type=radio> and <fieldset> [@ nsAttrAndChildArray::IndexOfAttr(nsIAtom*, int)]

Status: VERIFIED FIXED

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

Description

[James Teh [:Jamie]]

This bug was filed from the Socorro interface and is 
report bp-25ad241e-d0c5-4699-afff-848b72140827.
=============================================================

Str:
1. Open http://trac.edgewall.org/ticket/1
2. Click "Modify Ticket".
3. Focus the "leave" Action radio button.
4. Press up arrow.
Result: Crash!
Or:
3. Focus the "assign" Action radio button.
4. Press down arrow.
Result: Crash!

Tested in Firefox 31.0. Also occurs in Firefox 34.0a1 (2014-08-23); crash report: bp-cb097927-cd3e-4220-b99b-525b92140826.

An NVDA screen reader user reports this occurs in Windows 7 as well, but I don't have a crash ID.

Comment 1

[Nickolay_Ponomarev]

Attachment: testcase

STR with the reduced testcase:
1. hit <Tab> to focus the first radio button
2. hit <Up-arrow>

Comment 2

[Nickolay_Ponomarev]

The crash happens after http://hg.mozilla.org/mozilla-central/annotate/4b3d816c21fd/content/html/content/src/HTMLFormElement.cpp#l2077 calls ->Disabled() on NULL:

> frame #3: mozilla::dom::HTMLInputElement::Disabled(this=0x0000000000000000) const + 31 at HTMLInputElement.h:425
> frame #4: mozilla::dom::HTMLFormElement::GetNextRadioButton(this=0x0000000131442800, aName=0x00007fff5fbfc4d8, aPrevious=true, aFocusedRadio=0x000000012c7fc400, aRadioOut=0x00007fff5fbfc4d0) + 699 at HTMLFormElement.cpp:2077
> frame #6: mozilla::dom::HTMLInputElement::PostHandleEvent(this=0x000000012c7fc400, aVisitor=0x00007fff5fbfcaf8) + 4370 at HTMLInputElement.cpp:4042

The testcase has the @id of the <fieldset> matching the @name of the <input type=radio>:
> <fieldset id="action"><input name="action" ...
without it it doesn't crash.

Comment 3

[Nickolay_Ponomarev]

This is regression (the range I got so far is 2013-06-19 .. 2013-08-21).

I'm guessing it's from bug 901656: http://hg.mozilla.org/mozilla-central/rev/803d0a3d5307#l4.114
This change made `if (!radio) continue;` trip on NULL radio in the while() condition.

Comment 4

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Do not crash when the @id of a fieldset matches the @name of a radio

So the problem here is that we retrieve a list of elements which match the @name of the radio in their @id and @name. Then we try to cast these elements as HTMLInputElement in here 

radio = HTMLInputElement::FromContentOrNull(radioGroup->Item(index));

but then we don't check if we actually have a radio or not in here

} while ((radio->Disabled() && radio != currentRadio) || !isRadio);

checking if radio is null should work here. 

Not sure if we should also check if the @name matches to be sure we are dealing with the right radio controls, right now if a radio's @id matches another radiogroup's @name Gecko behaves weirdly.

bz, can you take a look? Thanks!

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8500181 [details] [diff] [review]
Do not crash when the @id of a fieldset matches the @name of a radio

> Not sure if we should also check if the @name matches to be sure we are
> dealing with the right radio controls, right now if a radio's @id matches
> another radiogroup's @name Gecko behaves weirdly.

Yeah, indeed.  We should be checking that the name matches here!

I'm OK with that happening either in this bug or in a followup, either way.

>     if (!radio)
>       continue;
> 
>     isRadio = radio->GetType() == NS_FORM_INPUT_RADIO;
>     if (!isRadio)
>       continue;

How about making this chunk be:

  isRadio = radio && radio->GetType() == NS_FORM_INPUT_RADIO;
  if (!isRadio)
    continue;

>+  } while (!radio || (radio->Disabled() && radio != currentRadio) || !isRadio);

And then this can be the slighly simpler:

  } while (!isRadio || (radio->Disabled() && radio != currentRadio));

Basically, have the single invariant that it's OK to work with "radio" as long as isRadio is true, instead of a more complicated set of rules.
  
r=me with that.

Comment 6

[[ex-Mozilla] Agi Sferro | :agi]

Attachment: Do not crash when the @id of a fieldset matches the @name of a radio

Let's do that! I'm reusing |isRadio| to check if the @name matches. Alternatively, we could get rid of the bool flag completely like this 

2059   do {
2060     if (aPrevious) {
2061       if (--index < 0) {
2062         index = numRadios -1;
2063       }
2064     }
2065     else if (++index >= (int32_t)numRadios) {
2066       index = 0;
2067     }
2068     radio = HTMLInputElement::FromContentOrNull(radioGroup->Item(index));
2069     if (!radio || radio->GetType() != NS_FORM_INPUT_RADIO) {
2070       continue;
2071     }
2072 
2073     nsAutoString name;
2074     radio->GetName(name);
2075     if (aName.Equals(name) && !radio->Disabled()) {
2076       break;
2077     }
2078   } while (radio != currentRadio);

it looks simpler to me but maybe we don't want to rewrite that. Thanks!

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8500845 [details] [diff] [review]
Do not crash when the @id of a fieldset matches the @name of a radio

Beautiful.  r=me

Comment 8

[Boris Zbarsky [:bzbarsky]]

> 2078   } while (radio != currentRadio);

Hmm.  I guess that's equivalent, yeah.  I'm fine sticking with what you have, though.

Comment 9

[[ex-Mozilla] Agi Sferro | :agi]

Thanks bz!

Try:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=29280bb7923c
https://tbpl.mozilla.org/?tree=Try&rev=29280bb7923c

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9e7dff9e4404

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9e7dff9e4404

Comment 13

[Alice0775 White]

[Tracking Requested - why for this release]:

[Tracking Requested - why for this release]:Crash on Esr31

Can you uplift this to ESR31?

Comment 14

[[ex-Mozilla] Agi Sferro | :agi]

Comment on attachment 8500845 [details] [diff] [review]
Do not crash when the @id of a fieldset matches the @name of a radio

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: Crash on some websites, it's a regression
User impact if declined: Firefox will crash if the @id of a fieldset matches the @name of a radio element
Fix Landed on Version: 35
Risk to taking this patch (and alternatives if risky): should not pose any risk, we added some checks to be sure we are dealing with the right kind of object.
String or UUID changes made by this patch: none

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.

Comment 15

[[ex-Mozilla] Agi Sferro | :agi]

Verified that the patch applies cleanly.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-esr31/rev/766a8abd842f

Comment 17

[Kamil Jozwiak [:kjozwiak]]

Reproduced the original issue using the following build:
* http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/31.0/win32/en-US/
** https://crash-stats.mozilla.com/report/index/9816f78b-196e-433a-b414-d1de32150218

Went through verification using the following buils:

fx38: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-02-18-03-02-28-mozilla-central/
fx37: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2015-02-18-00-50-13-mozilla-aurora/
fx36: http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/36.0b10-candidates/
fx31.5.0esr: http://ftp.mozilla.org/pub/mozilla.org/firefox/candidates/31.5.0esr-candidates/build2/
fx35.0.1: http://ftp.mozilla.org/pub/mozilla.org/firefox/releases/35.0.1/