Closed Bug 515051 Opened 10 years ago Closed 10 years ago

Stream listener registered in a network request channel eats JS error messages.

Categories

(Core :: Networking, defect, P2)

1.9.1 Branch
x86
Windows XP
defect

Tracking

()

RESOLVED FIXED
Tracking Status
status1.9.2 --- beta4-fixed

People

(Reporter: Honza, Assigned: mayhemer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [firebug-p1][firebug-blocks])

Attachments

(5 files, 7 obsolete files)

2.82 KB, application/x-xpinstall
Details
2.69 KB, application/x-xpinstall
Details
13.30 KB, patch
Details | Diff | Splinter Review
14.75 KB, patch
Details | Diff | Splinter Review
v2
13.99 KB, patch
Details | Diff | Splinter Review
Attached file Test case extension. (obsolete) —
This bug has been originally reported as Firebug issue 2271
http://code.google.com/p/fbug/issues/detail?id=2271

When looking at the problem I have found that if nsIStreamListener is registered for a network channel (using nsITraceableChannel) Firefox doesn't display error messages that occurs in XHR handler on a web page.

For instance, there is an error in the following snippet of a source code (the getElementId doesn't exist), which is not displayed.

var request = new XMLHttpRequest();
request.open("GET", "issue2271.php", true);

request.onreadystatechange = function()
{
    if (request.readyState == 4 && request.status == 200)
    {
        document.getElementId("something").innerText = request.responseText;
    }
}

request.send(null);


I am attaching a test extension.
Tested with Firefox 3.0.12: WORKS
Tested with Firefox 3.5.4pre: BUG

Steps to reproduce:

1) Install the attached extension in Fx 3.5.4pre
2) Load http://www.janodvarko.cz/firebug/tests/2271/issue2271.html
3) Press button on the page
4) See Firefox->Tools->Error Console
5) The error isn't displayed -> BUG

Honza
Whiteboard: [firebug-p1]
Summary: nsITraceableChannel eats error messages. → Stream listener registered in a network request channel eats error messages.
Summary: Stream listener registered in a network request channel eats error messages. → Stream listener registered in a network request channel eats JS error messages.
Since Necko doesn't deal with JS errors, -> xpconnect
Component: Networking → XPConnect
QA Contact: networking → xpconnect
Honza, does this fail on 3.6 or 3.7 nightly builds? I think we have a better chance to get a fix on 3.7 and back port than just for 3.5.
Tested with 3.7a1pre: FAILS
Tested with 3.6a1pre: FAILS

Honza
Attachment #399072 - Attachment is obsolete: true
On 3.5.2 the error message comes into jsd.onError() so the error is probably caught and not re-dispatched.  Isn't this related to the throw-means-cancel problem?
I think the problem is here:
http://mxr.mozilla.org/mozilla-central/source/dom/base/nsJSEnvironment.cpp#449

