Hang accessing MathML page

RESOLVED FIXED

Status

()

Core
XML
--
critical
RESOLVED FIXED
16 years ago
16 years ago

People

(Reporter: Max Alekseyev, Assigned: sicking)

Tracking

({regression})

Trunk
x86
All
regression
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

16 years ago
Linux build 2002120608

Mozilla hangs up accessing provided URL.
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?
Assignee: asa → peterv
Status: UNCONFIRMED → NEW
Component: Browser-General → XSLT
Ever confirmed: true
QA Contact: asa → keith

*** This bug has been marked as a duplicate of 158457 ***
Status: NEW → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → DUPLICATE
seems like this isn't a dup after all
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
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.
even better might be to execute <script for="..">s through the scriptloader.
That way we'd automatically disable them when needed
*** Bug 154197 has been marked as a duplicate of this bug. ***
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

16 years ago
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

16 years ago
Marking dependency with bug 109685 which sheds some light as to what is
happening in the sync loader.
Depends on: 109685

Comment 13

16 years ago
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. 
Depends on: 158457

Comment 14

16 years ago
BTW, <script> has been changed to <SCRIPT> in the testsuite. Therefore the
earlier URLs are not good to reproduce the bug anymore.
OS: Linux → All

Comment 15

16 years ago
major MATHML regression from 1.2.1
Flags: blocking1.3?

Comment 16

16 years ago
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.
Keywords: regression
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
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
Assignee: peterv → bugmail
Status: REOPENED → NEW
Component: XSLT → XML
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)
Attachment #116105 - Flags: superreview?(jst)
Attachment #116105 - Flags: review?(peterv)
Created attachment 116159 [details] [diff] [review]
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.
Attachment #116105 - Attachment is obsolete: true
Attachment #116159 - Flags: superreview?(peterv)
Attachment #116159 - Flags: review?(jkeiser)
Attachment #116105 - Flags: superreview?(jst)
Attachment #116105 - Flags: review?(peterv)
No longer blocks: 189338
*** Bug 189338 has been marked as a duplicate of this bug. ***
Comment on attachment 116159 [details] [diff] [review]
patch v2

sr=jst
Attachment #116159 - Flags: superreview?(peterv) → superreview+
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;
Attachment #116159 - Flags: review?(jkeiser) → review+
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.
Attachment #116159 - Flags: approval1.3?
landed on trunk
Status: NEW → ASSIGNED
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
Attachment #116159 - Flags: approval1.3?
marking bug fixed as this is on trunk
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago16 years ago
No longer depends on: 109685
Flags: blocking1.3?
Resolution: --- → FIXED

Comment 29

16 years ago
*** Bug 189725 has been marked as a duplicate of this bug. ***
You need to log in before you can comment on or make changes to this bug.