nsHTMLFieldSetElement::RemoveChildAt doesn't update mFirstLegend correctly

RESOLVED FIXED in mozilla2.0b7

Status

()

RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Ms2ger, Assigned: mounir)

Tracking

Trunk
mozilla2.0b7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

(Whiteboard: [sg:dos?])

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

8 years ago
nsresult
nsHTMLFieldSetElement::RemoveChildAt(PRUint32 aIndex, PRBool aNotify,
                                     PRBool aMutationEvent /* = PR_TRUE */)
{
  bool firstLegendHasChanged = false;

  if (GetChildAt(aIndex) == mFirstLegend) {
    // If we are removing the first legend we have to found another one.
    for (nsIContent* child = mFirstLegend; child;
         child = child->GetNextSibling()) {
      if (child->IsHTML(nsGkAtoms::legend)) {
        mFirstLegend = child;
        firstLegendHasChanged = true;
        break;
      }
    }
  }

This loop will always exit in the first iteration without changing mFirstLegend.
(Reporter)

Updated

8 years ago
blocking2.0: --- → ?
It will?  Why?

Or do you mean that if there's no next legend it won't set mFirstLegend to null?

Comment 2

8 years ago
Yeah, mFirstLegend may not be cleared always.
(Assignee)

Updated

8 years ago
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
(Assignee)

Comment 3

8 years ago
Created attachment 483869 [details] [diff] [review]
Patch v1
Attachment #483869 - Flags: review?(Olli.Pettay)
(Assignee)

Comment 4

8 years ago
Created attachment 484025 [details] [diff] [review]
Patch v2
Attachment #483869 - Attachment is obsolete: true
Attachment #484025 - Flags: review?(Olli.Pettay)
Attachment #483869 - Flags: review?(Olli.Pettay)

Comment 5

8 years ago
Comment on attachment 484025 [details] [diff] [review]
Patch v2

This could be a bit shorter:

  if (mFirstLegend && GetChildAt(aIndex) == mFirstLegend) {
    // If we are removing the first legend we have to found another one.
    nsIContent* child = mFirstLegend->GetNextSibling();
    mFirstLegend = nsnull;
    firstLegendHasChanged = true;
    for (; child; child = child->GetNextSibling()) {
      if (child->IsHTML(nsGkAtoms::legend)) {
        mFirstLegend = child;
        break;
      }
    }
Attachment #484025 - Flags: review?(Olli.Pettay) → review+

Updated

8 years ago
blocking2.0: ? → betaN+
Whiteboard: [sg:critical]
(Assignee)

Comment 6

8 years ago
I don't think that's critical actually given that no method is called on mFirstLegend. The value is only compared against other values.
(Assignee)

Comment 7

8 years ago
Pushed:
http://hg.mozilla.org/mozilla-central/rev/af39f954a01f
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
Target Milestone: mozilla2.0b8 → mozilla2.0b7
I double-checked comment 6 (and asked Olli to as well) and I agree: the worst that might happen here is a crash trying to read memory we no longer own, but that's pretty benign.
Group: core-security
Whiteboard: [sg:critical] → [sg:dos?]
You need to log in before you can comment on or make changes to this bug.