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

RESOLVED FIXED in mozilla1.8beta4

Status

()

Core
XSLT
P2
critical
RESOLVED FIXED
14 years ago
11 years ago

People

(Reporter: rbs, Assigned: peterv)

Tracking

(4 keywords)

Trunk
mozilla1.8beta4
x86
All
fixed1.8.1, hang, regression, verified1.8.0.2
Points:
---
Bug Flags:
blocking1.8b5 -
blocking1.8.0.2 +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [rft-dl], URL)

Attachments

(5 attachments, 3 obsolete attachments)

(Reporter)

Description

14 years ago
Bug 158457 has unfortunately regressed.
(Assignee)

Updated

14 years ago

Comment 1

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

Comment 2

14 years ago
Created attachment 123392 [details]
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
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

14 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

14 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.
(Reporter)

Comment 8

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

Comment 9

14 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

14 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

14 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

14 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

14 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

14 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

14 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

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

Comment 21

14 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

14 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

14 years ago
*** Bug 209869 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 24

14 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

14 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

14 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

14 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

14 years ago
Created attachment 139343 [details]
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.

Updated

14 years ago
Attachment #139343 - Flags: review?(peterv)

Comment 29

13 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

12 years ago
*** Bug 306439 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 32

12 years ago
Created attachment 194861 [details] [diff] [review]
v1
Assignee: axel → peterv
Attachment #139343 - Attachment is obsolete: true
Status: NEW → ASSIGNED
(Assignee)

Comment 33

12 years ago
Created attachment 195036 [details] [diff] [review]
v1.1

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

12 years ago
Created attachment 195159 [details] [diff] [review]
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.
Attachment #195036 - Attachment is obsolete: true
Attachment #195159 - Flags: superreview+
Attachment #195159 - Flags: review?(axel)
(Assignee)

Updated

12 years ago
Attachment #195036 - Flags: review?(axel)
(Assignee)

Comment 36

12 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
(Assignee)

Updated

12 years ago
Keywords: hang

Updated

12 years ago
Attachment #195159 - Flags: review?(axel) → review+
(Assignee)

Updated

12 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 12 years ago
Resolution: --- → FIXED
(Assignee)

Comment 37

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

Fixes hang in XSLT.
Attachment #195159 - Flags: approval1.8b5?

Comment 38

12 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

12 years ago
clearing this flag per the delivery teams comment from yesterday.
Flags: blocking1.8b5? → blocking1.8b5-
(Assignee)

Comment 40

12 years ago
*** Bug 317953 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 41

12 years ago
*** Bug 318563 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 42

12 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

12 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?
(Assignee)

Updated

12 years ago
Keywords: fixed1.8
Flags: blocking1.8.0.2+
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

12 years ago
Keywords: fixed1.8.0.2
could someone please attach a simple testcase to this bug

Comment 48

12 years ago
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

12 years ago
Created attachment 213110 [details]
The input xml file

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

Comment 50

12 years ago
Created attachment 213111 [details]
The expected html output

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

Updated

12 years ago
Whiteboard: [rft-dl]
Verified on Firefox 1.5.0.2 buildsfrom 20060306 on Windows, Mac and Linux
Keywords: fixed1.8.0.2 → verified1.8.0.2
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.