Closed Bug 237357 Opened 20 years ago Closed 20 years ago

Crash with html labels who are cross-referencing each other.

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Platform:
x86
Windows XP
Type:
defect
Priority:
Not set
Severity:
critical

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: martijn.martijn, Assigned: rp.moz)

Details

Attachments

(2 files, 3 obsolete files)

Reduced testcase
152 bytes, patch
Details | Diff | Splinter Review
new^2 patch
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.
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
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);
}
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?
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.



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.
Attached patch Reduced testcase — — Splinter Review 
Reduced testcase (this will still crash when you click on the text!)
Attachment #143787 - Attachment is obsolete: true
Me, me, me! :)

Patch coming up.
Status: NEW → ASSIGNED
this service is brought you by mcsmurf ;) (you forgot to change the assigne field)
Assignee: events → r.pronk
Status: ASSIGNED → NEW
Attached patch Patch (obsolete) — — Splinter Review 
Thanks mcsmurf!

This ought to do it.
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.
Attached patch new patch (obsolete) — — Splinter Review 
You mean like this?
Attachment #143815 - Attachment is obsolete: true
Attachment #143815 - Flags: superreview?(dbaron)
Attachment #143815 - Flags: review?(jst)
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-
Attached patch new^2 patch — — Splinter Review 
Once again.
Attachment #143886 - Attachment is obsolete: true
Attachment #143889 - Flags: superreview?(dbaron)
Attachment #143889 - Flags: superreview?(dbaron) → superreview+
Attachment #143886 - Flags: review?(jst)
Attachment #143889 - Flags: review?(jst)
Comment on attachment 143889 [details] [diff] [review]
new^2 patch

r=jst
Attachment #143889 - Flags: review?(jst) → review+
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 on attachment 143889 [details] [diff] [review]
new^2 patch

a=chofmann for 1.7b
Attachment #143889 - Flags: approval1.7b? → approval1.7b+
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: 20 years ago
Resolution: --- → FIXED
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.