604737 - nsHTMLFieldSetElement::RemoveChildAt doesn't update mFirstLegend correctly

Status: RESOLVED FIXED

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

Description

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

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.

Comment 1

[Boris Zbarsky [:bzbarsky]]

It will?  Why?

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

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

Yeah, mFirstLegend may not be cleared always.

Comment 3

[Mounir Lamouri (:mounir)]

Attachment: Patch v1

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Patch v2

Comment 5

[Olli Pettay [:smaug][[e-mail]]]

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;
      }
    }

Comment 6

[Mounir Lamouri (:mounir)]

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

Comment 7

[Mounir Lamouri (:mounir)]

Pushed:
http://hg.mozilla.org/mozilla-central/rev/af39f954a01f

Comment 8

[Daniel Veditz [:dveditz]]

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.