374547 - regression: unable to repeat xbl bound element inside another with the same binding

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[aaronr]

Attachment: testcase

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.

Comment 1

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

Attachment: proposed patch

I think recursion depth 20 should be enough.

Comment 2

[Johnny Stenback (:jst)]

Comment on attachment 259042 [details] [diff] [review]
proposed patch

Looks good to me, but either Jonas or Boris should sr this one.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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.

Comment 4

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

(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? ;)

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

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

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

Comment 7

[Boris Zbarsky [:bzbarsky]]

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 8

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

Attachment: v2

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 260111 [details] [diff] [review]
v2

sr=bzbarsky

Comment 10

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

Attachment: mochitest

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 11

[Boris Zbarsky [:bzbarsky]]

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.