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)
Core
DOM: Core & HTML
Tracking
()
VERIFIED
FIXED
mozilla35
People
(Reporter: Jamie, Assigned: agi)
References
Details
(4 keywords)
Crash Data
Attachments
(2 files, 1 obsolete file)
479 bytes,
text/html
|
Details | |
4.32 KB,
patch
|
bzbarsky
:
review+
bkerensa
:
approval-mozilla-esr31+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Keywords: reproducible,
testcase
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
Comment 1•10 years ago
|
||
STR with the reduced testcase: 1. hit <Tab> to focus the first radio button 2. hit <Up-arrow>
Comment 2•10 years ago
|
||
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
Comment 3•10 years ago
|
||
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
Assignee | ||
Comment 4•10 years ago
|
||
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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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+
Comment 8•10 years ago
|
||
> 2078 } while (radio != currentRadio);
Hmm. I guess that's equivalent, yeah. I'm fine sticking with what you have, though.
Assignee | ||
Comment 9•10 years ago
|
||
Thanks bz! Try: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=29280bb7923c https://tbpl.mozilla.org/?tree=Try&rev=29280bb7923c
Flags: in-testsuite+
Keywords: checkin-needed
Comment 10•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/9e7dff9e4404
Keywords: checkin-needed
Comment 11•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/9e7dff9e4404
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla35
Comment 13•9 years ago
|
||
[Tracking Requested - why for this release]: [Tracking Requested - why for this release]:Crash on Esr31 Can you uplift this to ESR31?
Assignee | ||
Comment 14•9 years ago
|
||
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?
Updated•9 years ago
|
Attachment #8500845 -
Flags: approval-mozilla-esr31? → approval-mozilla-esr31+
Updated•9 years ago
|
Comment 17•9 years ago
|
||
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/
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•