Bug 345711: Call InstallImplementation when it's safe, rather tha directly from LoadBindings. r/sr=jst

content/xbl/src/nsBindingManager.cpp

@@ -933,6 +933,11 @@ nsBindingManager::ProcessAttachedQueue()
     mAttachedStack.RemoveElementAt(lastItem);
 
     NS_ASSERTION(binding, "null item in attached stack?");
+    nsresult rv = binding->EnsureScriptAPI();
+    if (NS_FAILED(rv)) {
+      return;
+    }
+
     binding->ExecuteAttachedHandler();
   }
 

content/xbl/src/nsXBLBinding.cpp

@@ -272,7 +272,8 @@ nsXBLBinding::nsXBLBinding(nsXBLPrototypeBinding* aBinding)
   : mPrototypeBinding(aBinding),
     mInsertionPointTable(nsnull),
     mIsStyleBinding(PR_TRUE),
-    mMarkedForDeath(PR_FALSE)
+    mMarkedForDeath(PR_FALSE),
+    mInstalledAPI(PR_FALSE)
 {
   NS_ASSERTION(mPrototypeBinding, "Must have a prototype binding!");
   // Grab a ref to the document info so the prototype binding won't die
@@ -375,18 +376,6 @@ nsXBLBinding::SetBoundElement(nsIContent* aElement)
     mNextBinding->SetBoundElement(aElement);
 }
 
-nsXBLBinding*
-nsXBLBinding::GetFirstBindingWithConstructor()
-{
-  if (mPrototypeBinding->GetConstructor())
-    return this;
-
-  if (mNextBinding)
-    return mNextBinding->GetFirstBindingWithConstructor();
-
-  return nsnull;
-}
-
 PRBool
 nsXBLBinding::HasStyleSheets() const
 {
@@ -800,6 +789,22 @@ nsXBLBinding::GenerateAnonymousContent()
   }
 }
 
+nsresult
+nsXBLBinding::EnsureScriptAPI()
+{
+  if (mInstalledAPI) {
+    return NS_OK;
+  }
+  
+  // Set mInstalledAPI right away since we'll recurse into here from
+  // nsElementSH::PostCreate when InstallImplementation is called.
+  mInstalledAPI = PR_TRUE;
+
+  InstallEventHandlers();
+
+  return InstallImplementation();
+}
+
 void
 nsXBLBinding::InstallEventHandlers()
 {

content/xbl/src/nsXBLBinding.h

@@ -120,15 +120,13 @@ public:
 
   void GenerateAnonymousContent();
   void InstallAnonymousContent(nsIContent* aAnonParent, nsIContent* aElement);
-  void InstallEventHandlers();
-  nsresult InstallImplementation();
+  nsresult EnsureScriptAPI();
 
   void ExecuteAttachedHandler();
   void ExecuteDetachedHandler();
   void UnhookEventHandlers();
 
   nsIAtom* GetBaseTag(PRInt32* aNameSpaceID);
-  nsXBLBinding* GetFirstBindingWithConstructor();
   nsXBLBinding* RootBinding();
   nsXBLBinding* GetFirstStyleBinding();
 
@@ -169,6 +167,12 @@ public:
 
 // MEMBER VARIABLES
 protected:
+  // These two functions recursively install the event handlers
+  // and implementation on this binding and its base class bindings.
+  // External callers should call EnsureScriptAPI instead.
+  void InstallEventHandlers();
+  nsresult InstallImplementation();
+
   nsAutoRefCnt mRefCnt;
   nsXBLPrototypeBinding* mPrototypeBinding; // Weak, but we're holding a ref to the docinfo
   nsCOMPtr<nsIContent> mContent; // Strong. Our anonymous content stays around with us.
@@ -181,6 +185,7 @@ protected:
 
   PRPackedBool mIsStyleBinding;
   PRPackedBool mMarkedForDeath;
+  PRPackedBool mInstalledAPI;
 };
 
 #endif // nsXBLBinding_h_

content/xbl/src/nsXBLService.cpp

@@ -579,20 +579,11 @@ nsXBLService::LoadBindings(nsIContent* aContent, nsIURI* aURL,
   // Tell the binding to build the anonymous content.
   newBinding->GenerateAnonymousContent();
 
-  // Tell the binding to install event handlers
-  newBinding->InstallEventHandlers();
-
-  // Set up our properties
-  rv = newBinding->InstallImplementation();
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  // Figure out if we need to execute a constructor.
-  *aBinding = newBinding->GetFirstBindingWithConstructor();
-  NS_IF_ADDREF(*aBinding);
-
   // Figure out if we have any scoped sheets.  If so, we do a second resolve.
   *aResolveStyle = newBinding->HasStyleSheets();
   
+  newBinding.swap(*aBinding);
+
   return NS_OK; 
 }
 

dom/src/base/nsDOMClassInfo.cpp

@@ -6971,11 +6971,19 @@ nsElementSH::PostCreate(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
   // We must ensure that the XBL Binding is installed before we hand
   // back this object.
 
-  if (doc->BindingManager()->GetBinding(content)) {
-    // There's already a binding for this element so nothing left to
-    // be done here.
+  nsRefPtr<nsXBLBinding> binding;
+  if ((binding = doc->BindingManager()->GetBinding(content))) {
+    // There's already a binding for this element, make sure that
+    // the script API has been installed.
+    // Note that this could end up recusing into code that calls
+    // WrapNative. So don't do anything important beyond this point
+    // as that will not be done to the wrapper returned from that
+    // WrapNative call.
+    // In theory we could also call ExecuteAttachedHandler here if
+    // we also removed the binding from the PAQ queue, but that seems
+    // like a scary change that would mosly just add more inconsistencies.
 
-    return NS_OK;
+    return binding->EnsureScriptAPI();
   }
 
   // Get the computed -moz-binding directly from the style context
@@ -6984,7 +6992,6 @@ nsElementSH::PostCreate(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
 
   // Make sure the style context goes away _before_ we execute the binding
   // constructor, since the constructor can destroy the relevant presshell.
-  nsRefPtr<nsXBLBinding> binding;
   {
     // Scope for the nsRefPtr
     nsRefPtr<nsStyleContext> sc = pctx->StyleSet()->ResolveStyleFor(content,
@@ -7009,12 +7016,17 @@ nsElementSH::PostCreate(nsIXPConnectWrappedNative *wrapper, JSContext *cx,
   }
   
   if (binding) {
-    // Make sure the presshell is in a state where it's safe to execute script
+
+#ifdef DEBUG
     PRBool safeToRunScript = PR_FALSE;
     pctx->PresShell()->IsSafeToFlush(safeToRunScript);
-    if (safeToRunScript) {
-      binding->ExecuteAttachedHandler();
-    }
+    NS_ASSERTION(safeToRunScript, "Wrapping when it's not safe to flush");
+#endif
+
+    rv = binding->EnsureScriptAPI();
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    binding->ExecuteAttachedHandler();
   }
 
   return NS_OK;