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.

This fixes both testcases.

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?

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

Nits fixed.

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

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.

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

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