184159 - Hang accessing MathML page

Status: RESOLVED FIXED

(Core :: XML, defect)

Description

[Max Alekseyev]

Linux build 2002120608

Mozilla hangs up accessing provided URL.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Hangs build 2002-11-07-21 as well.  Does not hang 2002-11-06-22.  Not importing
the XSL stylesheet makes things happy...

Maybe the timer checkins from around then are the problem?

Comment 2

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

*** This bug has been marked as a duplicate of 158457 ***

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

seems like this isn't a dup after all

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: included stylesheet

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: top stylesheet

Comment 6

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: xml file

Comment 7

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

this isn't XSLT related at all although i don't yet know where to send it. The
problem is that the stylesheet contains <script for="..">. This is supported in
mozilla these days (bug 174404).

Normally we don't do anything for <script>s in documents loaded as data (such as
stylesheets) since we disable the scriptloader, however the code for <script
for=".."> doesn't seem to go through the scriptloader the same way and therefor
end up doing something that looks everthing up (not sure what yet). This isn't
specific to XSLT but will happen for document.load and XMLHttpRequest as well
afaict.

Comment 8

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

even better might be to execute <script for="..">s through the scriptloader.
That way we'd automatically disable them when needed

Comment 9

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

*** Bug 154197 has been marked as a duplicate of this bug. ***

Comment 10

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Rick: would you have time to look at this? This isn't restricted to XSLT but
will affect all ways of loading non-displayed documents (such as document.load
and XMLHttpRequest)

Comment 11

[rbs]

rpotts has left and moved on to other non-Mozilla things, so this bug has had no
traction.

The bug can be reproduced clearly on this page too (from bug 189725)
http://www.w3.org/Math/testsuite/testsuite/General/Math/emptymath2.xml

I locally downloaded the XSLT stylesheets involved, namely:

http://www.w3.org/Math/testsuite/style/mathml.xsl
http://www.w3.org/Math/testsuite/style/pmathml.xsl
http://www.w3.org/Math/testsuite/style/ctop.xsl

And donloaded and modified a local copy of emptymath2.xml so that it refers to
my local copies of the XSLT stylesheets.

Then I changed nsSyncLoader::LoadDocument() to use a sync stream:

-       rv = PushAsyncStream(listener);
+       rv = PushSyncStream(listener);

And the hang went away (when loading the local copy). 

Therefore, a fishy interaction seems to be happening with the stylesheet loader.

Comment 12

[rbs]

Marking dependency with bug 109685 which sheds some light as to what is
happening in the sync loader.

Comment 13

[rbs]

For ease of reference, adding another dependency to bug 158457 (see bug 158457
comment 33). As a work-around, people can use <SCRIPT for="..."> (capitals) so
that <SCRIPT> isn't recognized as an XHTML element since XHTML is case-sensitive. 

Comment 14

[rbs]

BTW, <script> has been changed to <SCRIPT> in the testsuite. Therefore the
earlier URLs are not good to reproduce the bug anymore.

Comment 15

[david avery]

major MATHML regression from 1.2.1

Comment 16

[Asa Dotzler [:asa]]

peterv, are you looking into this? It looks like a pretty serious problem for
mathML and drivers are evaluating it for blocking Mozilla 1.3.

Comment 17

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

I'm on this one. It should be fairly safe to fix this for 1.3, but i still 
havn't fully grokked what all the code does. I'll try my best tomorrow to come 
up with a patch

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

i've found the problem. It's not related to XSLT at all, any xml-page containing
a <html:script for=.. event=..> will block the parser and never unblock it.

The reason this hangs XSLT is because we sync-load xml-pages, but any xml-page
will leave the parser in a blocked state causing the page not to load.

Patch comming up

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch to fix

Comment 20

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 116105 [details] [diff] [review]
patch to fix

so the problem is that the script-element sets mIsEvaluated when the "for" and
the "event" attributes were present. This made us never call the scriptloader,
which thus never called the contentsink to unblock its parser.

