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

RESOLVED FIXED

Status

()

Core
XBL
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: aaronr, Assigned: smaug)

Tracking

(Depends on: 1 bug)

Trunk
x86
Windows XP
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

11 years ago
Created attachment 259037 [details]
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.
(Reporter)

Updated

11 years ago
Blocks: 356790
(Assignee)

Updated

11 years ago
Blocks: 350754
(Assignee)

Updated

11 years ago
Assignee: general → Olli.Pettay
(Assignee)

Comment 1

11 years ago
Created attachment 259042 [details] [diff] [review]
proposed patch

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.
(Assignee)

Comment 4

11 years ago
(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.  ;)
(Assignee)

Comment 6

11 years ago
(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.
(Assignee)

Comment 8

11 years ago
Created attachment 260111 [details] [diff] [review]
v2
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+
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Flags: in-testsuite?
(Assignee)

Comment 10

11 years ago
Created attachment 260273 [details] [diff] [review]
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).
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.