Closed Bug 158457 Opened 20 years ago Closed 20 years ago

Transformation of Universal MathML Stylesheet doesn't complete

Categories

(Core :: XSLT, defect)

x86
All
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: rbs, Assigned: sicking)

References

()

Details

Attachments

(5 files, 4 obsolete files)

With the first testcase which invokes "pmathml.xsl", the transfromation never
completes, and so "Hello to pmathml.xsl" is never displayed on the page.
I used a printf() in nsXSLContentSink::DidBuildModel() and it didn't show up.
This means onTransformDone() is never called.

With the second testcase which invokes "mathml.xsl", the transformation completes
successfully. The paradox to note is that "mathml.xsl" is including "pmathml.xsl"
as can seen with the view-source protocol here (copy the full "v-s:URL" into the
URL bar to see the syntax-higlighted source of the XSLT stylesheet):
view-source:http://www.w3.org/Math/XSL/mathml.xsl
view-source:http://www.w3.org/Math/XSL/pmathml.xsl

First testcase with pmathml.xsl (bug here)
==========================================
<?xml-stylesheet type="text/xsl"
                 href="http://www.w3.org/Math/XSL/pmathml.xsl"?>
<html xmlns="http://www.w3.org/1999/xhtml">
<head><title>pmathml.xsl</title></head>
<body>
Hello to pmathml.xsl
</body>
</html>

Second testcase with mathml.xsl (no bug here)
=============================================
<?xml-stylesheet type="text/xsl"
                 href="http://www.w3.org/Math/XSL/mathml.xsl"?>
<html xmlns="http://www.w3.org/1999/xhtml">
<head><title>mathml.xsl</title></head>
<body>
Hello to mathml.xsl
</body>
</html>

I will attach these testcases. 

