Last Comment Bug 205778 - document('') load of stylesheet conflicts with http cache
: document('') load of stylesheet conflicts with http cache
: fixed1.8.1, hang, regression, verified1.8.0.2
Product: Core
Classification: Components
Component: XSLT (show other bugs)
: Trunk
: x86 All
P2 critical with 1 vote (vote)
: mozilla1.8beta4
Assigned To: Peter Van der Beken [:peterv]
: Keith Visco
: Andrew Overholt [:overholt]
: 205633 209869 279869 306439 317953 318563 (view as bug list)
Depends on: 158457
  Show dependency treegraph
Reported: 2003-05-15 00:46 PDT by rbs
Modified: 2006-07-06 16:20 PDT (History)
15 users (show)
mscott: blocking1.8b5-
dveditz: blocking1.8.0.2+
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---

screenshot (9.21 KB, image/png)
2003-05-15 05:38 PDT, Hermann Schwab
no flags Details
txAsyncProcessor.cpp (3.57 KB, text/plain)
2004-01-18 08:51 PST, Axel Hecht
no flags Details
v1 (3.52 KB, patch)
2005-09-04 13:27 PDT, Peter Van der Beken [:peterv]
no flags Details | Diff | Splinter Review
v1.1 (3.53 KB, patch)
2005-09-06 12:17 PDT, Peter Van der Beken [:peterv]
bzbarsky: superreview+
Details | Diff | Splinter Review
v1.2 (3.75 KB, patch)
2005-09-07 10:07 PDT, Peter Van der Beken [:peterv]
axel: review+
peterv: superreview+
asa: approval1.8b5-
jonas: approval‑branch‑1.8.1+
dveditz: approval1.8.0.2+
Details | Diff | Splinter Review
stylesheet using document('') (817 bytes, application/xml)
2006-02-24 14:35 PST, Brad Clements
no flags Details
The input xml file (105 bytes, application/xml)
2006-02-24 14:36 PST, Brad Clements
no flags Details
The expected html output (354 bytes, text/html)
2006-02-24 14:37 PST, Brad Clements
no flags Details

Description User image rbs 2003-05-15 00:46:04 PDT
Bug 158457 has unfortunately regressed.
Comment 1 User image tenthumbs 2003-05-15 04:27:14 PDT
My trunk build hangs on the url here the same way as bug 205633. Related?
Comment 2 User image Hermann Schwab 2003-05-15 05:38:46 PDT
Created attachment 123392 [details]

Mozilla/5.0 (Windows; U; Win98; en-US; rv:1.4b) Gecko/20030514
If I try to view the URL in mozilla, mozilla hangs.
If I try to view the URL in Netscape 4.8, it asks me if I want to open it in
mozilla, or save the file.
So the file bugs-umss.xsl is saved to c:\windows\temp, and the same mozilla,
which was hanging at loading directly, starts and displays immediately, see
Looking at the source, I tried to download a stylesheet, and netscape offered
me to download pmathml.xsl using IE.
I´m seeing the same behaviour as I´ve seen in
Comment 3 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2003-05-15 08:58:43 PDT
For the record, bug 158457 can not have regressed with the same problem. We're
using a compleatly different (and new) codepath for this nowadays so this must
be some new problem
Comment 4 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2003-05-15 10:05:02 PDT
the problem is the document("") call. For some reason we end up waiting forever
for that document to arrive. I suspect necko or someone doesn't like that the
same URL is being loaded twice simultaniously.
Comment 5 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2003-05-15 15:10:19 PDT
What happens is this:

1. We load the (http) URI for the stylesheet
2. In the OnStopRequest for that URI we kick off the transformation
3. The transformation ends up needing to load the same URI again. This is by
   design, we are going to pass it through a different parser this time so we do
   need to load it again. This load will go through the syncloader.
4. We end up in nsHttpChannel::OpenCacheEntry which "fails" on line 1102
5. Since we are loading it syncronously we never exit the OnStopRequest from
   the first load, so the cash-entry is never unlocked.
Comment 6 User image Darin Fisher 2003-05-15 16:39:46 PDT
hmm... this is actually by design.  as i mentioned on IRC to sicking, there
might be a problem with closing the cache entry before calling OnStopRequest. 
and then problem is that our API allows consumers to access the state of the
channel in OnStopRequest.  since the cache entry is available via
nsICachingChannel, it is part of the state of the channel.  we could change this
policy, but i'm a bit worried about the implications elsewhere.

