Closed
Bug 205778
Opened 22 years ago
Closed 19 years ago
document('') load of stylesheet conflicts with http cache
Categories
(Core :: XSLT, defect, P2)
Tracking
()
RESOLVED
FIXED
mozilla1.8beta4
People
(Reporter: rbs, Assigned: peterv)
References
()
Details
(4 keywords, Whiteboard: [rft-dl])
Attachments
(5 files, 3 obsolete files)
9.21 KB,
image/png
|
Details | |
3.75 KB,
patch
|
axel
:
review+
peterv
:
superreview+
asa
:
approval1.8b5-
sicking
:
approval-branch-1.8.1+
dveditz
:
approval1.8.0.2+
|
Details | Diff | Splinter Review |
817 bytes,
application/xml
|
Details | |
105 bytes,
application/xml
|
Details | |
354 bytes,
text/html
|
Details |
Bug 158457 has unfortunately regressed.
Assignee | ||
Updated•22 years ago
|
My trunk build hangs on the url here the same way as bug 205633. Related?
Comment 2•22 years ago
|
||
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
Comment 6•22 years ago
|
||
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•22 years ago
|
||
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. ***
Comment 9•22 years ago
|
||
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•22 years ago
|
||
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.
Summary: Transformation of MathML doesn't complete (bug 158457 regressed) → [MathML] document('') load of stylesheet conflicts with http cache
Comment 11•22 years ago
|
||
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•22 years ago
|
||
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•22 years ago
|
||
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?
Comment 15•22 years ago
|
||
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.
Reporter | ||
Comment 16•22 years ago
|
||
jst recently checked in something on this dummy stuff, perhaps attachment 123205 [details] [diff] [review] from bug 93015 might give some helpful insight.
Reporter | ||
Comment 17•22 years ago
|
||
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•22 years ago
|
||
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
![]() |
||
Comment 20•22 years ago
|
||
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•22 years ago
|
||
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.
Reporter | ||
Comment 22•22 years ago
|
||
There is another implementation of this dummy request in the parser: http://lxr.mozilla.org/seamonkey/source/content/html/document/src/nsHTMLContentSink.cpp#497
Reporter | ||
Comment 23•22 years ago
|
||
*** Bug 209869 has been marked as a duplicate of this bug. ***
Reporter | ||
Comment 24•22 years ago
|
||
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•22 years ago
|
||
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.
Assignee: peterv → axel
Comment 26•22 years ago
|
||
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•22 years ago
|
||
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.
Target Milestone: --- → mozilla1.5beta
Comment 28•21 years ago
|
||
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.
Updated•21 years ago
|
Attachment #139343 -
Flags: review?(peterv)
Comment 29•21 years ago
|
||
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.
Attachment #139343 -
Flags: review?(peterv)
*** Bug 279869 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 31•19 years ago
|
||
*** Bug 306439 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 32•19 years ago
|
||
Assignee | ||
Comment 33•19 years ago
|
||
Decided to pass PR_TRUE to UnblockOnload. Boris, does that seem right?
Attachment #194861 -
Attachment is obsolete: true
Attachment #195036 -
Flags: superreview?(bzbarsky)
Attachment #195036 -
Flags: review?(axel)
![]() |
||
Comment 34•19 years ago
|
||
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.
Attachment #195036 -
Flags: superreview?(bzbarsky) → superreview+
Assignee | ||
Comment 35•19 years ago
|
||
Changed following bz's comments. This one also reports the error if PostEvent fails. Not ideal, but it'll have to do for now.
Attachment #195036 -
Attachment is obsolete: true
Attachment #195159 -
Flags: superreview+
Attachment #195159 -
Flags: review?(axel)
Assignee | ||
Updated•19 years ago
|
Attachment #195036 -
Flags: review?(axel)
Assignee | ||
Comment 36•19 years ago
|
||
This isn't really specific to MathML.
Flags: blocking1.8b5?
Priority: -- → P2
Summary: [MathML] document('') load of stylesheet conflicts with http cache → document('') load of stylesheet conflicts with http cache
Target Milestone: mozilla1.5beta → mozilla1.8beta4
Updated•19 years ago
|
Attachment #195159 -
Flags: review?(axel) → review+
Assignee | ||
Updated•19 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 19 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 37•19 years ago
|
||
Comment on attachment 195159 [details] [diff] [review] v1.2 Fixes hang in XSLT.
Attachment #195159 -
Flags: approval1.8b5?
Comment 38•19 years ago
|
||
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.
Attachment #195159 -
Flags: approval1.8b5? → approval1.8b5-
Comment 39•19 years ago
|
||
clearing this flag per the delivery teams comment from yesterday.
Flags: blocking1.8b5? → blocking1.8b5-
Assignee | ||
Comment 40•19 years ago
|
||
*** Bug 317953 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 41•19 years ago
|
||
*** Bug 318563 has been marked as a duplicate of this bug. ***
Assignee | ||
Comment 42•19 years ago
|
||
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).
Attachment #195159 -
Flags: branch-1.8.1?(bugmail)
Comment 43•19 years ago
|
||
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.
Attachment #195159 -
Flags: branch-1.8.1?(bugmail) → branch-1.8.1+
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
Attachment #195159 -
Flags: approval1.8.0.2?
Updated•19 years ago
|
Flags: blocking1.8.0.2+
Comment 46•19 years ago
|
||
Comment on attachment 195159 [details] [diff] [review] v1.2 approved for 1.8.0 branch, a=dveditz
Attachment #195159 -
Flags: approval1.8.0.2? → approval1.8.0.2+
Assignee | ||
Updated•19 years ago
|
Keywords: fixed1.8.0.2
Comment 47•19 years ago
|
||
could someone please attach a simple testcase to this bug
Comment 48•19 years ago
|
||
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•19 years ago
|
||
This .xml file has a PI that loads the corresponding .xsl file.
Comment 50•19 years ago
|
||
The output I expect from the transform. I generated this using libxslt
Updated•19 years ago
|
Whiteboard: [rft-dl]
Comment 51•19 years ago
|
||
Verified on Firefox 1.5.0.2 buildsfrom 20060306 on Windows, Mac and Linux
Keywords: fixed1.8.0.2 → verified1.8.0.2
Comment 52•18 years ago
|
||
we're using "fixed1.8.1" for the 1.8 branch post-1.8 (confusing, I know)
Keywords: fixed1.8 → fixed1.8.1
You need to log in
before you can comment on or make changes to this bug.
Description
•