224765 - [FIX][@ nsXBLService::LoadBindings]

Status: RESOLVED FIXED

(Core :: XBL, defect)

Description

[timeless]

nsXBLService::LoadBindings   4
		 34044 	 RESOLVED 	 FIXED 	 [e-mail] 	 M15 	 2000-04-25
		 90235 	 VERIFIED 	 FIXED 	 [e-mail] 	 mozilla1.0 	 2001-09-19 
BBID range: 24597610 - 24776573
Min/Max Seconds since last crash: 302 - 37945
Min/Max Runtime: 302 - 37945
Crash data range: 2003-10-20 to 2003-10-25
Build ID range: 2003102004 to 2003102404

Stack Trace: 

	 nsXBLService::LoadBindings
[c:/builds/seamonkey/mozilla/content/xbl/src/nsXBLService.cpp  line 563]
	 nsCSSFrameConstructor::ConstructFrameInternal
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp 
line 7129]
	 nsCSSFrameConstructor::ConstructFrame
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp 
line 7086]
	 nsCSSFrameConstructor::ContentInserted
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp 
line 9028]
	 nsCSSFrameConstructor::RecreateFramesForContent
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp 
line 11580]
	 nsCSSFrameConstructor::AttributeChanged
[c:/builds/seamonkey/mozilla/layout/html/style/src/nsCSSFrameConstructor.cpp 
line 10279]
	 StyleSetImpl::AttributeChanged
[c:/builds/seamonkey/mozilla/content/base/src/nsStyleSet.cpp  line 1690]
	 PresShell::AttributeChanged
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp  line 5283]
	 nsXULDocument::AttributeChanged
[c:/builds/seamonkey/mozilla/content/xul/document/src/nsXULDocument.cpp  line 1152]
	 nsXULElement::SetAttr
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp  line 2434]
	 nsXULElement::SetAttr
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp  line 2454]
	 nsXBLPrototypeBinding::AttributeChanged
[c:/builds/seamonkey/mozilla/content/xbl/src/nsXBLPrototypeBinding.cpp  line 499]
	 nsXBLBinding::AttributeChanged
[c:/builds/seamonkey/mozilla/content/xbl/src/nsXBLBinding.cpp  line 834]
	 nsXULElement::SetAttr
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp  line 2401]
	 nsXULElement::SetAttribute
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp  line 1230]
	 XPTC_InvokeByIndex
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp 
line 102]
	 XPCWrappedNative::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp  line 2019]
	 XPC_WN_CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp 
line 1270]
	 js_Invoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 914]
	 js_Interpret	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 2934]
	 js_Invoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 930]
	 js_InternalInvoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 1007]
	 js_InternalGetOrSet	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 1050]
	 js_SetProperty	[c:/builds/seamonkey/mozilla/js/src/jsobj.c  line 2760]
	 js_Interpret	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 2127]
	 js_Invoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 930]
	 js_InternalInvoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 1007]
	 JS_CallFunctionValue	[c:/builds/seamonkey/mozilla/js/src/jsapi.c  line 3573]
	 nsJSContext::CallEventHandler
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp  line 1300]
	 nsJSEventListener::HandleEvent
[c:/builds/seamonkey/mozilla/dom/src/events/nsJSEventListener.cpp  line 182]
	 nsXBLPrototypeHandler::ExecuteHandler
[c:/builds/seamonkey/mozilla/content/xbl/src/nsXBLPrototypeHandler.cpp  line 462]
	 nsXBLEventHandler::HandleEvent
[c:/builds/seamonkey/mozilla/content/xbl/src/nsXBLEventHandler.cpp  line 88]
	 nsEventListenerManager::HandleEventSubType
[c:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp  line
1423]
	 nsEventListenerManager::HandleEvent
[c:/builds/seamonkey/mozilla/content/events/src/nsEventListenerManager.cpp  line
1500]
	 nsXULElement::HandleDOMEvent
[c:/builds/seamonkey/mozilla/content/xul/content/src/nsXULElement.cpp  line 3168]
	 PresShell::HandleDOMEventWithTarget