As I read the code this means that JS complemented components will not report errors?
Blocks: 435025
Still trying to wrap my head around this, but it sounds like anything for which we have an nsITraceableChannel listener. This is marked as P1 so I'm going to mark it wanted and blocking.
Flags: wanted1.9.2?
Flags: blocking1.9.2?
Sounds like a duplicate of the general "calls through XPConnect into JS-implemented components will eat exceptions" thing that I'm sure we have a bug on.
Whiteboard: [firebug-p1] → [firebug-p1]DUPEME
Firebug could set the bool pref "dom.report_all_js_exceptions" to see these, I think. Otherwise this behavior is a by-design compromise between those who feel strongly that exceptions from inner frames are "handled" by their C++ callers and those who feel that each frame of JS is an island unto itself.
(In reply to comment #8)
> Otherwise this behavior is a by-design compromise between those who feel
> strongly that exceptions from inner frames are "handled" by their C++ callers
> and those who feel that each frame of JS is an island unto itself.

Is there some where I can read more about this? I don't see what this has to do with C++ or frames of JS. I'm with Boris: this is yet another "XPConnect fails to deal with exceptions" problem.
What's likely going on here is that we have a situation where we have a call
chain like

JS (firebugs network thing) -> C++ (Gecko) -> JS (Webpage)

If the inner javascript (webpage) throws an error, then, as the exception
transitions into C++, XPConnect sees that there is outer JS, and does not
report the error to the console.

The reason XPConnect behaves this way is that in the situation where we have:

JS -> Some C++ -> JS

and the inner JS throws, which gets propagated through the C++, but caught and
handled by the outer JS, then people did not want to see warnings in the
console.


One possible fix is to somehow make an exception for specific firebug JS files,
so that they don't count as "outer JS".

Of course, my recollection last I looked at this was that the way that firebug
hooked into necko was really hacky, so possibly we could try to get rid of that
instead?
(In reply to comment #10)
> What's likely going on here is that we have a situation where we have a call
> chain like
> 
> JS (firebugs network thing) -> C++ (Gecko) -> JS (Webpage)

I believe the issue here is

JS (web page ajax handler) -> C++ (Gecko) -> JS (Webpage)
                  ^---the error is in here       

> If the inner javascript (webpage) throws an error, then, as the exception
> transitions into C++, XPConnect sees that there is outer JS, and does not
> report the error to the console.

I don't understand why this works when Firebug is not involved, since in that case I can't follow the C++ code.

> 
> The reason XPConnect behaves this way is that in the situation where we have:
> 
> JS -> Some C++ -> JS
> 
> and the inner JS throws, which gets propagated through the C++, but caught and
> handled by the outer JS, then people did not want to see warnings in the
> console.

But it does not propagate through and get caught...

I believe that in this case the exceptions can at most cause the request to be canceled. 
 
> One possible fix is to somehow make an exception for specific firebug JS files,
> so that they don't count as "outer JS".
> 
> Of course, my recollection last I looked at this was that the way that firebug
> hooked into necko was really hacky, so possibly we could try to get rid of that
> instead?

Sorry, we already did that, which is why we get this problem only in Firebug 1.4+ and not in 1.3.
(In reply to comment #11)
> (In reply to comment #10)
> > What's likely going on here is that we have a situation where we have a call
> > chain like
> > 
> > JS (firebugs network thing) -> C++ (Gecko) -> JS (Webpage)
> 
> I believe the issue here is
> 
> JS (web page ajax handler) -> C++ (Gecko) -> JS (Webpage)
>                   ^---the error is in here       

Really? From comment 0:

> When looking at the problem I have found that if nsIStreamListener is
> registered for a network channel (using nsITraceableChannel) Firefox doesn't
> display error messages that occurs *in XHR handler on a web page*.

Emphasis mine. The comment further displays code, that seems to come from the web page, having an error, thus causing an exception to get thrown.


> > The reason XPConnect behaves this way is that in the situation where we have:
> > 
> > JS -> Some C++ -> JS
> > 
> > and the inner JS throws, which gets propagated through the C++, but caught and
> > handled by the outer JS, then people did not want to see warnings in the
> > console.
> 
> But it does not propagate through and get caught...

The decision to report or not has to be made at the point when the exception transitions from JS code to C++ code though. At that point we don't know if the C++ code will catch the error or not. It appears that in this case the exception gets caught somewhere in gecko. Which makes sense, I don't think we want to propagate errors in the readystatechange event handler all the way up to necko.

> > One possible fix is to somehow make an exception for specific firebug JS files,
> > so that they don't count as "outer JS".
> > 
> > Of course, my recollection last I looked at this was that the way that firebug
> > hooked into necko was really hacky, so possibly we could try to get rid of that
> > instead?
> 
> Sorry, we already did that, which is why we get this problem only in Firebug
> 1.4+ and not in 1.3.

Huh? If you got rid of the hacky necko-hooking, wouldn't it be the other way around?
Well firebug can't avoid Necko-hooking, since it wants to see the response body.
It's software, anything's possible ;)

We could add specific hooks for allowing inspection of response bodies, which wouldn't force the user of the hook to actually be on the stack during the channel callbacks.

IIRC (though it's quite likely that I do not), firebug replaced the streamlistenertee object with its own JS object. That's the part that felt hacky to me (if for no other reason, becuase streamlistenertees could be used in any number of places that firebug has no interest in listening to)
That might be what it used to do (I don't know), but now it uses the API that was designed for letting extensions know about stream contents, i.e. http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsITraceableChannel.idl

Wouldn't other ways of letting firebug know the contents require that the entire response be buffered in memory?
(In reply to comment #15)
> That might be what it used to do (I don't know), but now it uses the API that
> was designed for letting extensions know about stream contents, i.e.
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsITraceableChannel.idl

Ah, nice. One possible fix would be to change the API so that instead of replacing, and thus wrapping, the existing streamlistener, you can instead get notified in a fork-like fashion.

> Wouldn't other ways of letting firebug know the contents require that the
> entire response be buffered in memory?

Not sure I follow you.
(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > What's likely going on here is that we have a situation where we have a call
> > > chain like
> > > 
> > > JS (firebugs network thing) -> C++ (Gecko) -> JS (Webpage)
> > 
> > I believe the issue here is
> > 
> > JS (web page ajax handler) -> C++ (Gecko) -> JS (Webpage)
> >                   ^---the error is in here       
> 
> Really? From comment 0:
> 
> > When looking at the problem I have found that if nsIStreamListener is
> > registered for a network channel (using nsITraceableChannel) Firefox doesn't
> > display error messages that occurs *in XHR handler on a web page*.
> 
> Emphasis mine. The comment further displays code, that seems to come from the
> web page, having an error, thus causing an exception to get thrown.

Yes, that is what I meant. The web page code has the error, nor firebug's network thing as your first diagram implied.

> 
...
> > Sorry, we already did that, which is why we get this problem only in Firebug
> > 1.4+ and not in 1.3.
> 
> Huh? If you got rid of the hacky necko-hooking, wouldn't it be the other way
> around?

This is our third generation hack, so I'm not sure which one you don't like ;-). Maybe Odvarko can help us sort this out.
> This is our third generation hack, so I'm not sure which one you don't like
> ;-). Maybe Odvarko can help us sort this out.

The most "hacky" code in Firebug was related to how response bodies are gathered (btw. causing the famous double-load problem in the past, when Firebug was trying to get the response body by executing another net reqeuest). This code is now removed and replaced by utilizing the nsITraceableChannel. This interface allows to register a nsIStreamListener to a request and so, get entire response body.

Unfortunately, using this interface causes this bug.

The nsIStreamListener is currently the only way how to effectively get the response body (which is critical for Firebug) and so, I don't see any workaround. In fact avoiding the nsITraceableChannel would be step back.

Honza
Yeah, what I'm suggesting is to change the nsITracableChannel interface such that it can be used without causing this bug. As one option for fixing this bug...
> One possible fix would be to change the API so that instead of
> replacing, and thus wrapping, the existing streamlistener, you can instead get
> notified in a fork-like fashion.
I think this is a good way to solve the problem.

I believe that the request.setNewListener(newListener); (nsITraceableChannel) should return null (or undefined) in the new implementation so existing users can't call the original listener (which is the concept now) and thus break the new concept.

One bad side of the "fork-like" solution is that listeners won't be able to modify the received content. But I am not sure how useful this really was (especially in case of chunked responses).

Honza
I think it's useful to be able to modify the response. How do chunked responses change anything? That said, I'm not opposed to providing one interface for read-only observers and one for read-write ones.
honza, any thoughts?
Blocking as Firebug compatibility is a requirement for release; is this still thought to be a likely dupe, or should the DUPEME be removed?
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
not sure. This sounds like some of the other "error message" bugs but can't find a specific one in my collection. I'm going to remove the dupeme and replace it with firebug-blocks.
Whiteboard: [firebug-p1]DUPEME → [firebug-p1][firebug-blocks]
It sounds like the solution to this will be found in necko, so re-componenting.  Not sure who's going to fix it still, though. :-/
Component: XPConnect → Networking
QA Contact: xpconnect → networking
> I think it's useful to be able to modify the response.
Yes, agree.

> How do chunked responses change anything?
I have got some feedback from developers who wanted to filter the received content and e.g. remove/modify some specific parts of HTML responses. In case of chunked content it's hard to decide how the part, which isn't complete at the moment, should be modified. But, perhaps this requirement is out of scope of the interface.

Honza
> One possible fix would be to change the API so that instead of
> replacing, and thus wrapping, the existing streamlistener, you can instead get
> notified in a fork-like fashion.

If all you want is the data, you can do this already.  Just use an nsIStreamListenerTee as the listener you pass to setNewListener.
And if you really want the start/stop notifications, we could probably modify the stream listener tee pretty easily to allow that.
Another related thing is that the inputStream passed into the  nsIStreamListener.onDataAvailable doesn't implement nsISeekableStream interface. This means that if the stream is read, data must be cloned into another stream, so other listeners in the chain can also read it. This isn't good for performance and it also makes the code more complicated.

Should I report a new bug for this?

Honza
the stream is the input end of the pipe. if you want to read the data and also make it available to the other listeners, the data will have to be copied somewhere in any case. it's not like the entire response is kept in memory.
(In reply to comment #26)
> > How do chunked responses change anything?
> I have got some feedback from developers who wanted to filter the received
> content and e.g. remove/modify some specific parts of HTML responses. In case
> of chunked content it's hard to decide how the part, which isn't complete at
> the moment, should be modified. But, perhaps this requirement is out of scope
> of the interface.

But you have the same problem without chunked encoding...
Honza: I would say a new bug, yeah.
Honza (Bambas) said he'd work on this one. Reassigning.
Assignee: nobody → honzab.moz
Jan, can you clearly describe the requirements Firebug has here?  Is it just trying to look at the data as it goes by?  Modify the data the real consumer gets?  Something else?
Firebug is using the nsITraceableChannel & nsIStreamListener to intercept network responses and copying them into an internal cache (nothing is modified, just cloned for later usage). This internal cache is used (a) by the Net panel to show network responses and (b) by the Script panel to show Javascript - to the user.

Received data are stored into the Firebug's cache as it goes by (within the onDataAvailable). E.g. if there is an error during the page load and Firebug is set to break on that error, the source code must be already available in the FB cache (even if the page is not fully loaded yet) and so the Script panel can properly display it to the user.

And to summarize the original reported problem (comment 0): If a nsIStreamListener is registered (using nsITraceableChannel), some errors are not displayed in the Firefox Error Console.

Honza
OK, if that's your only use case then you should be using a stream listener tee.  Then your JS code won't be calling into other people's code directly, and you won't have the exception-eating problems.  As a bonus, your code can be simpler.

Of course I already said all that in comment 27...
I see, sounds promising. So, I guess the only thing we need is contract for the input stream tee - e.g. "@mozilla.org/network/input-stream-tee;1".

I believe the contract is not available yet.

This would also solve the problem described in 
https://bugzilla.mozilla.org/show_bug.cgi?id=526497

Thanks!
Honza
No, you don't need a contract for the input stream tee for this bug.  You want to use a stream listener tee here, not an input stream tee; the latter won't resolve this issue. Stream listener tee already has a contract.  And yes, using a stream listener tee would also solve bug 526497.
I see, understood.
Honza
So, Honza also needs to eavesdrop on OnStartRequest and OnStopRequest notifications (at least). Stream listener tee doesn't provide it, it just writes data to the provided output stream. I haven't found an interface (nor a component) that could do this, so, I have to write a new one, e.g.:

nsIStreamListenerEavesdropper : nsIStreamListener
{
  void init(in nsISreamListener target, in nsIRequestObserver eavesdropper, in nsIOutputStream captureStream)
}

Implementation then:
nsStreamListenerEveasdropper::Init(target, eaves, stream)
{
  mTee = new streamlistenertee
  mTee->init(target, stream)
  mEavesdropper = eaves
}

nsStreamListenerEveasdropper::OnStartRequest(...)
{
  rv = mTee->OnStartRequest()
  mEavesdropper->OnStartRequest()
  return rv
}

the same for OnStopRequest, there would be no delegation to the eavesdropped in OnDataAvailable, as there is no need and doesn't make sense to have it.

nsITraceableChannel.setNewListener then changes like this (if not even the name as well):

void setNewListener(nsIRequestObserver eavesdropper, nsIOutputStream captureStream)

and implementation:
{
  x = new nsStreamListenerEveasdropper()
  x->init(mListneer, eavesdropper, captureStream)
  mListener = x
}

Comments welcome.
Status: NEW → ASSIGNED
> So, Honza also needs to eavesdrop on OnStartRequest and OnStopRequest
> notifications (at least)

See comment 28.  Would be nice to not have this one-week communication lag... 

Why do we need to change nsITraceableChannel at all?  If we have this nsIStreamListenerEavesdropper (which I still claim we could just extend nsIStreamListenerTee with), it could just be passed to setNewListener as-is, no?
(In reply to comment #42)
> See comment 28.  Would be nice to not have this one-week communication lag... 
> 

Ah, I have to read more carefully next time.

> Why do we need to change nsITraceableChannel at all?  If we have this
> nsIStreamListenerEavesdropper (which I still claim we could just extend
> nsIStreamListenerTee with), it could just be passed to setNewListener as-is,
> no?

Yes, I can modify nsStreamListenerTee according to comment 41. It's not that widely used.
So, just to summarize the proposed solution (also to make sure I understand).

1) In order to intercept received response from the server an nsIStreamListener must be registered within a request (using nsITraceableChannel.setNewListener)

2) The "@mozilla.org/network/stream-listener-tee;1" is used as the listener. The result is that there is no JS code incorporated in the listeners-chain and exceptions are properly propagated (which solves this bug).

3) The response data are written into a sink (nsIOutputStream) that is registered within the listener-tee (using init method).

4) The onStartRequest and onStopRequest events are forwarded to an observer (nsIRequestObserver) that is registered within the tee-listener (also using  the init method?). 

Am I correct so far?

The only problem I see is that Firebug is currently copying/storing data into the internal cache as they come (i.e. within onDataAvailable) - in order to limit its size if necessary. If there is no onDataAvailable, the entire response must be always copied and Firebug must cut it in onStopRequest.

Also Firebug gets the response content type in onStartRequest (per 489317) and decides whether to cache or not in onDataAvailable. If onDataAvailable is not available, the response can be sometimes unnecessary copied into the outputstream.

Honza
Just having a quick chat with J.Bambas and there is a good chance that both problems from comment 44 are solvable:

1) Firebug could close the sink (nsIOutputStream) in onStartRequest if the response shouldn't be cached.

2) Firebug could set max size for the sink and so, only specific amount of data is cached.

Sounds good to me!

I'll test all with Firebug as soon as a patch is available here.
Honza
You could also set a pipe as the sink and only read as much data as you want, and close the pipe if you want no more data.
(In reply to comment #46)
> You could also set a pipe as the sink and only read as much data as you want,
> and close the pipe if you want no more data.
Ah, this sounds even better, thanks for the tip!
Honza
(In reply to comment #46)
> You could also set a pipe as the sink and only read as much data as you want,
> and close the pipe if you want no more data.

I don't agree. I suggest you use nsIStorageStream  ("@mozilla.org/storagestream;1") as the output sink. The storage stream is better imho for this use case. When you read from the pipe, you loose the data. From storage stream you can read more then ones. Depends on, how you want to use the data, however, the storage stream is also simpler to use.

> 4) The onStartRequest and onStopRequest events are forwarded to an observer
> (nsIRequestObserver) that is registered within the tee-listener (also using 
> the init method?). 
> 

This is the functionality I have to update. Will have a patch soon.
Note that with a storage stream you also have to be very careful to not effectively leak all the data.
Attached patch v1 for m-c (no test) (obsolete) — Splinter Review
Stream listener tee is now capable to take additional optional parameter in its init method, nsIRequestObserver object that receives onStartRequest and onStopRequest.

1.9.2 patch coming soon.
Attachment #411524 - Flags: review?(bzbarsky)
Attached patch v1 for 1.9.2 (obsolete) — Splinter Review
Honza, please test on 1.9.2 branch.
Attachment #411524 - Flags: review?(bzbarsky) → review+
Comment on attachment 411714 [details] [diff] [review]
v1 for 1.9.2

Looks fine; no need for explicit = null in init.

And please do write some tests, ok?
Attachment #411714 - Flags: review+
Comment on attachment 411714 [details] [diff] [review]
v1 for 1.9.2

Oh, and call the new interface's method initWithObserver or something; otherwise which init you get starts depending on xpconnect implementation details...
Ah, true, I have already that experience... Will create a new patch with a test.
You really should document in the interface that you're not following the nsIRequestObserver contract (i.e., an exception from onStartRequest doesn't cancel the request)

Better yet, you should follow that interface.

Might also be a good to idea to add a brief comment about how the request observer is used, since you also have the listener argument...
(In reply to comment #55)
> Better yet, you should follow that interface.
> 

You mean to propagate the exception to the caller? To cancel the request when an exception is threw in the eavesdropper's listener? I'm not sure it's a good idea.

> Might also be a good to idea to add a brief comment about how the request
> observer is used, since you also have the listener argument...

Ok, will do that.
Attached patch v1.1 (obsolete) — Splinter Review
Improoved version according to comment 55, with modified test for traceable channel.
Attachment #411524 - Attachment is obsolete: true
Attached patch v1.1 for 1.9.2 (obsolete) — Splinter Review
And for the 1.9.2 branch.
Attachment #411714 - Attachment is obsolete: true
Attachment #411976 - Flags: review?(cbiesinger)
Attachment #411971 - Flags: review?(cbiesinger)
(In reply to comment #56)
> You mean to propagate the exception to the caller? To cancel the request when
> an exception is threw in the eavesdropper's listener? I'm not sure it's a good
> idea.

Yeah. Why not let observers cancel the channel?
(In reply to comment #59)
> Yeah. Why not let observers cancel the channel?
I already did it that way. Please check the patches officially.
Attachment #411971 - Flags: review?(cbiesinger) → review+
Comment on attachment 411971 [details] [diff] [review]
v1.1

+    nsresult rv1 = mListener->OnStopRequest(request, context, status);
+    nsresult rv2 = NS_OK;
+    if (mObserver)
+        rv2 = mObserver->OnStopRequest(request, context, status);

In OnStopRequest, you don't have to bother with rv1/rv2. Channels ignore return values there.

+++ b/netwerk/test/unit/test_traceable_channel.js

+      pipe = Cc["@mozilla.org/pipe;1"].createInstance(Ci.nsIPipe);
+      pipe.init(false, false, 1024, 5, null);

I think it'd be better to keep using a storage stream or use an infinite-sized pipe... while a limited-size pipe works for this specific case, it's just a recipe for people shooting themselves in the foot when they copy this code.
Comment on attachment 411976 [details] [diff] [review]
v1.1 for 1.9.2

r+ with the same comments as the trunk patch. I don't think it's useful to attach branch patches before the trunk patch has been reviewed, but maybe that's just me...
Attachment #411976 - Flags: review?(cbiesinger) → review+
(In reply to comment #61)
> In OnStopRequest, you don't have to bother with rv1/rv2. Channels ignore
> return values there.
> 

OK

> +++ b/netwerk/test/unit/test_traceable_channel.js
> I think it'd be better to keep using a storage stream or use an infinite-sized
> pipe... while a limited-size pipe works for this specific case, it's just a
> recipe for people shooting themselves in the foot when they copy this code.

I got assertion failure here:
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsInputStreamTee.cpp#181

because of this:
http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsStorageStream.cpp#241

but an infinite pipe is probably better.
Interesting. I suppose it's arguable whether storage streams are nonblocking. Anyway, in that case it does seem better to use a pipe.
(In reply to comment #64)
> Interesting. I suppose it's arguable whether storage streams are nonblocking.
Create a bug for this? I'd say it has blocking behavior. It will never throw WOULD_BLOCK error and when the buffer is full we get (also interesting) NS_ERROR_OUT_OF_MEMORY error.

> Anyway, in that case it does seem better to use a pipe.
Agree.
On the other hand, the stream doesn't actually block either. It (the input stream) always has data available or is at EOF - it never blocks waiting for data. nsStringStream.cpp also returns true for IsNonBlocking... but yeah, I think it'd make sense to change that to false.
I have tested the "v1.1 for 1.9.2" patch (in 1.9.2) and all work fine till I limit the pipe size. In such a case Firefox freezes when the response is bigger than the limit.

Here is my code I am using to limit the pipe to get max 100 bytes.
pipe.init(false, false, 100, 1, null);

Honza
Don't you want nonblocking behavior for a limited-size pipe?
Just tried that and if I set the nonBlockingOutput to true, Firefox crashes (disappears immediately) for me.

Honza
Uh... under what conditions?  I'm surprised that you don't just hang with the blocking pipe, when trying to write more than 100 bytes into it...
Here is my STR:

1) Install the attached extension 
https://bugzilla.mozilla.org/attachment.cgi?id=412225
2) Load any page (I was testing e.g. with www.google.com)
3) Firefox disappears when the page is trying to load.

The pipe is initialized as follows:

var pipe = Components.classes["@mozilla.org/pipe;1"]
   .createInstance(Components.interfaces.nsIPipe);
pipe.init(false, true, 100, 1, null);

(see the example)

It would be nice if somebody can verify (could be a problem with my build-?)
Honza
Uh...  Works fine for me.  That extension code doesn't even run on pageloads, does it?
Attached file Firefox crash test. (obsolete) —
Please try this extension. I missed chrome.manifest in 412225.
Honza
Attachment #412225 - Attachment is obsolete: true
Ah.  _And_ I need to be testing on 1.9.2, _and_ I need to apply your 1.9.2 patch first.  Not clear to me why I had to go read the extension code to get the actual steps to reproduce...

So nsIInputStreamTee does document that its sink must be blocking.  nsIStreamListenerTee does not, but of course uses nsIInputStreamTee.

So nsInputStreamTee::SetSink asserts, nsInputStreamTee::TeeSegment gets NS_BASE_STREAM_WOULD_BLOCK and sets mSink to 0.  That's sad.

All fine, but the crash happens because of an infinite recursion:

(gdb) frame 1
#1  0x00e22e06 in nsStreamListenerWrapper::OnStartRequest (this=0x18fd86d0, aRequest=0x18fdc300, aContext=0x0) at /Users/bzbarsky/mozilla/1.9.2-branch/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp:5904
5904        NS_FORWARD_NSIREQUESTOBSERVER(mListener->)
(gdb) p this
$19 = (nsStreamListenerWrapper *) 0x18fd86d0
(gdb) p this->mListener.mRawPtr
$20 = (nsStreamListenerTee *) 0x1f1e6370
(gdb) p this->mListener.mRawPtr->mListener.mRawPtr
$21 = (nsStreamListenerWrapper *) 0x18fd86d0

And _that_ happens because your extension is just buggy: it uses CCSV to get the stream listener tee, which means it always gets the same tee object.  If it inserts a tee twice (most easily on a 3xx redirect, which is what you probably hit with google), it'll insert the same object twice and then you lose.

Of course if you make the output stream for that pipe blocking you lose due to the browser freezing, as expected.

Conclusion: either use a blocking infinite-length pipe, or make sure something is async (on a different thread, since the main thread will block) reading from that blocking finite-length pipe or something is async (different thread) closing the blocking finite-length pipe.  And don't getService random things that aren't services.
Honza, try the storage stream. If you can modify the source code, then change result value here [1] to PR_FALSE or just let the assertion be ignored by setting env var XPCOM_DEBUG_BREAK=warn.

[1] http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsStorageStream.cpp#241
Yeah... I wouldn't expect a limited-size pipe to work, as mentioned in comment 61. Are you saying there's a bug here?
Honza requires a size limitation, so we have to figure this out.

Definitely, the pipe won't work here. When it tries to write more then is a buffer limit, nsPipe::GetWriteSegment returns NS_BASE_STREAM_WOULD_BLOCK. nsPipeOutputStream::WriteSegments then calls nsPipe::Wait method, that indefinitely waits for the pipe's monitor. It will block until we read from the input stream and free some space, that never happens...

To use it in a non-blocking setting is probably a way (a bit nasty) but input stream tee doesn't accept it.

We have to fix the storage stream imho.
> Conclusion: either use a blocking infinite-length pipe, or make sure something
> is async (on a different thread, since the main thread will block) reading from
> that blocking finite-length pipe or something is async (different thread)
> closing the blocking finite-length pipe.
Yes, infinite pipe would be a workaround. Notice that I am a Javascript so all my code is on the UI thread.

> And don't getService random things that aren't services.
Sorry (copy-paste mistake).

Honza
(In reply to comment #75)
If I change the nsStorageStream::IsNonBlocking to return PR_FALSE and use nsIStorageStream for the sink it works.

The problem is that I can't limit the size of the buffer.
I am using following statement:
storageStream.init(8192, 15, null);
But this doesn't limit the buffer to 15 bytes as I would expect.

If I set the segment size to eg. 8 it works, but the segment size must be power of 2, so I am not flexible in the limit size.

// Segment size must be a power of two
if (mSegmentSize != ((PRUint32)1 << mSegmentSizeLog2))
    return NS_ERROR_INVALID_ARG;

It looks like fixing the pipe would be the best solution.

Honza
There isn't anything to fix in the pipe...  If you want a custom behavior that none of the existing stream classes provide (and it sounds like you do) then you should probably implement a custom stream class yourself (it could, e.g. write a limited amount of data into a pipe and then claim to be closed).
Does xpconnect let you implement interfaces in JS that have a noscript function? I'm also not sure how well write() would work if the input data contains nulls...
Oh, hmm.  Yeah, ok.
Attached file Test Case 4
I have done some further testing of the patch 'v1.1 for 1.9.2' with Firebug and here are some results.

1) I have registered the stream-listener-tee into a request (using nsITraceableChannel) and implemented a JS wrapper for nsIOutputStream (sink) that is passed in to the InitWithObserver method.

2) I have used the wrapper for to limit the size of the cached response and I have experienced an exception (probably predicted in comment #81).
See the following stack trace.

 	msvcr90d.dll!_strlen()  + 0x30 bytes	
 	js3250.dll!JS_NewStringCopyZ(JSContext * cx=0x00df07d8, const char * s=0x08ef3fa8)  Line 5266 + 0x9 bytes	C++
>	xpc3250.dll!XPCConvert::NativeData2JS(XPCLazyCallContext & lccx={...}, int * d=0x0012f28c, const void * s=0x0012f498, const nsXPTType & type={...}, const nsID * iid=0x0012f34c, JSObject * scope=0x05533180, unsigned int * pErr=0x00000000)  Line 362 + 0xe bytes	C++
 	xpc3250.dll!XPCConvert::NativeData2JS(XPCCallContext & ccx={...}, int * d=0x0012f28c, const void * s=0x0012f498, const nsXPTType & type={...}, const nsID * iid=0x0012f34c, JSObject * scope=0x05533180, unsigned int * pErr=0x00000000)  Line 2974 + 0x24 bytes	C++
 	xpc3250.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper=0x0658ed30, unsigned short methodIndex=0x0005, const XPTMethodDescriptor * info=0x01a29ef0, nsXPTCMiniVariant * nativeParams=0x0012f498)  Line 1570 + 0x31 bytes	C++
 	xpc3250.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=0x0005, const XPTMethodDescriptor * info=0x01a29ef0, nsXPTCMiniVariant * params=0x0012f498)  Line 571	C++
 	xpcom_core.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x08dc8450, unsigned int methodIndex=0x00000005, unsigned int * args=0x0012f558, unsigned int * stackBytesToPop=0x0012f548)  Line 114 + 0x21 bytes	C++
 	xpcom_core.dll!SharedStub()  Line 142	C++
 	xpcom_core.dll!nsInputStreamTee::TeeSegment(const char * buf=0x08ef3fa8, unsigned int count=0x00000015)  Line 80 + 0x2b bytes	C++
 	xpcom_core.dll!nsInputStreamTee::WriteSegmentFun(nsIInputStream * in=0x065298e8, void * closure=0x07deea28, const char * fromSegment=0x08ef3fa8, unsigned int offset=0x00000000, unsigned int count=0x00000015, unsigned int * writeCount=0x0012f5f4)  Line 110	C++
 	xpcom_core.dll!nsStringInputStream::ReadSegments(unsigned int (nsIInputStream *, void *, const char *, unsigned int, unsigned int, unsigned int *)* writer=0x00488190, void * closure=0x07deea28, unsigned int aCount=0x00000015, unsigned int * result=0x0012f5f4)  Line 276 + 0x22 bytes	C++
 	xpcom_core.dll!nsInputStreamTee::ReadSegments(unsigned int (nsIInputStream *, void *, const char *, unsigned int, unsigned int, unsigned int *)* writer=0x03114310, void * closure=0x093e85d8, unsigned int count=0x00000015, unsigned int * bytesRead=0x0012f5f4)  Line 157	C++
 	gklayout.dll!nsXMLHttpRequest::OnDataAvailable(nsIRequest * request=0x08bda650, nsISupports * ctxt=0x00000000, nsIInputStream * inStr=0x07deea28, unsigned int sourceOffset=0x00000000, unsigned int count=0x00000015)  Line 1886	C++


3) I have also used the wrapper to store parts of the response as they come (within the nsIOutputStream.write method). I don't have the test case yet, but if there is an JS error on the page and Firebug is set to "Break on Errors", the debugger halts during the page load and Firebug needs the source code immediately.