the safest solution would be to workaround necko's behavior, but that is not
going to be pretty.  one option might be to post a PLEvent to the UI thread's
event queue, and only do the sync load from within the callback from that
PLEvent.  is that an option?  do you really need to keep OnStopRequest on the stack?

i'm going to be thinking more about possibly closing down the cache entry before
calling OnStopRequest.  maybe that will prove to be ok.
Comment 7 User image Darin Fisher 2003-05-15 17:03:10 PDT
thought about this some more, and i don't think we want to close the cache entry
before calling OnStopRequest.  that would prevent an application from inspecting
necko's cache after say a HTTP load was aborted to see if HTTP left around a
partial cache entry.  it's a rather arbitrary example, but maybe more
significant is the potential risk of regressions resulting from such a change in
necko's API.  

my recommendation is to post a PLEvent from your OnStopRequest handler and move
whatever code you have in OnStopRequest into the handler for that PLEvent. 
since OnStopRequest is fired from a PLEvent, this should just work.
Comment 8 User image rbs 2003-05-15 18:07:01 PDT
*** Bug 205633 has been marked as a duplicate of this bug. ***
Comment 9 User image Axel Hecht 2003-05-16 02:47:26 PDT
That plevent relay would have to happen in txMozillaXSLTProcessor::DoTransform,
I'd say. Peter and I agreed that sicking has to learn plevents. Harharhar.
Comment 10 User image Axel Hecht 2003-05-16 03:10:57 PDT
Coming to think of it, we should fix this in a way that will make interrupting
the transformation a piece of cake. That is, the handler should prolly just
include everything from
 nsresult rv = txXSLTProcessor::execute(es);
down. Adjusting topic to reflect what's going on.
Comment 11 User image Darin Fisher 2003-05-16 10:13:45 PDT
i should add that once you go with a PLEvent solution you will have to worry
about things changing out from under you.  be sure to verify that the user
hasn't moved onto something else when the PLEvent fires.  there are a number of
ways to deal with this problem.
Comment 12 User image Axel Hecht 2003-05-16 10:26:39 PDT
darin, would listening to the loadgroup be safe?
Either as group observer (which needs to forward to the previous, so it wouldn't
be completely trivial) or by implementing nsIRequest and add the processor to
the loadgroup? Probably part of that question is, on which thread would that 
cancel happen?
Comment 13 User image Darin Fisher 2003-05-16 10:37:26 PDT
cancel always happens on the main thread.  you shouldn't have to worry about
that.  the typical solution is to stick a dummy request in the load group and
watch for it to be canceled.  or just record that it was canceled so that when
your PLEvent fires, you can check the status of the dummy request.
Comment 14 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2003-05-16 15:54:10 PDT
The biggest problem is that once we exit OnStopRequest the loadgroup will be
finished and all sorts of events will be fired off (onload on the document,
something on the nsIWebProgressListener, etc etc). This causes all sorts of bad
things to happen.

Is there some way we could tell the loadgroup to hold off regarding itself as
finished? I.e. could we increase the request-count manually without actually
loading something? Is that what you talk about in comment 13? If so, how would
we do such a thing?
Comment 15 User image Darin Fisher 2003-05-16 15:58:08 PDT
sicking: think of it like javascript in an onLoad handler assigning an address
to an image node.  it will add a new nsIRequest to the load group.  this will
get canceled if the user clicks on a link, and since you would only want to do
this next processing step if the original OnStopRequest passed you a success
code, you shouldn't have to worry about the situation of your nsIRequest being
added into the load group of a canceled browser window.

check out nsLayoutDummyRequest... you basically want something like that i think.
Comment 16 User image rbs 2003-05-16 16:23:32 PDT
jst recently checked in something on this dummy stuff, perhaps attachment 123205 [details] [diff] [review]
from bug 93015 might give some helpful insight.
Comment 18 User image Boris Zbarsky [:bz] (still a bit busy) 2003-05-17 00:14:32 PDT
If you do use nsDummyLayoutRequest, then when/if we just make it possible to
tell loadgroups about outstanding load events we can just convert all
nsDummyLayoutRequest users to that more or less by search/replace.....
Comment 19 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2003-05-22 14:34:29 PDT
unfortunatly i can't use nsDummyLayoutRequest since this could would probably
live in transformiix
Comment 20 User image Boris Zbarsky [:bz] (still a bit busy) 2003-05-22 16:13:17 PDT
Then how about just going all the way and creating an nsILoadGroup2 which has a
way to tell it that a load event is pending and that a load event has been
fired?  Then the loadgroup can keep track of active requests _and_ pending load
events, and fire onload when there are none left of either....