[c:/builds/seamonkey/mozilla/layout/html/base/src/nsPresShell.cpp  line 6265]
	 nsPopupSetFrame::OnCreate
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsPopupSetFrame.cpp  line 601]
	 nsPopupSetFrame::ShowPopup
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsPopupSetFrame.cpp  line 346]
	 nsPopupBoxObject::ShowPopup
[c:/builds/seamonkey/mozilla/layout/xul/base/src/nsPopupBoxObject.cpp  line 194]
	 XPTC_InvokeByIndex
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp 
line 102]
	 XPCWrappedNative::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp  line 2019]
	 XPC_WN_CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp 
line 1270]
	 js_Invoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 914]
	 js_Interpret	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 2934]
	 js_Invoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 930]
	 nsXPCWrappedJSClass::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappedjsclass.cpp  line 1333]
	 nsXPCWrappedJS::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappedjs.cpp  line 429]
	 PrepareAndDispatch
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp 
line 119]
	 SharedStub
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcstubs.cpp 
line 147]
	 nsGlobalHistory::OnStartLookup
[c:/builds/seamonkey/mozilla/xpfe/components/history/src/nsGlobalHistory.cpp 
line 4096]
	 XPTC_InvokeByIndex
[c:/builds/seamonkey/mozilla/xpcom/reflect/xptcall/src/md/win32/xptcinvoke.cpp 
line 102]
	 XPCWrappedNative::CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednative.cpp  line 2019]
	 XPC_WN_CallMethod
[c:/builds/seamonkey/mozilla/js/src/xpconnect/src/xpcwrappednativejsops.cpp 
line 1270]
	 js_Invoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 914]
	 js_Interpret	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 2934]
	 js_Invoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 930]
	 js_InternalInvoke	[c:/builds/seamonkey/mozilla/js/src/jsinterp.c  line 1007]
	 JS_CallFunctionValue	[c:/builds/seamonkey/mozilla/js/src/jsapi.c  line 3573]
	 nsJSContext::CallEventHandler
[c:/builds/seamonkey/mozilla/dom/src/base/nsJSEnvironment.cpp  line 1300]
	 GlobalWindowImpl::RunTimeout
[c:/builds/seamonkey/mozilla/dom/src/base/nsGlobalWindow.cpp  line 5010]
	 GlobalWindowImpl::TimerCallback
[c:/builds/seamonkey/mozilla/dom/src/base/nsGlobalWindow.cpp  line 5367]
	 nsTimerImpl::Fire	[c:/builds/seamonkey/mozilla/xpcom/threads/nsTimerImpl.cpp 
line 383]
	 nsAppShell::Run	[c:/builds/seamonkey/mozilla/widget/src/windows/nsAppShell.cpp
 line 142]
	 nsAppShellService::Run
[c:/builds/seamonkey/mozilla/xpfe/appshell/src/nsAppShellService.cpp  line 484]
 
 	Source File : c:/builds/seamonkey/mozilla/content/xbl/src/nsXBLService.cpp
line : 563
     (24776573)	URL: my.yahoo.com
     (24776573)	Comments: typing in another address

 560                          NS_NewURI(getter_AddRefs(uri), spec.get());
 561                          PRBool equal;
 562                          if (NS_SUCCEEDED(uri->Equals(aURL, &equal)) && equal)
 563 hyatt      1.61            return NS_OK;

I believe NS_NewURI failed

Comment 1

[timeless]

Attachment: don't crash, follow else (should match old behavior since aURL succeeded)

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 134830 [details] [diff] [review]
don't crash, follow else (should match old behavior since aURL succeeded)

Maybe capturing NS_NewURI's rv and if (NS_FAILED(rv)) return rv; would be
better.

/be

Comment 3

[timeless]

Maybe, but I was trying to match the old control flow. Returning an error
doesn't match what happened before. I could add an assertion so that someone
could later track down the problem and deal w/ it. This bug is pure talkback
analysis. As I have not encountered this crash in my cvs build on windows (which
we all know catches lots of problems), it's quite likely I'll never catch it.

Comment 4

[Boris Zbarsky [:bzbarsky]]

