An XForms user reported this bug when they tried to use nested xforms:repeat elements. I've simplified the testcase such that it no longer requires XForms to recreate. Basically, it looks like if you have an element foo:bar that has an xbl binding associated with it and then in the anonymous content introduce another foo:bar element, then things stop working and an error will be reported to the error console: Warning: The XBL binding "file:///D:/myprogs/xml/xbl/repeat.xhtml#redDiv" is already used by an ancestor element Source File: file:///D:/myprogs/xml/xbl/repeat.xhtml Line: 0 The patch for bug 350754 introduced the function IsAncestorBinding() to nsXBLService.cpp to battle recursion. It is this function that is reporting the error. This bug doesn't occur on the branches.

I think recursion depth 20 should be enough.

Comment on attachment 259042 [details] [diff] [review] proposed patch Looks good to me, but either Jonas or Boris should sr this one.

So in this case, what actually terminates the recursion? As for the patch, I'd prefer: The XBL binding "%S" is already used by too many ancestor elements; not applying it to prevent infinite recursion. Oh, and you need to change the name of the key, not just the value, apparently. Yay our localization setup.

(In reply to comment #3) > So in this case, what actually terminates the recursion? The return clause after nsContentUtils::ReportToConsole http://lxr.mozilla.org/seamonkey/source/content/xbl/src/nsXBLService.cpp#136 > > As for the patch, I'd prefer: > > The XBL binding "%S" is already used by too many ancestor elements; not > applying > it to prevent infinite recursion. > > Oh, and you need to change the name of the key, not just the value, apparently. > Yay our localization setup. > OK, then I'll change those. Want to sr then? ;)

> The return clause after nsContentUtils::ReportToConsole No, no. I meant in the xforms testcase. It's not hitting the 20 levels of recursion you introduced, right? Why not? And yes, once my comments are addressed I'll sr. ;)

(In reply to comment #5) > I meant in the xforms testcase. It's not hitting the 20 levels of > recursion you introduced, right? Why not? > That testcase isn't using xforms, but xforms:repeat uses (or may use) similar structure. The testcase clones the child nodes of the "repeat" and child nodes may contain new "repeats" (nested repeats are allowed in xforms). So, there will be "repeats" in anonymous content which have "repeats" as their binding parent. The depth of the recursion depends just on the "templates" of the "repeats".

Hmm. So there's valid XForms content that we won't handle, but we judge it unlikely that it'll be used? OK, sounds reasonable.

Comment on attachment 260111 [details] [diff] [review] v2 sr=bzbarsky

Using data url because of bug 304684. Added the timeout to make sure that the test really fails without the fix, not just "hang" mochitest. And dest.textContent = bindingDepth; makes it easy see how deep nesting works (max 20).

Comment on attachment 260273 [details] [diff] [review] mochitest I think you should also test that nesting 21 deep fails. And please add comments to make it clear that the commented-out binding is the data: URI.