Closed Bug 1059030 Opened 10 years ago Closed 10 years ago

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

Categories

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

defect
Not set
critical

Tracking

()

VERIFIED FIXED
mozilla35
Tracking Status
firefox35 --- verified
firefox-esr31 35+ verified

People

(Reporter: Jamie, Assigned: agi)

References

Details

(4 keywords)

Crash Data

Attachments

(2 files, 1 obsolete file)

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.
OS: Windows 8.1 → All
Hardware: x86 → All
Summary: crash in nsAttrAndChildArray::IndexOfAttr(nsIAtom*, int) → crash navigating forms with <input type=radio> and <fieldset> [@ nsAttrAndChildArray::IndexOfAttr(nsIAtom*, int)]
Version: 31 Branch → Trunk
Attached file testcase
STR with the reduced testcase:
1. hit <Tab> to focus the first radio button
2. hit <Up-arrow>
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.
Component: Untriaged → DOM: Core & HTML
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.
Blocks: 901656
Keywords: regression
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!
Attachment #8500181 - Flags: review?(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.
Attachment #8500181 - Flags: review?(bzbarsky) → review+
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!
Assignee: nobody → agi.novanta
Attachment #8500181 - Attachment is obsolete: true
Attachment #8500845 - Flags: review?(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
Attachment #8500845 - Flags: review?(bzbarsky) → review+
> 2078   } while (radio != currentRadio);

Hmm.  I guess that's equivalent, yeah.  I'm fine sticking with what you have, though.
https://hg.mozilla.org/mozilla-central/rev/9e7dff9e4404
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
[Tracking Requested - why for this release]:

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

Can you uplift this to ESR31?
Flags: needinfo?(agi.novanta)
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.
Flags: needinfo?(agi.novanta)
Attachment #8500845 - Flags: approval-mozilla-esr31?
Attachment #8500845 - Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Verified that the patch applies cleanly.
Keywords: checkin-needed
You need to log in before you can comment on or make changes to this bug.