Closed
Bug 237357
Opened 21 years ago
Closed 21 years ago
Crash with html labels who are cross-referencing each other.
Categories
(Core :: DOM: UI Events & Focus Handling, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: martijn.martijn, Assigned: rp.moz)
Details
Attachments
(2 files, 3 obsolete files)
|
152 bytes,
patch
|
Details | Diff | Splinter Review | |
|
1.43 KB,
patch
|
jst
:
review+
dbaron
:
superreview+
chofmann
:
approval1.7b+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040302 Firefox/0.8.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7b) Gecko/20040302 Firefox/0.8.0+
Perhaps, this bug falls in the category silly, but this crashes Mozilla
consistently.
This is the html code:
<html>
<head>
<title>Untitled</title>
<style type="text/css">
#dit,#dat{-moz-user-focus:normal;}
</style>
</head>
<body>
<label for="dat" id="dit">dat</label><label for="dit" id="dat">dit</label>
</body>
</html>
I will upload a testcase
Reproducible: Always
Steps to Reproduce:
1.Visit testcase
2.Click on 'dit' or 'dat'
3.
Actual Results:
Mozilla disapperas, most likely crashing.
Expected Results:
Not crash at least.
|
Reporter | |
Comment 1•21 years ago
|
||
|
Assignee | |
Comment 2•21 years ago
|
||
Confirming with Mozilla 1.7a under WinXP.
It crashes with a stack overflow. It calls SetFocus with 'this' alternating
between 0x02ebfca0 and 0x02ebaff0.
Stacktrace:
nsString::GetWritableFragment(nsWritableFragment<unsigned short> & {...},
nsFragmentRequest kNextFragment, unsigned int 0) line 133 + 6 bytes
nsWritingIterator<unsigned short>::normalize_forward() line 420 + 39 bytes
nsWritingIterator<unsigned short>::advance(int 3) line 343
nsWritingIterator<unsigned short>::write(const unsigned short * 0x02ebfd64,
unsigned int 3) line 369
nsCharSinkTraits<nsWritingIterator<unsigned short>
>::write(nsWritingIterator<unsigned short> & {...}, const unsigned short *
0x02ebfd64, unsigned int 3) line 569
copy_string(nsReadingIterator<unsigned short> & {...}, const
nsReadingIterator<unsigned short> & {...}, nsWritingIterator<unsigned short> &
{...}) line 90 + 39 bytes
nsAString::UncheckedAssignFromReadable(const nsAString & {...}) line 295 + 44 bytes
nsAString::do_AssignFromReadable(const nsAString & {...}) line 265
nsAString::Assign(const nsAString & {...}) line 258 + 22 bytes
nsAString::operator=(const nsAString & {...}) line 264 + 19 bytes
nsAttrValue::ToString(nsAString & {...}) line 207 + 26 bytes
nsGenericHTMLElement::GetAttr(int 0, nsIAtom * 0x002ee3f0, nsAString & {...})
line 1937
nsHTMLLabelElement::GetHtmlFor(nsHTMLLabelElement * const 0x02ebfcc8, nsAString
& {...}) line 297
nsHTMLLabelElement::GetForContent() line 476 + 32 bytes
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 419 + 12 bytes
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
nsHTMLLabelElement::SetFocus(nsIPresContext * 0x02c725f8) line 422
...
...
... (hundreds of these)
Severity: normal → critical
Status: UNCONFIRMED → NEW
Ever confirmed: true
|
|
Assignee | |
Comment 3•21 years ago
|
||
Note that the comment in the source code seems to be untrue. It mentions that
"we don't have -moz-user-focus: normal" but the testcase does have
-moz-user-focus: normal in it and SetFocus is triggered by a click, not by an
acceskey.
void
nsHTMLLabelElement::SetFocus(nsIPresContext* aContext)
{
// Since we don't have '-moz-user-focus: normal', the only time
// |SetFocus| will be called is when the accesskey is activated.
nsCOMPtr<nsIContent> content = GetForContent();
if (content)
content->SetFocus(aContext);
}
|
|
Assignee | |
Comment 4•21 years ago
|
||
I think the easiest way to prevent this crash is to avoid letting
nsHTMLLabelElement::SetFocus set the focus to another label element.
Would that be allowed by the W3 specs?
|
|
Reporter | |
Comment 5•21 years ago
|
||
I think that would be allowed.
Labels should be associated with form controls according to:
http://www.w3.org/TR/html401/interact/forms.html#h-17.9
When I look at the types of form controls there are:
http://www.w3.org/TR/REC-html40/interact/forms.html#h-17.2.1
I see nothing mentioned of being the label element a form control.
I see now that you don't even need the moz-user-focus. It is still crashing
without the css. I see now moz-user-focus is not working on label elements.
|
|
Assignee | |
Comment 6•21 years ago
|
||
Changing summary based on comment #5
Summary: Crash with html labels with moz-user-focus: normal who are cross-referencing each other. → Crash with html labels who are cross-referencing each other.
|
|
Assignee | |
Comment 7•21 years ago
|
||
Reduced testcase (this will still crash when you click on the text!)
Attachment #143787 -
Attachment is obsolete: true
|
||
Comment 9•21 years ago
|
||
this service is brought you by mcsmurf ;) (you forgot to change the assigne field)
Assignee: events → r.pronk
Status: ASSIGNED → NEW
|
|
Assignee | |
Comment 10•21 years ago
|
||
Thanks mcsmurf!
This ought to do it.
|
|
Assignee | |
Updated•21 years ago
|
Attachment #143815 -
Flags: superreview?(dbaron)
Attachment #143815 -
Flags: review?(jst)
I think I'd prefer either:
(1) make nsHTMLLabelElement not claim to be an eHTML_FORM_CONTROL (I'm not sure
whether this would break anything), or
(2) [safer] Pull the two tests in nsHTMLLabelElement that check for that out
into their own function:
inline PRBool IsNonLabelFormControl(nsIContent *aContent)
{
return aContent->IsContentOfType(nsIContent::eHTML_FORM_CONTROL) &&
aContent->Tag() != nsHTMLAtoms::label;
}
Those would change what |GetForContent| returns.
|
|
Assignee | |
Comment 12•21 years ago
|
||
You mean like this?
Attachment #143815 -
Attachment is obsolete: true
|
|
Assignee | |
Updated•21 years ago
|
Attachment #143815 -
Flags: superreview?(dbaron)
Attachment #143815 -
Flags: review?(jst)
|
|
Assignee | |
Updated•21 years ago
|
Attachment #143886 -
Flags: superreview?(dbaron)
Attachment #143886 -
Flags: review?(jst)
Comment on attachment 143886 [details] [diff] [review]
new patch
Yes, except don't make it a member function -- and some compilers will give an
error if you declare a function inline but don't define it before its first use
-- so just declare and define at the same point (non-member).
Attachment #143886 -
Flags: superreview?(dbaron) → superreview-
|
|
Assignee | |
Updated•21 years ago
|
Attachment #143889 -
Flags: superreview?(dbaron)
Attachment #143889 -
Flags: superreview?(dbaron) → superreview+
|
|
Assignee | |
Updated•21 years ago
|
Attachment #143886 -
Flags: review?(jst)
|
|
Assignee | |
Updated•21 years ago
|
Attachment #143889 -
Flags: review?(jst)
|
||
Comment 15•21 years ago
|
||
Comment on attachment 143889 [details] [diff] [review]
new^2 patch
r=jst
Attachment #143889 -
Flags: review?(jst) → review+
|
|
Assignee | |
Comment 16•21 years ago
|
||
Let's see if drivers still want this for 1.7b.
I think the risk is fairly low and it stops a crash from happening.
sr=dbaron
r=jst
Flags: blocking1.7b?
Comment on attachment 143889 [details] [diff] [review]
new^2 patch
You want an approval request, not a blocking request.
Attachment #143889 -
Flags: approval1.7b?
Flags: blocking1.7b?
|
||
Comment 18•21 years ago
|
||
Comment on attachment 143889 [details] [diff] [review]
new^2 patch
a=chofmann for 1.7b
Attachment #143889 -
Flags: approval1.7b? → approval1.7b+
|
|
Assignee | |
Comment 19•21 years ago
|
||
Great! dbaron, could you check this in for me please? I have no cvs rights. Thanks.
Fix checked in to trunk, 2004-03-15 14:28 -0800.
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
|
||
Updated•6 years ago
|
Component: Event Handling → User events and focus handling
You need to log in
before you can comment on or make changes to this bug.
Description
•