imo the right fix here is to use URIs instead of strings throughout XBL... I
have a partial patch to that effect and should have a more complete one tonight.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: Something like this

This cleans up most of the URI stuff in XBL to use nsIURI instead of strings.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134887 [details] [diff] [review]
Something like this

bryner, what do you think?

Comment 7

[timeless]

Comment on attachment 134887 [details] [diff] [review]
Something like this

>RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLPrototypeBinding.cpp,v
>retrieving revision 1.78
>+nsXBLPrototypeBinding::nsXBLPrototypeBinding()
>   if (gRefCnt == 1) {
>     kAttrPool = new nsFixedSizeAllocator();
you crash here:
>     kAttrPool->Init("XBL Attribute Entries", kAttrBucketSizes, kAttrNumBuckets, kAttrInitialSize);
>     kInsPool = new nsFixedSizeAllocator();
and here:
>     kInsPool->Init("XBL Insertion Point Entries", kInsBucketSizes, kInsNumBuckets, kInsInitialSize);

>+nsXBLPrototypeBinding::Init(const nsACString& aID,
>+                            nsIXBLDocumentInfo* aInfo,
>+                            nsIContent* aElement)
>+{
before you reach here:
>+  if (!kAttrPool || !kInsPool) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

>RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLBinding.cpp,v
>retrieving revision 1.168
>@@ -517,17 +517,17 @@ nsXBLBinding::GenerateAnonymousContent()
>   content->GetAttr(kNameSpaceID_None, nsXBLAtoms::includes, includes);
>   if (!includes.IsEmpty()) {
>     nsCAutoString id;
>     mPrototypeBinding->GetID(id);
I've been cleaning up this:
>     nsCAutoString message("An XBL Binding with an id of ");
>     message += id;
>     message += " and found in the file ";
to this:
      nsCAutoString uri;
      mPrototypeBinding->GetDocURI()->GetSpec(uri);
      nsCAutoString
	message(NS_LITERAL_CSTRING("An XBL Binding with an id of ")
	      + id
	      + NS_LITERAL_CSTRING(" and found in the file ")
	      + uri
	      + NS_LITERAL_CSTRING(" is still using the deprecated\n<content
includes=\"\"> syntax! Use <children> instead!\n"));

>     NS_WARNING(message.get());

but you don't have to do that here, i probably even have a patch for this file
in some tree :).

Comment 8

[Boris Zbarsky [:bzbarsky]]

Ah, good point.  I will add null-checks to the constructor...

The XBL code in general assumes that OOM never happens.  It's pretty bad.  :(

Comment 9

[timeless]

yeah i know, i had patches to bullet proof xblservice.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134830 [details] [diff] [review]
don't crash, follow else (should match old behavior since aURL succeeded)

obsoleting this, since my patch kills that NS_NewURI call.

Comment 11

[Brian Ryner (not reading)]

Comment on attachment 134887 [details] [diff] [review]
Something like this

>Index: content/xbl/public/nsIXBLBinding.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/xbl/public/nsIXBLBinding.h,v
>retrieving revision 1.36
>diff -p -u -8 -r1.36 nsIXBLBinding.h
>--- content/xbl/public/nsIXBLBinding.h	15 Apr 2003 20:35:03 -0000	1.36
>+++ content/xbl/public/nsIXBLBinding.h	6 Nov 2003 04:34:17 -0000
>@@ -41,26 +41,27 @@
> 
>   Private interface to the XBL Binding
> 
> */
> 
> #ifndef nsIXBLBinding_h__
> #define nsIXBLBinding_h__
> 
>-#include "nsString.h"
> #include "nsISupports.h"
> #include "nsISupportsArray.h"
> 
> class nsIContent;
> class nsIDocument;
> class nsIDOMNodeList;
> class nsIScriptContext;
> class nsXBLPrototypeBinding;
> class nsVoidArray;
>+class nsIURI;
>+class nsACString;
> 

Hm, for some reason I thought certain compilers didn't like using forward
declarations with reference parameters.  I'm not asking you to change it now,
but be aware of that when checking in.

>Index: content/xbl/src/nsXBLDocumentInfo.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLDocumentInfo.h,v
>retrieving revision 1.4
>diff -p -u -8 -r1.4 nsXBLDocumentInfo.h
>--- content/xbl/src/nsXBLDocumentInfo.h	6 Mar 2003 23:59:14 -0000	1.4
>+++ content/xbl/src/nsXBLDocumentInfo.h	6 Nov 2003 04:34:18 -0000
>@@ -43,35 +43,34 @@
> class nsXBLPrototypeBinding;
> class nsObjectHashtable;
> 
> class nsXBLDocumentInfo : public nsIXBLDocumentInfo, public nsIScriptGlobalObjectOwner, public nsSupportsWeakReference
> {
> public:
>   NS_DECL_ISUPPORTS
>   
>-  nsXBLDocumentInfo(const char* aDocURI, nsIDocument* aDocument);
>+  nsXBLDocumentInfo(nsIDocument* aDocument);
>   virtual ~nsXBLDocumentInfo();
>   
>-  NS_IMETHOD GetDocument(nsIDocument** aResult) { *aResult = mDocument; NS_IF_ADDREF(*aResult); return NS_OK; };
>+  NS_IMETHOD GetDocument(nsIDocument** aResult) { NS_ADDREF(*aResult = mDocument); return NS_OK; };

I guess this is ok to assume mDocument != null since we would crash in
NS_NewXBLDocumentInfo now if that was ever the case.

>Index: content/xbl/src/nsXBLPrototypeBinding.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLPrototypeBinding.h,v
>retrieving revision 1.31
>diff -p -u -8 -r1.31 nsXBLPrototypeBinding.h
>--- content/xbl/src/nsXBLPrototypeBinding.h	11 Sep 2003 12:23:55 -0000	1.31
>+++ content/xbl/src/nsXBLPrototypeBinding.h	6 Nov 2003 04:34:19 -0000
>@@ -69,19 +70,19 @@ class nsIXBLBinding;
> // binding table (with the XUL cache disabled).
> 
> class nsXBLPrototypeBinding
> {
> public:
>   already_AddRefed<nsIContent> GetBindingElement();
>   void SetBindingElement(nsIContent* aElement);
> 
>-  nsresult GetBindingURI(nsCString& aResult);
>-  nsresult GetDocURI(nsCString& aResult);
>-  nsresult GetID(nsCString& aResult) { aResult = mID; return NS_OK; }
>+  nsIURI* GetBindingURI() { return mBindingURI; }
>+  nsIURI* GetDocURI();
>+  nsresult GetID(nsACString& aResult) { return mBindingURI->GetRef(aResult); }

As long as you're tweaking these method signatures, they can all 3 be |const|.

r/sr=bryner.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

>Hm, for some reason I thought certain compilers didn't like using forward
>declarations with reference parameters.  I'm not asking you to change it now,
>but be aware of that when checking in.

I don't recall any such problems.

Comment 13

[Boris Zbarsky [:bzbarsky]]

> I guess this is ok to assume mDocument != null since we would crash in
> NS_NewXBLDocumentInfo now if that was ever the case.

Yep.  The old NS_NewXBLDocumentInfo crashes if mDocument == null too, so I
figured it didn't happen much... In any case, if we decide to fix that it should
be by NS_NewXBLDocumentInfo erroring on a null document.

> As long as you're tweaking these method signatures, they can all 3 be |const|.

Will do.

Is that an r+sr, or should I get r= from someone else (if so, whom?)

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 134887 [details] [diff] [review]
Something like this

Can't you please, pretty-please use the most righteous new noun-phrase naming
convention for weak-ref accessers (BindingURI, not GetBindingURI, per bug
223255)?

/be

Comment 15

[Boris Zbarsky [:bzbarsky]]

Sure.  If we have decided on that convention, I'll roll it into this patch. 
Have we?

Comment 16

[Brendan Eich [:brendan]]

I assert that we have agreement -- roc relented in bug 223255 comment 20.

Nigel McFarlane, author of "Rapid Application Development with Mozilla"
(http://www.amazon.com/exec/obidos/tg/detail/-/0131423436/qid=1068521818/sr=1-1/ref=sr_1_1/102-0209652-3557734?v=glance&s=books)
weighed in just today, but I don't think he's changing anyone's mind.  In
particular, weak vs. strong reference is not the axis along which the different
GetFoo and Foo names lie.

I'd like to declare victory (sorry, Nigel) and make progress, since
deCOMtamination work is ongoing, and will be for a while.

/be

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch with brendan's requested changes

Comment 18

[Boris Zbarsky [:bzbarsky]]

Attachment: Same, but should build

Some XBLResourceLoader changes I happened to have in my tree snuck into that
last patch...

Comment 19

[Boris Zbarsky [:bzbarsky]]

Crap.  I can't quite land this as-is, because it forces XBL to only work for
real honest-to-God things with refs, and jar: is not amongst that number (see
bug 224797).  This causes a bit of a problem with Inspector (the window doesn't
load properly the first time you open it, though it works well thereafter).

Comment 20

[Brian Ryner (not reading)]

Comment on attachment 134887 [details] [diff] [review]
Something like this

Can you do a new review request when you get this working for Inspector?
Thanks.

Comment 21

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 134887 [details] [diff] [review]
Something like this

It's working for Inspector, in my build.  The changes to make that happen are
in bug 224797 and are all outside XBL code.  So this patch stands as-is, I just
can't land it till I check in the patch in bug 224797.

Comment 22

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 135300 [details] [diff] [review]
Same, but should build

Patch for bug 224797 is in.  Here's hoping bryner can get to this before the
freeze... ;)

Comment 23

[Brian Ryner (not reading)]

Comment on attachment 135300 [details] [diff] [review]
Same, but should build

>Index: content/xbl/src/nsXBLPrototypeBinding.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLPrototypeBinding.cpp,v
>retrieving revision 1.78
>diff -p -u -1 -0 -r1.78 nsXBLPrototypeBinding.cpp
>--- content/xbl/src/nsXBLPrototypeBinding.cpp	22 Oct 2003 06:09:31 -0000	1.78
>+++ content/xbl/src/nsXBLPrototypeBinding.cpp	11 Nov 2003 22:46:00 -0000
>@@ -213,64 +213,78 @@ static const PRInt32 kAttrInitialSize = 
> static const size_t kInsBucketSizes[] = {
>   sizeof(nsXBLInsertionPointEntry)
> };
> 
> static const PRInt32 kInsNumBuckets = sizeof(kInsBucketSizes)/sizeof(size_t);
> static const PRInt32 kInsInitialSize = (NS_SIZE_IN_HEAP(sizeof(nsXBLInsertionPointEntry))) * kNumElements;
> 
> // Implementation /////////////////////////////////////////////////////////////////
> 
> // Constructors/Destructors
>-nsXBLPrototypeBinding::nsXBLPrototypeBinding(const nsACString& aID,
>-                                             nsIXBLDocumentInfo* aInfo,
>-                                             nsIContent* aElement)
>+nsXBLPrototypeBinding::nsXBLPrototypeBinding()
> : mImplementation(nsnull),
>   mBaseBinding(nsnull),
>   mInheritStyle(PR_TRUE), 
>   mHasBaseProto(PR_TRUE),
>   mKeyHandlersRegistered(PR_FALSE),
>   mResources(nsnull),
>   mAttributeTable(nsnull), 
>   mInsertionPointTable(nsnull),
>   mInterfaceTable(nsnull)
> {
>-  
>-  mID = ToNewCString(aID);
>-
>-  mXBLDocInfoWeak = do_GetWeakReference(aInfo);
>-  
>   gRefCnt++;
>-  //  printf("REF COUNT UP: %d %s\n", gRefCnt, (const char*)mID);
> 
>   if (gRefCnt == 1) {
>     kAttrPool = new nsFixedSizeAllocator();
>     kAttrPool->Init("XBL Attribute Entries", kAttrBucketSizes, kAttrNumBuckets, kAttrInitialSize);
>     kInsPool = new nsFixedSizeAllocator();
>     kInsPool->Init("XBL Insertion Point Entries", kInsBucketSizes, kInsNumBuckets, kInsInitialSize);
>   }
>+}
>+
>+nsresult
>+nsXBLPrototypeBinding::Init(const nsACString& aID,
>+                            nsIXBLDocumentInfo* aInfo,
>+                            nsIContent* aElement)
>+{
>+  if (!kAttrPool || !kInsPool) {
>+    return NS_ERROR_OUT_OF_MEMORY;
>+  }

I don't know that there's a point to this check since we don't check for
allocation failure in the constructor.	We would crash instead of getting here.
 I'd say either check in the ctor or remove the check in Init().


>Index: content/xbl/src/nsXBLWindowHandler.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/xbl/src/nsXBLWindowHandler.cpp,v
>retrieving revision 1.26
>diff -p -u -1 -0 -r1.26 nsXBLWindowHandler.cpp
>--- content/xbl/src/nsXBLWindowHandler.cpp	13 Sep 2003 17:55:02 -0000	1.26
>+++ content/xbl/src/nsXBLWindowHandler.cpp	11 Nov 2003 22:46:13 -0000
>@@ -102,46 +100,51 @@ const char nsXBLSpecialDocInfo::sPlatfor
> // Allow for a userHTMLBindings.xml.
> // XXXbsmedberg Should be in the profile chrome directory, when we have a resource mapping for that
> const char nsXBLSpecialDocInfo::sUserHTMLBindingStr[] = "resource://gre/res/builtin/userHTMLBindings.xml";
> 
> void nsXBLSpecialDocInfo::LoadDocInfo()
> {
>   if (mInitialized)
>     return;
>   mInitialized = PR_TRUE;
> 
>-  mHTMLBindingStr = sHTMLBindingStr;
>-  mPlatformHTMLBindingStr = sPlatformHTMLBindingStr;
>-  mUserHTMLBindingStr = sUserHTMLBindingStr;
>-
>-  if (mHTMLBindings && mPlatformHTMLBindings && mUserHTMLBindings)
>-    return;
>-
>   nsresult rv;
>   nsCOMPtr<nsIXBLService> xblService = 
>            do_GetService("@mozilla.org/xbl;1", &rv);
>   if (NS_FAILED(rv) || !xblService)
>     return;
> 
>   // Obtain the XP and platform doc infos
>-  xblService->LoadBindingDocumentInfo(nsnull, nsnull, 
>-                                      mHTMLBindingStr,
>-                                      nsCAutoString(""), PR_TRUE, 
>-                                      getter_AddRefs(mHTMLBindings));
>-  xblService->LoadBindingDocumentInfo(nsnull, nsnull,
>-                                      mPlatformHTMLBindingStr,
>-                                      nsCAutoString(""), PR_TRUE, 
>-                                      getter_AddRefs(mPlatformHTMLBindings));
>-  xblService->LoadBindingDocumentInfo(nsnull, nsnull,
>-                                      mUserHTMLBindingStr,
>-                                      nsCAutoString(""), PR_TRUE, 
>-                                      getter_AddRefs(mUserHTMLBindings));
>+  nsCOMPtr<nsIURI> bindingURI;
>+  NS_NewURI(getter_AddRefs(bindingURI), sHTMLBindingStr);
>+  if (bindingURI) {
>+    xblService->LoadBindingDocumentInfo(nsnull, nsnull, 
>+                                        bindingURI,
>+                                        PR_TRUE, 
>+                                        getter_AddRefs(mHTMLBindings));
>+  }
>+  
>+  NS_NewURI(getter_AddRefs(bindingURI), sPlatformHTMLBindingStr);

Here, could we just do bindingURI->SetSpec() instead of creating a new URI?

r/sr=bryner with those fixed. Looks good.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> I don't know that there's a point to this check

Right.  timeless brought that up too... Will fix.

> Here, could we just do bindingURI->SetSpec() instead of creating a new URI?

Yeah, I guess we can, since they're all the same type of URI.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: Patch updated to review comments

Comment 26

[timeless]

Comment on attachment 135704 [details] [diff] [review]
Patch updated to review comments

I'm feeling really pathological...
 >   gRefCnt++;
 >   if (gRefCnt == 1) {
In the old code, this was sort of OK, because it would just crash.

Suppose you have a system like raistlin where you don't have enough memory on
Saturday, but there's more 
memory on Sunday (this is true - i have a dead Mozilla waiting for me to debug
a *0=x crash). The new code 
means that once it fails you'll never be able to get this object to work, it
should really try each time, 
but only if it failed the previous time.

Comment 27

[Boris Zbarsky [:bzbarsky]]

Yeah, I'll do that right after I fix the rest of XBL to at least not crash on
OOM... ;)

Comment 28

[Boris Zbarsky [:bzbarsky]]

Checked in.

Comment 29

[Christopher Aillon (sabbatical, not receiving bugmail)]

Btw, it looks like this is responsible for a 20ms spike in Ts on comet, and 35ms
on luna...

Comment 30

[Boris Zbarsky [:bzbarsky]]

Yeah, I saw that.... I'm trying to figure out why, though I suspect the code in
LoadBindingDocumentInfo() is largely to blame...  I wish we had an accessor for
"all but the ref part".  :(

Comment 31

[Boris Zbarsky [:bzbarsky]]

Note that I just checked nsXBLService::LoadBindingDocumentInfo and of the 433
calls into it during startup only 22 missed the XUL prototype cache... so it's
not like I screwed up which URIs we use as cache keys or anything like that. ;)

Comment 32

[Boris Zbarsky [:bzbarsky]]

The other possibility is that the NS_NewURI in nsXBLPrototypeBinding::Init is
being expensive...  Darin, would it be faster to just get the spec of the
document URI, append the '#' and NewURI() that?  The document URI is guaranteed
to not have a ref here....

Comment 33

[Darin Fisher]

bz: i think it would be better to do that.  i just ran TestStandardURL to get a
rough idea of how much each method costs:

> ./TestStandardURL 'http://bugzilla.mozilla.org/show_bug.cgi/id=224765' 100000
completed SetSpec test in 636 milliseconds
completed GetSpec test in 79 milliseconds
...
completed Resolve test in 449 milliseconds

NS_NewURI that requires relative URI resolution is going to take extra time.  in
fact, resolution is quite costly.  i think a lot of the cost of resolution is
string allocation and string copying.  perhaps nsStandardURL::Init can be
optimized to resolve "into" mSpec.

at any rate, taking those numbers in hand and assuming that NS_NewURI, in your
case, is roughly equal to T(Resolve) + T(SetSpec), i'd say that you are better
off calling GetSpec, appending your reference fragment, and then calling
NS_NewURI.  that would essentially amount to T(GetSpec) + T(SetSpec).

Comment 34

[Boris Zbarsky [:bzbarsky]]

Attachment: So sorta like this

Comment 35

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136290 [details] [diff] [review]
So sorta like this

Er... with the second assert changed to use "debugRef", not "ref".  ;)

Comment 36

[Darin Fisher]

Comment on attachment 136290 [details] [diff] [review]
So sorta like this

if you can conveniently get to the document charset, then i would recommend
doing so.  that will allow you to drop the baseURI parameter.  otherwise,
yeah... this is what i meant.  it "should" help performance ;-)

Comment 37

[Boris Zbarsky [:bzbarsky]]

Yeah, I could get the charset off the document easily.

jst, bryner, would it make sense to have GetDocumentCharacterSet return a
|const nsAFlatCString &| instead of |const nsACString&|?  Then we could just
.get() on it directly....

Comment 38

[Johnny Stenback (:jst)]

bz, I'm making that change in bug 226522.

Comment 39

[Darin Fisher]

so did this patch help?

Comment 40

[Boris Zbarsky [:bzbarsky]]

No idea.  I haven't checked it in yet... and I'm not really sure I will till
1.7a opens.

Comment 41

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 136290 [details] [diff] [review]
So sorta like this

I tried checking in the patch to Init(), and it doesn't seem to have an effect
on Ts, while it increases codesize.  I've backed it out.