205778 - document('') load of stylesheet conflicts with http cache

Status: RESOLVED FIXED

(Core :: XSLT, defect, P2)

Description

[rbs]

Bug 158457 has unfortunately regressed.

Comment 1

[tenthumbs]

My trunk build hangs on the url here the same way as bug 205633. Related?

Comment 2

[Hermann Schwab]

Attachment: screenshot

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
screenshot.
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
http://bugzilla.mozilla.org/show_bug.cgi?id=205633#c2

Comment 3

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

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

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

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

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

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.

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp&rev=1.164&root=/cvsroot&mark=1102#1096

Comment 6

[Darin Fisher]

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

[Darin Fisher]

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

[rbs]

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

Comment 9

[Axel Hecht]

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

[Axel Hecht]

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

[Darin Fisher]

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

[Axel Hecht]

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

[Darin Fisher]

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

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

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

[Darin Fisher]

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

[rbs]

jst recently checked in something on this dummy stuff, perhaps attachment 123205 [details] [diff] [review]
from bug 93015 might give some helpful insight.

Comment 17

[rbs]

Better link:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsImageLoadingContent.cpp&branch=&root=/cvsroot&subdir=mozilla/content/base/src&command=DIFF_FRAMESET&rev1=1.10&rev2=1.11

Comment 18

[Boris Zbarsky [:bzbarsky]]

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

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

unfortunatly i can't use nsDummyLayoutRequest since this could would probably
live in transformiix

Comment 20

[Boris Zbarsky [:bzbarsky]]

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

[Darin Fisher]

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

[rbs]

There is another implementation of this dummy request in the parser:
http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLContentSink.cpp#497

Comment 23

[rbs]

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

Comment 24

[rbs]

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

[Axel Hecht]

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

[Darin Fisher]

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

[Axel Hecht]

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

[Axel Hecht]

Attachment: txAsyncProcessor.cpp

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 
"@mozilla.org/document-transformer;2?type=xslt"
(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

[Axel Hecht]

Comment on attachment 139343 [details]
txAsyncProcessor.cpp

peterv said: check for duplicated code, and check the contract ids. If we need
the change, change them more.

Comment 30

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

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

Comment 31

[Peter Van der Beken [:peterv]]

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

Comment 32

[Peter Van der Beken [:peterv]]

Attachment: v1

Comment 33

[Peter Van der Beken [:peterv]]

Attachment: v1.1

Decided to pass PR_TRUE to UnblockOnload. Boris, does that seem right?

Comment 34

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 195036 [details] [diff] [review]
v1.1

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

>+void* PR_CALLBACK
>+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
callbacks.

>+    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

[Peter Van der Beken [:peterv]]

Attachment: v1.2

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

[Peter Van der Beken [:peterv]]

This isn't really specific to MathML.

Comment 37

[Peter Van der Beken [:peterv]]

Comment on attachment 195159 [details] [diff] [review]
v1.2

Fixes hang in XSLT.

Comment 38

[Asa Dotzler [:asa]]

Comment on attachment 195159 [details] [diff] [review]
v1.2

We're too late in the game and this isn't high enough priority to take so late.

Comment 39

[Scott MacGregor]

clearing this flag per the delivery teams comment from yesterday.

Comment 40

[Peter Van der Beken [:peterv]]

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

Comment 41

[Peter Van der Beken [:peterv]]

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

Comment 42

[Peter Van der Beken [:peterv]]

Comment on attachment 195159 [details] [diff] [review]
v1.2

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

[Brad Clements]

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  1.5.0.1  :-(

Comment 44

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

Comment on attachment 195159 [details] [diff] [review]
v1.2

Yeah, it looks safe enough.

Comment 45

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

Comment on attachment 195159 [details] [diff] [review]
v1.2

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

[Daniel Veditz [:dveditz]]

Comment on attachment 195159 [details] [diff] [review]
v1.2

approved for 1.8.0 branch, a=dveditz

Comment 47

[Dave Liebreich [:davel]]

could someone please attach a simple testcase to this bug

Comment 48

[Brad Clements]

Attachment: 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

[Brad Clements]

Attachment: The input xml file

This .xml file has a PI that loads the corresponding .xsl file.

Comment 50

[Brad Clements]

Attachment: The expected html output

The output I expect from the transform. I generated this using libxslt

Comment 51

[Tracy Walker [:tracy]]

Verified on Firefox 1.5.0.2 buildsfrom 20060306 on Windows, Mac and Linux

Comment 52

[Daniel Veditz [:dveditz]]

we're using "fixed1.8.1" for the 1.8 branch post-1.8 (confusing, I know)