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 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 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 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 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 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.

Pushed by ecoal95@gmail.com: https://hg.mozilla.org/integration/autoland/rev/c46274ce509e Fix GetFlattenedTreeParentNodeInternal test for unassigned XBL children nodes. r=smaug