And then we can remove the dummylayoutrequest stuff.
Comment 21 User image Darin Fisher 2003-05-22 16:35:13 PDT
boris: this code still needs some kind of callback to record whether or not the
load group got canceled.  the load group can't record this information since it
might get reused immediately.  i think a dummy request still makes the most
sense... maybe we can just move its implementation into necko and make it
available as an XPCOM component.
Comment 22 User image rbs 2003-05-23 07:59:12 PDT
There is another implementation of this dummy request in the parser:
Comment 23 User image rbs 2003-06-18 21:48:36 PDT
*** Bug 209869 has been marked as a duplicate of this bug. ***
Comment 24 User image rbs 2003-06-22 16:57:50 PDT
re: comment #19
sicking, hope this is still in your radar. As you know, it is an important bug
for xp mathml support (i.e., with IE+MathPlayer).

For 1.4, what about just duplicating the dummy request (as the parser did). I am
afraid that it might be another 2-3 years before the next Nav comes from the
trunk with an idealistic fix.
Comment 25 User image Axel Hecht 2003-06-25 13:05:43 PDT
I'm looking into this. Can't promise any schedule yet.

So, we need both a PLEvent and a nsIRequest. I want to be able to send off
events more than once, so that we can unblock the UI. This raises the question,
can I PostEvent a PLEvent more than once? Then I could fold the event and the
request into one object.
Comment 26 User image Darin Fisher 2003-06-25 16:40:41 PDT
hmm... it might be possible to reuse a PLEvent object since you get to implement
a destructor method for it.  however, you probably need to call PL_InitEvent
each time before calling PostEvent.
Comment 27 User image Axel Hecht 2003-07-05 09:32:05 PDT
I don't see me getting this done for 1.5a freeze.
I would like to cut the txMozillaXSLTProcessor into the js part and into a new
class for the content transformation. With the new helper classes for the request
and the event, the important stuff would just get lost. I started 
txAsyncProcessor.cpp, which already has 150 lines with just the request and silly 
parts of the event, that isn't really doing anything yet.
Comment 28 User image Axel Hecht 2004-01-18 08:51:51 PST
Created attachment 139343 [details]

Async stuff, nobrainer so far. This is about the bloat that we have to add.
I'd like to tear txMozillaXSLTProcessor into two classes, one for js, one for
content. I bet I have to change the contract ID for content in that process, I 

was thinking of going with 
(note the incremented version number) and leave the version 1 for compatibility

for the js class.

I think of splitting the interfaces
nsIXSLTProcessor, nsIXSLTProcessorObsolete, nsIDocumentObserver into the class
txMozillaXSLTProcessor (I don't wanna rename that, but it's the js one) and
nsIDocumentTransformer and the event stuff into
txAsyncXSLTProcessor (do we need a Mozilla in that name?)

Attaching so that I can put this on Peter's request queue.
Comment 29 User image Axel Hecht 2004-04-05 10:00:59 PDT
Comment on attachment 139343 [details]

peterv said: check for duplicated code, and check the contract ids. If we need
the change, change them more.
Comment 30 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2005-01-27 09:41:49 PST
*** Bug 279869 has been marked as a duplicate of this bug. ***
Comment 31 User image Peter Van der Beken [:peterv] 2005-08-31 02:53:30 PDT
*** Bug 306439 has been marked as a duplicate of this bug. ***
Comment 32 User image Peter Van der Beken [:peterv] 2005-09-04 13:27:13 PDT
Created attachment 194861 [details] [diff] [review]
Comment 33 User image Peter Van der Beken [:peterv] 2005-09-06 12:17:52 PDT
Created attachment 195036 [details] [diff] [review]

Decided to pass PR_TRUE to UnblockOnload. Boris, does that seem right?
Comment 34 User image Boris Zbarsky [:bz] (still a bit busy) 2005-09-06 12:33:18 PDT
Comment on attachment 195036 [details] [diff] [review]

>Index: extensions/transformiix/source/xslt/txMozillaXSLTProcessor.cpp

>+HandleTransformBlockerEvent(PLEvent *aEvent)

