Bug 158457 has unfortunately regressed.

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

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

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

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.

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

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.

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.

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

That plevent relay would have to happen in txMozillaXSLTProcessor::DoTransform, I'd say. Peter and I agreed that sicking has to learn plevents. Harharhar.

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.

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.

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?

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.

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?

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.

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

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

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

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

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.

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.

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

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

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.

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.

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.

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.

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

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

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

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

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.

Changed following bz's comments. This one also reports the error if PostEvent fails. Not ideal, but it'll have to do for now.

This isn't really specific to MathML.

Comment on attachment 195159 [details] [diff] [review] v1.2 Fixes hang in XSLT.

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.

clearing this flag per the delivery teams comment from yesterday.

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

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

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).

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 on attachment 195159 [details] [diff] [review] v1.2 Yeah, it looks safe enough.

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 on attachment 195159 [details] [diff] [review] v1.2 approved for 1.8.0 branch, a=dveditz

could someone please attach a simple testcase to this bug

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.

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

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

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

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