I was also experiencing an exception when doing this.



 	msvcr90d.dll!__VEC_memcpy()  + 0x140 bytes	
 	msvcr90d.dll!__VEC_memcpy()  + 0x52 bytes	
>	xpcom_core.dll!nsSegmentedBuffer::AppendNewSegment()  Line 108 + 0x12 bytes	C++
 	xpcom_core.dll!NS_InvokeByIndex_P(nsISupports * that=0x08322674, unsigned int methodIndex=0x00000005, unsigned int paramCount=0x00000003, nsXPTCVariant * params=0x0012d0e8)  Line 103	C++
 	xpc3250.dll!XPCWrappedNative::CallMethod(XPCCallContext & ccx={...}, XPCWrappedNative::CallMode mode=CALL_METHOD)  Line 2721 + 0x21 bytes	C++
 	xpc3250.dll!XPC_WN_CallMethod(JSContext * cx=0x00ee4ff8, JSObject * obj=0x06f80920, unsigned int argc=0x00000002, int * argv=0x022ff920, int * vp=0x0012d3ac)  Line 1740 + 0xe bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x00ee4ff8, unsigned int argc=0x00000002, int * vp=0x022ff918, unsigned int flags=0x00000002)  Line 1360 + 0x17 bytes	C++
 	js3250.dll!js_Interpret(JSContext * cx=0x00ee4ff8)  Line 2240 + 0x16 bytes	C++
 	js3250.dll!js_Invoke(JSContext * cx=0x00ee4ff8, unsigned int argc=0x00000002, int * vp=0x022ff8f8, unsigned int flags=0x00000000)  Line 1368 + 0x9 bytes	C++
 	xpc3250.dll!nsXPCWrappedJSClass::CallMethod(nsXPCWrappedJS * wrapper=0x09185bb8, unsigned short methodIndex=0x0005, const XPTMethodDescriptor * info=0x00f14af8, nsXPTCMiniVariant * nativeParams=0x0012df6c)  Line 1671 + 0x1b bytes	C++
 	xpc3250.dll!nsXPCWrappedJS::CallMethod(unsigned short methodIndex=0x0005, const XPTMethodDescriptor * info=0x00f14af8, nsXPTCMiniVariant * params=0x0012df6c)  Line 571	C++
 	xpcom_core.dll!PrepareAndDispatch(nsXPTCStubBase * self=0x09157ef8, unsigned int methodIndex=0x00000005, unsigned int * args=0x0012e02c, unsigned int * stackBytesToPop=0x0012e01c)  Line 114 + 0x21 bytes	C++
 	xpcom_core.dll!SharedStub()  Line 142	C++
 	xpcom_core.dll!nsInputStreamTee::TeeSegment(const char * buf=0x0856ff04, unsigned int count=0x000009d4)  Line 80 + 0x2b bytes	C++
 	xpcom_core.dll!nsInputStreamTee::WriteSegmentFun(nsIInputStream * in=0x08ef3f88, void * closure=0x0831b578, const char * fromSegment=0x0856ff04, unsigned int offset=0x00000000, unsigned int count=0x000009d4, unsigned int * writeCount=0x0012e0bc)  Line 110	C++


