1016415 - Potential for crash with an empty <menugroup>

Status: VERIFIED FIXED

(Firefox :: Menus, defect)

Description

[Jared Wein [:jaws] (please needinfo? me)]

See https://hg.mozilla.org/integration/fx-team/rev/df4be0b6c018#l4.13

If the <menugroup> is empty, then the first child will be null, and line 4.16 will dereference it.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Patch and test

I verified that the test causes a crash without the patch applied.

Comment 2

[Neil Deakin]

Comment on attachment 8429468 [details] [diff] [review]
Patch and test

>   for (nsCOMPtr<nsIContent> grandChild = aPopup->GetFirstChild();
>        grandChild;
>        grandChild = grandChild->GetNextSibling()) {
>     if (grandChild->IsXUL(nsGkAtoms::menugroup)) {
>+      if (grandChild->GetChildCount() == 0) {
>+        continue;
>+      }
>       grandChild = grandChild->GetFirstChild();

It would be simpler to just use:

  if (!grandChild)
    continue;

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

We can't use `if (!grandChild) continue;` because at that point we wouldn't be able to do `grandChild = grandChild->GetNextSibling()` at the beginning of the next iteration of the loop.

Comment 4

[Neil Deakin]

True. I misread that, so ignore my comment.

Comment 5

[Ryan VanderMeulen [:RyanVM]]

Do you have a link to a recent Try run by chance? :)

Comment 6

[Jared Wein [:jaws] (please needinfo? me)]

https://tbpl.mozilla.org/?tree=Try&rev=e51413266ab8

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/7634c7b7e9e9

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/7634c7b7e9e9