Closed Bug 604737 Opened 9 years ago Closed 9 years ago

nsHTMLFieldSetElement::RemoveChildAt doesn't update mFirstLegend correctly

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla2.0b7
Tracking Status
blocking2.0 --- betaN+

People

(Reporter: Ms2ger, Assigned: mounir)

Details

(Whiteboard: [sg:dos?])

Attachments

(1 file, 1 obsolete file)

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.
blocking2.0: --- → ?
It will?  Why?

Or do you mean that if there's no next legend it won't set mFirstLegend to null?
Yeah, mFirstLegend may not be cleared always.
Assignee: nobody → mounir.lamouri
Status: NEW → ASSIGNED
Attached patch Patch v1 (obsolete) — Splinter Review
Attachment #483869 - Flags: review?(Olli.Pettay)
Attached patch Patch v2Splinter Review
Attachment #483869 - Attachment is obsolete: true
Attachment #484025 - Flags: review?(Olli.Pettay)
Attachment #483869 - Flags: review?(Olli.Pettay)
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+
blocking2.0: ? → betaN+
Whiteboard: [sg:critical]
I don't think that's critical actually given that no method is called on mFirstLegend. The value is only compared against other values.
Pushed:
http://hg.mozilla.org/mozilla-central/rev/af39f954a01f
Status: ASSIGNED → RESOLVED
Closed: 9 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.