All can be reproduced using the extension I have attached. You need the 'v1.1 for 1.9.2' patch and 1.9.2 build.
It's easy to reproduce the #3, just try to load a page e.g. www.google.com
#2 seems to be more tricky and my STR requires Firebug test suite. I'll provide detailed info if necessary.


I think that the fork made by stream-listener-tee is effective tactic how to avoid having JS within the request chain listeners, however the nsIOutputStream tactic doesn't have to be so effective. Just and idea (and perhaps not acceptable), but would it be to crazy to register another nsIStreamListener into the tee-listener and observe all the incoming data using the same construct as before but through the tee-listener?

Honza
Attachment #412238 - Attachment is obsolete: true
xpconnect lets you try. any calls to any noscript methods should return an error (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED). if the caller manages to deal with this, great, if not, "caller sucks, file a bug".
Attachment #411971 - Attachment is obsolete: true
Comment on attachment 412607 [details] [diff] [review]
as landed [Re-checkin comment 98]

http://hg.mozilla.org/mozilla-central/rev/ebc28a0d4b96
Attachment #412607 - Attachment description: as landed [Comment 85 → as landed [Checkin comment 86]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
FYI, this broke Thunderbird.
http://tinderbox.mozilla.org/showbuilds.cgi?tree=Thunderbird