This and the destroy callback probably need to be declared (not necessarily
defined) in an extern "C" block to actually match the NSPR types for PLEvent

>+    document->UnblockOnload(PR_TRUE);

Yeah, passing PR_TRUE here should be just fine.

>@@ -329,7 +352,38 @@ txMozillaXSLTProcessor::DoTransform()

>+    document->BlockOnload();
>+    eventQ->PostEvent(event);
>+    if (NS_FAILED(rv)) {
>+        PL_DestroyEvent(event);

Won't this leave onload blocked?  That is, perhaps the UnblockOnload() call
should be in the destroy callback, not the handle callback?

sr=bzbarsky with those fixed.
Comment 35 User image Peter Van der Beken [:peterv] 2005-09-07 10:07:46 PDT
Created attachment 195159 [details] [diff] [review]

Changed following bz's comments. This one also reports the error if PostEvent
fails. Not ideal, but it'll have to do for now.
Comment 36 User image Peter Van der Beken [:peterv] 2005-09-07 11:41:49 PDT
This isn't really specific to MathML.
Comment 37 User image Peter Van der Beken [:peterv] 2005-09-08 11:12:38 PDT
Comment on attachment 195159 [details] [diff] [review]

Fixes hang in XSLT.
Comment 38 User image Asa Dotzler [:asa] 2005-09-12 16:24:21 PDT
Comment on attachment 195159 [details] [diff] [review]

We're too late in the game and this isn't high enough priority to take so late.
Comment 39 User image Scott MacGregor 2005-09-13 09:42:31 PDT
clearing this flag per the delivery teams comment from yesterday.
Comment 40 User image Peter Van der Beken [:peterv] 2005-11-28 06:53:09 PST
*** Bug 317953 has been marked as a duplicate of this bug. ***
Comment 41 User image Peter Van der Beken [:peterv] 2005-12-02 01:59:55 PST
*** Bug 318563 has been marked as a duplicate of this bug. ***
Comment 42 User image Peter Van der Beken [:peterv] 2006-02-09 07:41:49 PST
Comment on attachment 195159 [details] [diff] [review]

Sicking, what do you think? It fixes a hang, and it seems to have stuck to the trunk (I'd take it, I just want a second opinion).
Comment 43 User image Brad Clements 2006-02-09 08:10:36 PST
I sure would like to have this fixed, I really depend on using document('') in stylesheets. 

I was hoping this would have already been deployed in  :-(
Comment 44 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-02-09 13:17:36 PST
Comment on attachment 195159 [details] [diff] [review]

Yeah, it looks safe enough.
Comment 45 User image Jonas Sicking (:sicking) No longer reading bugmail consistently 2006-02-17 16:42:17 PST
Comment on attachment 195159 [details] [diff] [review]

The patch is pretty simple and has baked on the trunk for a while without regressions. This is fixing a hang in XSLT so I think it would be good to have in FF 1.5
Comment 46 User image Daniel Veditz [:dveditz] 2006-02-22 00:59:09 PST
Comment on attachment 195159 [details] [diff] [review]

approved for 1.8.0 branch, a=dveditz
Comment 47 User image Dave Liebreich [:davel] 2006-02-24 14:18:54 PST
could someone please attach a simple testcase to this bug
Comment 48 User image Brad Clements 2006-02-24 14:35:16 PST
Created attachment 213109 [details]
stylesheet using document('')

Here's a stylesheet that demonstrates the bug. I'll also attach the input xml, and the expected output from the transform.

In IE (when served with proper mime type), loading the .xml file causes IE to load this .xsl file from the same network path and generate xhtml output.

With FF, the .xsl file gets loaded, but then FF hangs on the document('') function call.
Comment 49 User image Brad Clements 2006-02-24 14:36:08 PST
Created attachment 213110 [details]
The input xml file

This .xml file has a PI that loads the corresponding .xsl file.
Comment 50 User image Brad Clements 2006-02-24 14:37:05 PST
Created attachment 213111 [details]
The expected html output

The output I expect from the transform. I generated this using libxslt
Comment 51 User image Tracy Walker [:tracy] 2006-03-06 13:33:12 PST
Verified on Firefox buildsfrom 20060306 on Windows, Mac and Linux
Comment 52 User image Daniel Veditz [:dveditz] 2006-07-06 16:20:30 PDT
we're using "fixed1.8.1" for the 1.8 branch post-1.8 (confusing, I know)

Note You need to log in before you can comment on or make changes to this bug.