With this patch we will take the normal route and treat an element with
for/event like an element that should not be evaluated (as it shouldn't)

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: patch v2

In the first patch i forgot that <script for="window" event="onload"> need
special treatment.

So I ended up making a somewhat bigger change to avoid havint the same logic in
two separate places. So now it's the job of the script-loader to return a
special errorcode when it runs into a <script> that is an eventhandler. The
script-element detects this and installs the eventhandler.

This also has the advantage that if the scriptloader is disabled we won't
detect the special errorcode and not install the eventhandler.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

*** Bug 189338 has been marked as a duplicate of this bug. ***

Comment 23

[Johnny Stenback (:jst)]

Comment on attachment 116159 [details] [diff] [review]
patch v2

sr=jst

Comment 24

[Peter Van der Beken [:peterv]]

Comment on attachment 116159 [details] [diff] [review]
patch v2

>Index: content/base/public/nsContentErrors.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/public/nsContentErrors.h,v
>retrieving revision 3.1
>diff -u -6 -r3.1 nsContentErrors.h
>--- content/base/public/nsContentErrors.h	25 Nov 2002 11:20:49 -0000	3.1
>+++ content/base/public/nsContentErrors.h	4 Mar 2003 00:07:59 -0000
>@@ -70,7 +70,11 @@
>   NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_CONTENT, 5)
> #define NS_XML_AUTOLINK_REPLACE \
>   NS_ERROR_GENERATE_SUCCESS(NS_ERROR_MODULE_CONTENT, 6)
> #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___
>Index: content/base/src/nsScriptLoader.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsScriptLoader.cpp,v
>retrieving revision 1.30
>diff -u -6 -r1.30 nsScriptLoader.cpp
>--- content/base/src/nsScriptLoader.cpp	8 Jan 2003 19:18:34 -0000	1.30
>+++ content/base/src/nsScriptLoader.cpp	4 Mar 2003 00:08:01 -0000
>@@ -39,12 +39,14 @@
> #include "nsContentPolicyUtils.h"
> #include "nsIDOMWindow.h"
> #include "nsIHttpChannel.h"
> #include "nsIScriptElement.h"
> #include "nsIDocShell.h"
> #include "jsapi.h"
>+#include "nsContentUtils.h"
>+#include "nsUnicharUtils.h"
> 
> static NS_DEFINE_CID(kCharsetConverterManagerCID, NS_ICHARSETCONVERTERMANAGER_CID);
> 
> //////////////////////////////////////////////////////////////
> //
> //////////////////////////////////////////////////////////////
>@@ -229,12 +231,78 @@
>     node->GetParentNode(getter_AddRefs(parent));
>   }
> 
>   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, 
>                                      nsIScriptLoaderObserver *aObserver)
> {
>   NS_ENSURE_ARG(aElement);
>@@ -247,12 +315,18 @@
>   }
> 
>   // Check to see that the element is not in a container that
>   // suppresses script evaluation within it.
>   if (!mEnabled || InNonScriptingContainer(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;
>   mDocument->GetScriptGlobalObject(getter_AddRefs(globalObject));
>Index: content/base/src/nsScriptLoader.h
>===================================================================
>RCS file: /cvsroot/mozilla/content/base/src/nsScriptLoader.h,v
>retrieving revision 1.4
>diff -u -6 -r1.4 nsScriptLoader.h
>--- content/base/src/nsScriptLoader.h	11 Oct 2002 00:38:16 -0000	1.4
>+++ content/base/src/nsScriptLoader.h	4 Mar 2003 00:08:01 -0000
>@@ -49,12 +49,13 @@
>   NS_DECL_ISUPPORTS
>   NS_DECL_NSISCRIPTLOADER
>   NS_DECL_NSISTREAMLOADEROBSERVER
> 
> protected:
>   PRBool InNonScriptingContainer(nsIDOMHTMLScriptElement* aScriptElement);
>+  PRBool IsScriptEventHandler(nsIDOMHTMLScriptElement* aScriptElement);
>   nsresult FireErrorNotification(nsresult aResult,
>                                  nsIDOMHTMLScriptElement* aElement,
>                                  nsIScriptLoaderObserver* aObserver);
>   nsresult ProcessRequest(nsScriptLoadRequest* aRequest);
>   void FireScriptAvailable(nsresult aResult,
>                            nsScriptLoadRequest* aRequest,
>Index: content/html/content/src/nsHTMLScriptElement.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/content/html/content/src/nsHTMLScriptElement.cpp,v
>retrieving revision 1.55
>diff -u -6 -r1.55 nsHTMLScriptElement.cpp
>--- content/html/content/src/nsHTMLScriptElement.cpp	27 Feb 2003 00:49:31 -0000	1.55
>+++ content/html/content/src/nsHTMLScriptElement.cpp	4 Mar 2003 00:08:05 -0000
>@@ -438,68 +438,12 @@
>                                     mScriptEventHandler);
>   else
>   NS_INTERFACE_MAP_ENTRY_CONTENT_CLASSINFO(HTMLScriptElement)
> 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)
> {
>   nsresult rv = nsGenericHTMLContainerElement::SetAttr(aNameSpaceID,
>                                                        aName,
>@@ -510,45 +454,12 @@
>   if (aNameSpaceID != kNameSpaceID_None) {
>     return rv;
>   }
> 
>   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;
> }
> 
> NS_IMETHODIMP 
>@@ -738,18 +649,46 @@
>   if (mIsEvaluated || mEvaluating || !mDocument || !mParent) {
>     return;
>   }
> 
>   // 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()) {
>     mIsEvaluated = PR_TRUE;

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 116159 [details] [diff] [review]
patch v2

this patch is only semi-safe.

However it should only affect <script> elements with for/event-attributes and
right now such elements works *very* poorly anyway.

The patch is not really as big as it looks, most of it is just moving code
without changing what it does.

Comment 26

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

landed on trunk

Comment 27

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 116159 [details] [diff] [review]
patch v2

I'm going to unrequest approval for this bug. I don't feel that the patch is
safe enough at this stage and the MathML testsuit has worked around the bug for
now.

Feel free to rerequest approval if you don't agree

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

marking bug fixed as this is on trunk

Comment 29

[rbs]

*** Bug 189725 has been marked as a duplicate of this bug. ***