mailnews/news/src/nsNNTPProtocol.cpp:917: error: no matching function for call to ‘nsIStreamListenerTee::Init(nsCOMPtr<nsIStreamListener>&, nsCOMPtr<nsIOutputStream>&)’
../../../mozilla/dist/include/nsIStreamListenerTee.h:55: note: candidates are: virtual nsresult nsIStreamListenerTee::Init(nsIStreamListener*, nsIOutputStream*, nsIRequestObserver*)
(In reply to comment #88)
> FYI, this broke Thunderbird.

bug 529057 submitted on it.
Depends on: 529057
Backed out as http://hg.mozilla.org/mozilla-central/rev/d2a11453df98, so far from mozilla-central only.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attached patch v2Splinter Review
If it really is a regression, try this patch. Hope that "if (requestObserver)" condition in the init method, that is the only change, won't cause any trouble. The test has not been changed.
Attachment #412705 - Flags: review?(bzbarsky)
So the v2 patch just gets rid of the tests on mObserver in start/stop and replaces them with a test in Init() and an object allocation?

I see no possible way that can be a perf win.

Then again, I see no possible way this patch would have caused a Tp problem to start with....
On the other hand, there's a Tp4 regression from this patch on 1.9.2 as well...  I have no idea _why_.

Was there a particular test that got slower?
Are we going to back this out of 1.9.2 as well?  Because this is definitely a regression on both trees (although I can't tell why as well).
FWIW, it seems like this patch is not to blame after all.  Please see my post on dev-tree-management: <http://groups.google.com/group/mozilla.dev.tree-management/msg/5f5ab094dd57bb9e>

Also, please note that there was a similar regresssion on TraceMonkey tree during the same time, but they haven't synced with m-c, so it can't be blamed on this patch in any way.  :-)
Re-landing the patch again. Would be great to update it to the v2 patch ;)
Status: REOPENED → ASSIGNED
Comment on attachment 412607 [details] [diff] [review]
as landed [Re-checkin comment 98]

http://hg.mozilla.org/mozilla-central/rev/49e5adaab8e3
Attachment #412607 - Attachment description: as landed [Checkin comment 86] → as landed [Re-checkin comment 98]
Status: ASSIGNED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
(In reply to comment #84)
> xpconnect lets you try. any calls to any noscript methods should return an
> error (NS_ERROR_XPC_JSOBJECT_HAS_NO_FUNCTION_NAMED). if the caller manages to
> deal with this, great, if not, "caller sucks, file a bug".
I have reported a bug for this problem:
https://bugzilla.mozilla.org/show_bug.cgi?id=529547

Honza
Depends on: 529668
Flags: wanted1.9.2?
So I'm not clear on the v2 business here.  Why do we need/want it?
Comment on attachment 412705 [details] [diff] [review]
v2

There is no need, just a slightly better performance patch.
Attachment #412705 - Flags: review?(bzbarsky)
Depends on: 653533
You need to log in before you can comment on or make changes to this bug.