350754 - Crash [@ ntdll.dll][@ nsFrameManager::GetPrimaryFrameFor] with xbl testcase

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[Martijn Wargers (dead)]

See upcoming testcase, which crashes current trunk Mozilla builds when loading.
It also crashes Mozilla1.7, so no (recent) regression.
It seems like some kind of infinite recursion crash or something.

Talkback ID: TB22701395E
ntdll.dll + 0x10701 (0x7c910701)
MSVCR80.dll + 0x4ce9 (0x78134ce9)
0x06f06800

Talkback ID: TB22699388Z
PL_DHashTableOperate  [mozilla\xpcom\build\pldhash.c, line 537]
nsFrameManager::GetPrimaryFrameFor  [mozilla\layout\base\nsframemanager.cpp, line 350]
nsCSSFrameConstructor::FindPrimaryFrameFor  [mozilla\layout\base\nscssframeconstructor.cpp, line 11434]
nsFrameManager::GetPrimaryFrameFor  [mozilla\layout\base\nsframemanager.cpp, line 404]
nsCSSFrameConstructor::FindPrimaryFrameFor  [mozilla\layout\base\nscssframeconstructor.cpp, line 11434]
nsFrameManager::GetPrimaryFrameFor  [mozilla\layout\base\nsframemanager.cpp, line 404]
nsCSSFrameConstructor::FindPrimaryFrameFor  [mozilla\layout\base\nscssframeconstructor.cpp, line 11434]
nsFrameManager::GetPrimaryFrameFor  [mozilla\layout\base\nsframemanager.cpp, line 404]
nsCSSFrameConstructor::FindPrimaryFrameFor  [mozilla\layout\base\nscssframeconstructor.cpp, line 11434]
nsFrameManager::GetPrimaryFrameFor  [mozilla\layout\base\nsframemanager.cpp, line 404]
nsCSSFrameConstructor::FindPrimaryFrameFor  [mozilla\layout\base\nscssframeconstructor.cpp, line 11434]
nsFrameManager::GetPrimaryFrameFor  [mozilla\layout\base\nsframemanager.cpp, line 404]
nsCSSFrameConstructor::FindPrimaryFrameFor  [mozilla\layout\base\nscssframeconstructor.cpp, line 11434]
nsFrameManager::GetPrimaryFrameFor  [mozilla\layout\base\nsframemanager.cpp, line 404]
nsCSSFrameConstructor::FindPrimaryFrameFor  [mozilla\layout\base\nscssframeconstructor.cpp, line 11434]
nsFrameManager::GetPrimaryFrameFor  [mozilla\layout\base\nsframemanager.cpp, line 404]
nsCSSFrameConstructor::FindPrimaryFrameFor  [mozilla\layout\base\nscssframeconstructor.cpp, line 11434]
nsFrameManager::GetPrimaryFrameFor  [mozilla\layout\base\nsframemanager.cpp, line 404]
nsCSSFrameConstructor::FindPrimaryFrameFor  [mozilla\layout\base\nscssframeconstructor.cpp, line 11434]
etc

That last Talkback ID, I got with an external xbl document.

Comment 1

[Martijn Wargers (dead)]

Attachment: testcase

Comment 2

[Mats Palmgren (inactive)]

Attachment: Testcase #2 - mutual recursive binding

Comment 3

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

This fixes both testcases.

Comment 4

[Mats Palmgren (inactive)]

Attachment: Screenshot of the warning message

Comment 5

[Boris Zbarsky [:bzbarsky]]

Hmm...  It would make more sense to me to have the check in LoadBindings() instead of in the callers.

Also, why is the walk along the GetParent() chain starting at GetBindingParent() and not along the GetBindingParent() chain?

On a separate note, Martijn, what happens if two XBL bindings inherit from each other?  I suspect we don't deal well with that either; if so, separate bug?

Comment 6

[Mats Palmgren (inactive)]

Attachment: Patch rev. 2

Moved to nsXBLService::LoadBindings() and walking GetBindingParent() chain.

The first test:
  if (!bindingParent || aChild == bindingParent) {

is just an optimisation - we could just fall through and not enter
the loop. About 40% of the calls take the early return.
Let me know if you don't think it's worth it.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 236603 [details] [diff] [review]
Patch rev. 2

>Index: content/xbl/src/nsXBLService.cpp
>+IsAncestorBinding(nsIDocument* aDocument,

>+  if (!bindingParent || aChild == bindingParent) {

I think we can nix this optimization.  More below.

>+  nsCString spec1;
>+  nsresult rv = aChildBindingURI->GetSpec(spec1);

We shouldn't need the spec, imo.

>+    if (NS_SUCCEEDED(rv) && spec1.Equals(spec2)) {

I think we should use nsIURI::Equals here.  And if that fails, treat it as being equal, to be safe.  Then we wouldn't need spec1 or the optimization above.

With those nits this looks pretty good.

Comment 8

[Mats Palmgren (inactive)]

Attachment: Patch rev. 3

Nits fixed.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 237213 [details] [diff] [review]
Patch rev. 3

Looks great!

Comment 10

[Martijn Wargers (dead)]

I'm an idiot, I didn't even notice I filed an infinite recursion bug.
This is basically a duplicate of bug 55070.

(In reply to comment #5)
> On a separate note, Martijn, what happens if two XBL bindings inherit from each
> other?  I suspect we don't deal well with that either; if so, separate bug?

 xbl:inherits="style" is working fine with the patch. Using extends is still crashing with the patch, I attached a testcase for that to bug 55070.

Comment 11

[Martijn Wargers (dead)]

Mats, the patch has r+ and sr+, so it can be checked in. Are you waiting for something?

Comment 12

[Mats Palmgren (inactive)]

Checked in to trunk at 2006-10-08 03:18 PDT.

-> FIXED