During testing, note that the title of the window is set properly in either
case, but the content area isn't updated in the the first testcase due to the
fact that the transformation is not completing as I mentioned above.
Attached file testcase with pmathml.xsl (bug here) (obsolete) —
Attached file testcase with mathml.xsl (no bug here) (obsolete) —
This seems to have something to do with scriptloading. It looks like we're
blocking the parser for a script load in the stylesheet but never unblock it.
Reassigning to sicking for now.
Assignee: peterv → sicking
Nominating for 1.0.1 and 1.1 since this bug is an impediment to using all
flavors of the Universal MathML stylesheet.
sicking, perhaps this bug might be fixed by what you said in bug 132844 comment 22?
no, that shouldn't affect this. I think it whould be fixed if we didn't create
nsHTMLScriptElements for <script>s in the stylesheet document (which we
shouldn't do anyway). However I need to do some reshuffeling in the contentsink
to be able to do that. I will hopefully get time to look at this really soon
note that a 
<meta content="0;URL=../{/redirect/url}.{$ext}" http-equiv="Refresh"/>
with a xhtml namespace triggers a redirect from the stylesheet, so it doesn't
matter if the meta actually makes it into the content, or if there are AVTs to
be applied.
The test cases that have been attatched refer to the public copy of
pmathml.xsl at w3c.org
Note I am about to update that copy to use <SCRIPT rather than <script
so that enables the stylesheet to work again in mozilla1.1 and NS 1.0
(note the old code worked in mozilla 1.0 and NS 7 pre-release)

so this is to note that the test cases should refer to local copies of the 
stylesheet that use <script otherwise they will no longer demonstrate the 
problem.

The <script in question is not activated at all in mozilla it is in conditional 
code only used in IE (the page still does not display if you surround the 
<script> element with <xsl:if test="false()">...
*** Bug 161087 has been marked as a duplicate of this bug. ***
*** Bug 174353 has been marked as a duplicate of this bug. ***
Attached patch patch to fix (obsolete) — Splinter Review
The problem was that the XSL sink unregistered itself as nsIScriptLoadObserver,
this caused us to not get the neccesary notification leaving the parser
blocked. Note that we need to get (and do get if we are an observer) this
notification even if the scriptloader is disabled.

So the fix is to simply remove the line that unregisters ourself and it should
work. However that was way too easy :), so the patch does the following:

* Cleaning up the initialization of stylesheet loading in the xml-contentsink
by
  making it properly call nsIDocument::StartDocumentLoad.
* Rely on the fix from bug 172372 to stop stylesheet and script loads.
* Remove the manual disabling of stylesheet and script loads in the
  xsl-contentsink.

(obsoleting the testcases since they no longer work)
Attachment #92086 - Attachment is obsolete: true
Attachment #92087 - Attachment is obsolete: true
as always when i'm attaching patches at this hour there are some quirks in them.
Ignore the empty files please, they are part of other fixes
*** Bug 173034 has been marked as a duplicate of this bug. ***
Comment on attachment 103446 [details] [diff] [review]
patch to fix

sr=peterv. While you're tweaking things in there you might want to set the
referrer on the channel for the stylesheet (see the comment just below your
change in nsXMLContentSink.cpp).
Attachment #103446 - Flags: superreview+
Attached patch sync to tip and set the refferer (obsolete) — Splinter Review
fixes petervs comment and sync the patch to the tip
Attachment #103446 - Attachment is obsolete: true
Comment on attachment 103632 [details] [diff] [review]
sync to tip and set the refferer

> 
>   nsCOMPtr<nsIChannel> channel;
>   rv = NS_NewChannel(getter_AddRefs(channel), aUrl);
>   if (NS_FAILED(rv)) return rv;
> 
>+  nsCOMPtr<nsILoadGroup> loadGroup;
>+  rv = mDocument->GetDocumentLoadGroup(getter_AddRefs(loadGroup));
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  channel->SetLoadGroup(loadGroup);
>+

Nit: Can't you delay the channel creation until a load group is available?
Attachment #103632 - Flags: review+
Comment on attachment 103646 [details] [diff] [review]
fix harishs comment

moving forward reviews
Attachment #103646 - Flags: superreview+
Attachment #103646 - Flags: review+
Comment on attachment 103646 [details] [diff] [review]
fix harishs comment

a=asa for checkin to 1.2 (on behalf of drivers)
Attachment #103646 - Flags: approval+
fix checked in
Status: NEW → ASSIGNED
wrong button :)
Status: ASSIGNED → RESOLVED
Closed: 20 years ago
Resolution: --- → FIXED
Re-opening, I am still experiencing the bug.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I have set the background color of the working testcase to green. To clearly
see the bug, click on either of the testcases, and then go and then go forward.

The working testcase (with its green background color) will appear properly,
whereas the other one (with its white background color) is still not rendered.
*** Bug 184159 has been marked as a duplicate of this bug. ***
Adding URL from bug 184159
OS: Windows -> All
Shouldn't severity be increased due to browser hang?
OS: Windows 2000 → All
Still don't fully understand why this wasn't fixed by my last checkin (i do
think it fixed my local testcase i had). However this should be fixed now along
with bug 169124
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
The url in the URL field hangs build 2002-11-07-21, but not 2002-11-06-22 on
Linux.. anything in that range look suspicious?
Sorry, man.  Look at the build date in bug 184159.  And I'm using 2002-12-04-22
on Linux and hanging... (169124 was checked in on the second).
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
looks like bug 184159 is something different, this one is fixed.
Status: REOPENED → RESOLVED
Closed: 20 years ago20 years ago
Resolution: --- → FIXED
regarding my comment 29. The reason why this wasn't fixed by my first patch
didn't fix attachment 107191 [details] is that that stylesheet contains <script
for="...">. Normal scripts got fixed, but the for-attribute still messed up
things. They still do when they are in an included stylesheet, which is what bug
184159 is about.
*** Bug 187502 has been marked as a duplicate of this bug. ***
Blocks: 184159
Blocks: 205778
mass verifying
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.