1419334 - The check added in bug 1418560 for fallback content may not be that right in presence of dynamic changes.

Status: RESOLVED FIXED

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

Description

[Emilio Cobos Álvarez [:emilio]]

We test it when the node doesn't have the MAY_BE_IN_BINDING_MNGR flag, but the flag is not precise, so in presence of dynamic changes that somehow make an element unassigned, like changing the binding, it may not be correct.

Comment 1

[Emilio Cobos Álvarez [:emilio]]

Attachment: Bug 1419334: Fix GetFlattenedTreeParentNodeInternal test for unassigned XBL children nodes.

We test it when the node doesn't have the MAY_BE_IN_BINDING_MNGR flag, but the
flag is not precise, so in presence of dynamic changes that somehow make an
element unassigned, like changing the binding, it may not be correct.

Review commit: https://reviewboard.mozilla.org/r/201574/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/201574/

Comment 2

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

Comment on attachment 8930408 [details]
Bug 1419334: Fix GetFlattenedTreeParentNodeInternal test for unassigned XBL children nodes.

https://reviewboard.mozilla.org/r/201574/#review206864

::: commit-message-6d760:3
(Diff revision 1)
> +Bug 1419334: Fix GetFlattenedTreeParentNodeInternal test for XBL fallback content in presence of some dynamic changes. r?smaug
> +
> +We test it when the node doesn't have the MAY_BE_IN_BINDING_MNGR flag, but the

I don't understand the comment.

::: dom/base/FragmentOrElement.cpp:255
(Diff revision 1)
>        return nullptr;
>      }
>      parent = destInsertionPoints->LastElement()->GetParent();
>      MOZ_ASSERT(parent);
> -  } else if (HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) {
> +  } else if (HasFlag(NODE_MAY_BE_IN_BINDING_MNGR) ||
> +             parent->HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) {

Why do we need this? Looks like a bug elsewhere. What is failing without this?

Comment 3

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8930408 [details]
Bug 1419334: Fix GetFlattenedTreeParentNodeInternal test for unassigned XBL children nodes.

https://reviewboard.mozilla.org/r/201574/#review206864

> I don't understand the comment.

I mean that right now we do something like:

```
if (MayBeInBindingManager()) {
    // .. Check insertion point
} else if (ParentHasActiveBindingWithContent()) {
    // .. is fallback content not bound to any insertion point.
}
```

But nothing makes `MAY_BE_IN_BINDING_MNGR` necessarily not being set for fallback content not bound to any insertion point (if that content used to be assigned before in a previous binding).

> Why do we need this? Looks like a bug elsewhere. What is failing without this?

See above, this is trying to put both XBL checks in the same block just for convenience. This could be:

```
if (MayBeInBindingManager() && GetXBLInsertionPoint()) {
  parent = insertion point parent
}

if (parent->MayBeInBindingManager() &&
    parent->OwnerDoc()->BindingManager()->...(parent) &&
    !GetXBLInsertionPoint()) {
  // Fallback content not assigned to any insertion point.
  return nullptr;
}
```

That looked like more complex / slower to me, but happy to do that instead if you prefer.

Comment 4

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8930408 [details]
Bug 1419334: Fix GetFlattenedTreeParentNodeInternal test for unassigned XBL children nodes.

(Per above comments, I don't think my patch is incorrect. Happy to rework how it is expressed though)

Comment 5

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

Comment on attachment 8930408 [details]
Bug 1419334: Fix GetFlattenedTreeParentNodeInternal test for unassigned XBL children nodes.

https://reviewboard.mozilla.org/r/201574/#review206872

ok, so fallback content should have the flag set.

Comment 6

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8930408 [details]
Bug 1419334: Fix GetFlattenedTreeParentNodeInternal test for unassigned XBL children nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201574/diff/1-2/

Comment 7

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

Comment on attachment 8930408 [details]
Bug 1419334: Fix GetFlattenedTreeParentNodeInternal test for unassigned XBL children nodes.

https://reviewboard.mozilla.org/r/201574/#review206946

::: dom/base/FragmentOrElement.cpp:257
(Diff revision 2)
>      parent = destInsertionPoints->LastElement()->GetParent();
>      MOZ_ASSERT(parent);
> -  } else if (HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) {
> +  } else if (HasFlag(NODE_MAY_BE_IN_BINDING_MNGR) ||
> +             parent->HasFlag(NODE_MAY_BE_IN_BINDING_MNGR)) {
> +    // We need to check `parent` to properly handle the unassigned child case
> +    // below, since if we were never assigned we would never have the flag set.

Could you add a comment here that the child may have the flag set if it was assigned at some point.

Comment 8

[Emilio Cobos Álvarez [:emilio]]

Comment on attachment 8930408 [details]
Bug 1419334: Fix GetFlattenedTreeParentNodeInternal test for unassigned XBL children nodes.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/201574/diff/2-3/

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c46274ce509e
Fix GetFlattenedTreeParentNodeInternal test for unassigned XBL children nodes. r=smaug

Comment 10

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/c46274ce509e