Bug 184159: Make scripts with for- and event-attributes not lock up the xml-contentsink.

r=peterv sr=jst

content/base/public/nsContentErrors.h

@@ -73,4 +73,8 @@
 #define NS_XML_AUTOLINK_UNDEFINED \
   NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_CONTENT, 7)
 
+/** Error codes for nsIScriptLoader */
+#define NS_CONTENT_SCRIPT_IS_EVENTHANDLER \
+  NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_CONTENT, 8)
+
 #endif // nsContentErrors_h___

content/base/src/nsScriptLoader.cpp

@@ -42,6 +42,8 @@
 #include "nsIScriptElement.h"
 #include "nsIDocShell.h"
 #include "jsapi.h"
+#include "nsContentUtils.h"
+#include "nsUnicharUtils.h"
 
 
 static NS_DEFINE_CID(kCharsetAliasCID, NS_CHARSETALIAS_CID);
@@ -234,6 +236,72 @@ nsScriptLoader::InNonScriptingContainer(nsIDOMHTMLScriptElement* aScriptElement)
   return PR_FALSE;
 }
 
+// Helper method for checking if the script element is an event-handler
+// This means that it has both a for-attribute and a event-attribute.
+// Also, if the for-attribute has a value that matches "\s*window\s*",
+// and the event-attribute matches "\s*onload([ \(].*)?" then it isn't an
+// eventhandler. (both matches are case insensitive).
+// This is how IE seems to filter out a window's onload handler from a
+// <script for=... event=...> element.
+
+PRBool
+nsScriptLoader::IsScriptEventHandler(nsIDOMHTMLScriptElement *aScriptElement)
+{
+  nsCOMPtr<nsIContent> contElement = do_QueryInterface(aScriptElement);
+  if (!contElement ||
+      !contElement->HasAttr(kNameSpaceID_None, nsHTMLAtoms::_event) ||
+      !contElement->HasAttr(kNameSpaceID_None, nsHTMLAtoms::_for)) {
+      return PR_FALSE;
+  }
+
+  nsAutoString str;
+  nsresult rv = contElement->GetAttr(kNameSpaceID_None, nsHTMLAtoms::_for,
+                                     str);
+  NS_ENSURE_SUCCESS(rv, PR_FALSE);
+
+  const nsAString& for_str = nsContentUtils::TrimWhitespace(str);
+
+  if (!for_str.Equals(NS_LITERAL_STRING("window"),
+                      nsCaseInsensitiveStringComparator())) {
+    return PR_TRUE;
+  }
+
+  // We found for="window", now check for event="onload".
+
+  rv = contElement->GetAttr(kNameSpaceID_None, nsHTMLAtoms::_event, str);
+  NS_ENSURE_SUCCESS(rv, PR_FALSE);
+
+  const nsAString& event_str = nsContentUtils::TrimWhitespace(str, PR_FALSE);
+
+  if (event_str.Length() < 6) {
+    // String too short, can't be "onload".
+
+    return PR_TRUE;
+  }
+
+  if (!Substring(event_str, 0, 6).Equals(NS_LITERAL_STRING("onload"),
+                                         nsCaseInsensitiveStringComparator())) {
+    // It ain't "onload.*".
+
+    return PR_TRUE;
+  }
+
+  nsAutoString::const_iterator start, end;
+  event_str.BeginReading(start);
+  event_str.EndReading(end);
+
+  start.advance(6); // advance past "onload"
+
+  if (start != end && *start != '(' && *start != ' ') {
+    // We got onload followed by something other than space or
+    // '('. Not good enough.
+
+    return PR_TRUE;
+  }
+
+  return PR_FALSE;
+}
+
 /* void processScriptElement (in nsIDOMHTMLScriptElement aElement, in nsIScriptLoaderObserver aObserver); */
 NS_IMETHODIMP 
 nsScriptLoader::ProcessScriptElement(nsIDOMHTMLScriptElement *aElement, 
@@ -254,6 +322,12 @@ nsScriptLoader::ProcessScriptElement(nsIDOMHTMLScriptElement *aElement,
     return FireErrorNotification(NS_ERROR_NOT_AVAILABLE, aElement, aObserver);
   }
 
+  // Check that the script is not an eventhandler
+  if (IsScriptEventHandler(aElement)) {
+    return FireErrorNotification(NS_CONTENT_SCRIPT_IS_EVENTHANDLER, aElement,
+                                 aObserver);
+  }
+
   // See if script evaluation is enabled.
   PRBool scriptsEnabled = PR_TRUE;
   nsCOMPtr<nsIScriptGlobalObject> globalObject;

content/base/src/nsScriptLoader.h

@@ -52,6 +52,7 @@ public:
 
 protected:
   PRBool InNonScriptingContainer(nsIDOMHTMLScriptElement* aScriptElement);
+  PRBool IsScriptEventHandler(nsIDOMHTMLScriptElement* aScriptElement);
   nsresult FireErrorNotification(nsresult aResult,
                                  nsIDOMHTMLScriptElement* aElement,
                                  nsIScriptLoaderObserver* aObserver);

content/html/content/src/nsHTMLScriptElement.cpp

@@ -441,62 +441,6 @@ NS_HTML_CONTENT_INTERFACE_MAP_BEGIN(nsHTMLScriptElement,
 NS_HTML_CONTENT_INTERFACE_MAP_END
 
 
-// Helper method for checking if the script element has a "for"
-// attribute with a value that matches "\s*window\s*", and an "event"
-// attribute that matches "\s*onload([ \(].*)?", both case
-// insensitive. This is how IE seems to filter out a window's onload
-// handler from a <script for=... event=...> element.
-
-PRBool
-nsHTMLScriptElement::IsOnloadEventForWindow()
-{
-  nsAutoString str;
-  nsresult rv = GetAttr(kNameSpaceID_None, nsHTMLAtoms::_for, str);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  const nsAString& for_str = nsContentUtils::TrimWhitespace(str);
-
-  if (!for_str.Equals(NS_LITERAL_STRING("window"),
-                      nsCaseInsensitiveStringComparator())) {
-    return PR_FALSE;
-  }
-
-  // We found for="window", now check for event="onload".
-
-  rv = GetAttr(kNameSpaceID_None, nsHTMLAtoms::_event, str);
-  NS_ENSURE_SUCCESS(rv, rv);
-
-  const nsAString& event_str = nsContentUtils::TrimWhitespace(str, PR_FALSE);
-
-  if (event_str.Length() < 6) {
-    // String too short, can't be "onload".
-
-    return PR_FALSE;
-  }
-
-  if (!Substring(event_str, 0, 6).Equals(NS_LITERAL_STRING("onload"),
-                                         nsCaseInsensitiveStringComparator())) {
-    // It ain't "onload.*".
-
-    return PR_FALSE;
-  }
-
-  nsAutoString::const_iterator start, end;
-  event_str.BeginReading(start);
-  event_str.EndReading(end);
-
-  start.advance(6); // advance past "onload"
-
-  if (start != end && *start != '(' && *start != ' ') {
-    // We got onload followed by something other than space or
-    // '('. Not good enough.
-
-    return PR_FALSE;
-  }
-
-  return PR_TRUE;
-}
-
 NS_IMETHODIMP 
 nsHTMLScriptElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
                              const nsAString& aValue, PRBool aNotify)
@@ -513,39 +457,6 @@ nsHTMLScriptElement::SetAttr(PRInt32 aNameSpaceID, nsIAtom* aName,
 
   if (aNotify && aName == nsHTMLAtoms::src) {
     MaybeProcessScript();
-  } else if (((aName == nsHTMLAtoms::_for &&
-               HasAttr(kNameSpaceID_None, nsHTMLAtoms::_event)) ||
-              (aName == nsHTMLAtoms::_event &&
-               HasAttr(kNameSpaceID_None, nsHTMLAtoms::_for))) &&
-             !IsOnloadEventForWindow()) {
-    // If the script has NOT been executed yet then create a script
-    // event handler if necessary...
-    if (!mIsEvaluated && !mScriptEventHandler) {
-      mScriptEventHandler = new nsHTMLScriptEventHandler(this);
-      if (!mScriptEventHandler) {
-        return NS_ERROR_OUT_OF_MEMORY;
-      }
-
-      NS_ADDREF(mScriptEventHandler);
-
-      // Mark the script as having already been executed.
-      //
-      // Event handlers are only compiled and executed in response to
-      // an event firing...
-      mIsEvaluated = PR_TRUE;
-    }
-
-    if (mScriptEventHandler) {
-      if (aName == nsHTMLAtoms::_event) {
-        rv = mScriptEventHandler->ParseEventString(aValue);
-      } else {
-        nsAutoString event_val;
-        rv = GetAttr(kNameSpaceID_None, nsHTMLAtoms::_event, event_val);
-        NS_ENSURE_SUCCESS(rv, rv);
-
-        rv = mScriptEventHandler->ParseEventString(event_val);
-      }
-    }
   }
 
   return rv;
@@ -741,14 +652,42 @@ nsHTMLScriptElement::MaybeProcessScript()
 
   // We'll always call this to make sure that
   // ScriptAvailable/ScriptEvaluated gets called. See bug 153600
+  nsresult rv = NS_OK;
   nsCOMPtr<nsIScriptLoader> loader;
   mDocument->GetScriptLoader(getter_AddRefs(loader));
   if (loader) {
     mEvaluating = PR_TRUE;
-    loader->ProcessScriptElement(this, this);
+    rv = loader->ProcessScriptElement(this, this);
     mEvaluating = PR_FALSE;
   }
 
+  if (rv == NS_CONTENT_SCRIPT_IS_EVENTHANDLER) {
+
+    // If the script has NOT been executed yet then create a script
+    // event handler if necessary...
+    if (!mIsEvaluated && !mScriptEventHandler) {
+      // Set mIsEvaluated, this element will be handled by the
+      // nsIScriptEventManager
+      mIsEvaluated = PR_TRUE;
+
+      mScriptEventHandler = new nsHTMLScriptEventHandler(this);
+      if (!mScriptEventHandler) {
+        return;
+      }
+
+      NS_ADDREF(mScriptEventHandler);
+
+      // The script-loader will make sure that the script is not evaluated
+      // right away.
+    }
+
+    if (mScriptEventHandler) {
+      nsAutoString event_val;
+      GetAttr(kNameSpaceID_None, nsHTMLAtoms::_event, event_val);
+      mScriptEventHandler->ParseEventString(event_val);
+    }
+  }
+
   // But we'll only set mIsEvaluated if we did really load or evaluate
   // something
   if (HasAttr(kNameSpaceID_None, nsHTMLAtoms::src) || mChildren.Count()) {