Closed Bug 374547 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: XBL, defect)

x86
Windows XP
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronr, Assigned: smaug)

References

Details

Attachments

(3 files, 1 obsolete file)

Attached file 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.
Blocks: 356790
Blocks: 350754
Assignee: general → Olli.Pettay
Attached patch proposed patch (obsolete) — Splinter Review
I think recursion depth 20 should be enough.
Attachment #259042 - Flags: superreview?(jst)
Attachment #259042 - Flags: review?(mats.palmgren)
Comment on attachment 259042 [details] [diff] [review]
proposed patch

Looks good to me, but either Jonas or Boris should sr this one.
Attachment #259042 - Flags: superreview?(jst)
Attachment #259042 - Flags: superreview?(jonas)
Attachment #259042 - Flags: review?(mats.palmgren)
Attachment #259042 - Flags: review+
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.
Attached patch v2Splinter Review
Attachment #259042 - Attachment is obsolete: true
Attachment #260111 - Flags: superreview?(bzbarsky)
Attachment #259042 - Flags: superreview?(jonas)
Comment on attachment 260111 [details] [diff] [review]
v2

sr=bzbarsky
Attachment #260111 - Flags: superreview?(bzbarsky) → superreview+
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Attached patch mochitestSplinter Review
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).
Attachment #260273 - Flags: review?(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.
Attachment #260273 - Flags: review?(bzbarsky) → review-
Depends on: 382956
You need to log in before you can comment on or make changes to this bug.