Last Comment Bug 430155 - new nsHttpChannel interface to allow examination of HTTP data before it is passed to the channel's creator.
: new nsHttpChannel interface to allow examination of HTTP data before it is pa...
Status: RESOLVED FIXED
[firebug-p1]
: dev-doc-complete, verified1.9.0.4
Product: Core
Classification: Components
Component: Networking (show other bugs)
: Trunk
: All All
: -- enhancement with 20 votes (vote)
: ---
Assigned To: Jan Honza Odvarko [:Honza]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-04-21 15:21 PDT by Jan Wrobel
Modified: 2009-02-17 14:17 PST (History)
43 users (show)
shaver: wanted1.9.1+
samuel.sidler+old: wanted1.9.0.x+
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
nsITraceableHttpChannel interface and its implementation in nsHttpChannel. (8.15 KB, patch)
2008-04-21 15:33 PDT, Jan Wrobel
no flags Details | Diff | Review
sample implementation of tracing listener in javascript. (3.19 KB, application/javascript)
2008-05-19 16:25 PDT, Jan Wrobel
no flags Details
Cache extension (based on nsITraceableHttpChannel) for Firebug (4.03 KB, application/x-xpinstall)
2008-05-23 04:02 PDT, Jan Honza Odvarko [:Honza]
no flags Details
nsITraceableHttpChannel interface + nsStreamListenerWrapper (6.68 KB, patch)
2008-06-03 13:35 PDT, Jan Honza Odvarko [:Honza]
no flags Details | Diff | Review
unittest for nsITraceableHttpChannel interface (4.47 KB, patch)
2008-06-28 14:10 PDT, Jan Wrobel
no flags Details | Diff | Review
nsITraceableHttpChannel interface + nsStreamListenerWrapper 2 (10.20 KB, patch)
2008-07-01 04:36 PDT, Jan Honza Odvarko [:Honza]
bzbarsky: superreview-
Details | Diff | Review
nsITraceablChannel interface (17.13 KB, patch)
2008-07-17 17:39 PDT, Jan Wrobel
bzbarsky: superreview-
Details | Diff | Review
nsITraceableChannel + testcase - refactored. (17.01 KB, patch)
2008-08-04 23:49 PDT, Jan Honza Odvarko [:Honza]
bzbarsky: superreview+
Details | Diff | Review
Final nsITraceableChannel patch. (17.00 KB, patch)
2008-08-05 13:36 PDT, Jan Honza Odvarko [:Honza]
cbiesinger: review+
Details | Diff | Review
nsITraceableChannel patch with changes suggested by Christian (16.96 KB, patch)
2008-08-17 15:55 PDT, Jan Wrobel
odvarko: review+
shaver: superreview+
Details | Diff | Review
nsITraceableChannel patch against the trunk (16.14 KB, patch)
2008-08-20 23:41 PDT, Jan Wrobel
wrobel: review+
Details | Diff | Review
nsITraceableChannel patch against the trunk (12.53 KB, patch)
2008-08-27 10:08 PDT, Rob Campbell [:rc] (:robcee)
mbeltzner: approval1.9.0.4+
Details | Diff | Review

Description Jan Wrobel 2008-04-21 15:21:50 PDT
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.13) Gecko/20080325 Ubuntu/7.10 (gutsy) Firefox/2.0.0.13
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.13) Gecko/20080325 Ubuntu/7.10 (gutsy) Firefox/2.0.0.13

Currently there is no easy way to intercept HTTP traffic from Firefox extensions. 
I'm dealing with this problem for some time and I found many people asking for such feature, it would allow some cool extensions development. 

The only way that exists now to intercept HTTP traffic is to use some ugly hacks, but these are very hard to maintain, not 100% reliable, and not easily portable to new versions of Firefox.

My proposal is to define new interface nsITraceableHttpChannel and add implementation of it to nsHttpChannel.

I've already discussed this idea with Boris Zbarsky on dev-tech-network mailing list: http://groups.google.com/group/mozilla.dev.tech.network/browse_thread/thread/62af84e6d35ab77d/81f5b792fac408d4


Reproducible: Always

Steps to Reproduce:
1.
2.
3.
Comment 1 Jan Wrobel 2008-04-21 15:33:41 PDT
Created attachment 316900 [details] [diff] [review]
nsITraceableHttpChannel interface and its implementation in nsHttpChannel.

This patch is tested with the trunk Mozilla tree and modified version of Firekeeper extension.
Comment 2 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-04-21 19:08:18 PDT
Comment on attachment 316900 [details] [diff] [review]
nsITraceableHttpChannel interface and its implementation in nsHttpChannel.

Christian is better qualified to review this than I am...
Comment 3 Justin Dolske [:Dolske] 2008-05-08 15:30:19 PDT
Biesi: ping?

This has been raised as an issue potentially holding up Firebug (although there's an ugly workaround). Seems simple and low-risk, if it's an easy r+ we could then let drivers make the call.
Comment 4 Christian :Biesinger (don't email me, ping me on IRC) 2008-05-09 01:40:07 PDT
Hm... this seems like a relatively hard to implement interface. You have to remember to store the original listener & pass things on to them, and especially you have to read the data and create a new stream to pass it on. That seems slow.

Is this for the case of modifying the data, or just reading it?
Comment 5 Jan Wrobel 2008-05-09 13:32:10 PDT
In my case, I need such interface just for reading. But I need to receive data before it is passed to the original listener to be able to  cancel request when suspicious content is detected.

I agree that this patch is not perfect and I'm open for any suggestions how this can be better done.

But consider some strong sites of my approach:
-it is flexible - it allows data capturing, but also data modification in case someone needs such functionality and also allows to cancel request before data is passed to the original listener (which is crucial for me).
-it allows for multiple listeners.
-it is very simple change in Firefox code, so it is IMO simple to determine if it is done right and does not mess anything up. Which may not be as easy in case of more sophisticated code.
-I don't think performance is a big issue as there are already other places where chain of listeners is created. For example nsHttpChannel::InstallCacheListener() does exactly the same thing. It replaces channel's listener with one that writes data to cache and then passes it to the original listener. Stream converters are also nsIStreamListeners that are somewhere in the processing chain and do similar job. Firekeeper uses the same approach for more than a year (although now replacing listener is VERY painful) and there aren't any noticeable performance problems (and this extension does some sophisticated computation with each part of received response).
-I don't fully agree that this is hard to implement. You need an object implementing nsiStreamListener with just 3 methods: 
OnStartRequest(), OnStopRequest() and OnDataAvailable().
and implementation of first two can be just a one line that passes call to the original listener. If you only need to read data you even don't have to implement this, you can replace original listener with already existing nsStreamListenerTee object and pass to it object implementing nsIOutputStream (your own implementation or some existing). It doesn't seem hard and I can write a detailed doc with examples how to do it. To do something like this in current Firefox I had to write replacement for 4 browser objects and implement 27 interfaces, so at least for me it seems to be OK ;)

But as I wrote at the beginning I'm open to any suggestions and I can help to create any other solution that you think is better than one proposed.



Comment 6 Jan Honza Odvarko [:Honza] 2008-05-19 00:48:56 PDT
Fixing this bug would also solve a big trouble in Firebug (known as double-load). To solve it - FB needs to read incoming response bodies. After lengthy analysis I have find no way how to do it. There are painful workarounds (just like Jan. mentioned) in C++, but it's not available for javascript due to https://bugzilla.mozilla.org/show_bug.cgi?id=433711

It would really help to Firebug to have the nsITraceableHttpChannel interface available. The proposed solution looks fine and it would definitely solve the problem in FB. Of course supposing it's available from javascript. So, just to note that #433711 could be potentially a problem here too. Jan: have you made any testing with this already?

Biesi: could we target for Firefox 3.0.1? 

Honza





Comment 7 Jan Wrobel 2008-05-19 16:25:36 PDT
Created attachment 321672 [details]
sample implementation of tracing listener in javascript.

I've just tested nsITraceableHttpChannel interface in javascript and it works fine with XMLHttpRequests.
I'm attaching sample code that replaces channel's listener during  'http-on-examine-response' event, so you can evaluate amount of code that is needed to do the job.
Comment 8 Shawn Wilsher :sdwilsh 2008-05-20 08:13:49 PDT
(In reply to comment #1)
> This patch is tested with the trunk Mozilla tree and modified version of
> Firekeeper extension.
This would need automated tests in order to land.
Comment 9 Jan Honza Odvarko [:Honza] 2008-05-22 05:47:30 PDT
Jan: Are you working on the automated test? I have no experiences with that yet, but I can help if you would need.
Comment 10 Jan Wrobel 2008-05-22 15:29:07 PDT
Yes, I'm working on the test. It should be ready tomorrow or on Saturday. 
Comment 11 Jan Honza Odvarko [:Honza] 2008-05-22 21:59:53 PDT
You rock!
Comment 12 John J. Barton 2008-05-22 22:45:00 PDT
I think another plus to this would be a second use of the API by someone other than its author. Jan-s, is it practical to implement the Firebug use of this API and try it to gain confidence that there is a hidden show stopper? 
Comment 13 Jan Honza Odvarko [:Honza] 2008-05-23 04:02:29 PDT
Created attachment 322236 [details]
Cache extension (based on nsITraceableHttpChannel) for Firebug

Here is an extension for Firebug that implements a cache - based on nsITraceableHttpChannel interface. 

The extension replaces "top.SourceCache.prototype.load" method, which is used to get source of given URL (in Firebug). Notice that this method is source of the double-load problem. 

I am seeing two exceptions in the implementation (I have used and modified the attached JS example, so it could be perhaps my mistake):

1) Component does not have requested interface arg 0 [nsITraceableHttpChannel.listener], NS_NOINTERFACE
chrome://firecache/content/fireCache.js (95)
- Open http://www.janodvarko.cz/firebug/tests/Issue550.htm and press "GET Request button"
This exception is raised when reading the original listener from nsITraceableHttpChannel (weird, all seems to be fine).

2) Component returned failure code: 0x80540006 [nsIStreamListener.onDataAvailable]
chrome://firecache/content/fireCache.js (148)
- Open http://www.janodvarko.cz/firebug/tests/351/Issue351.htm
The exception is raised for http://www.janodvarko.cz/favicon.ico request. Notice that the file doesn't exist, but I guess the exception
shouldn't be there.

Could anybody test it as well and confirm these exceptions (especially #1 looks weird).

Of course, don't forget to apply the https://bugzilla.mozilla.org/attachment.cgi?id=316900 patch and install FB 1.2.0b1 from http://www.getfirebug.com/releases/

Honza
Comment 14 Jan Wrobel 2008-05-24 10:08:17 PDT
I can confirm #1. 
It is really strange because debug dump indicates that subject passed to 'http-on-examine-response' observer does have nsITraceableHttpChannel interface:

observe([xpconnect wrapped (nsISupports, nsIHttpChannel, nsIChannel, nsITraceableHttpChannel, ....) @ 0x85a7780 (native @ 0x87d0a80)], http-on-examine-response, null)

yet, NS_NOINTERFACE exception is raised for a Honza's test page.

Similar code in C++ works without any problems. I checked some obvious things that might be causing this, but everything looks OK. 'http-on-examine-response' is always delivered after channel's listener is set. It is only generated by nsHttpChannel, so it is not possible that some other channel that does not implement nsITraceableChannelListener is passed to the event listener. 

It seems like some strange javascript behavior. Anyone saw something similar and have any idea what may be causing this?


Comment 15 Justin Dolske [:Dolske] 2008-05-24 15:31:59 PDT
(In reply to comment #13)

> 1) Component does not have requested interface arg 0
> [nsITraceableHttpChannel.listener], NS_NOINTERFACE
> chrome://firecache/content/fireCache.js (95)

What interface is being requested, and from who? The line number here (95?) doesn't seem to match up to the right line in the code... But it sounds like when getting subject.listener, the current listener doesn't support nsIStreamListener?

> 2) Component returned failure code: 0x80540006
> [nsIStreamListener.onDataAvailable]
> chrome://firecache/content/fireCache.js (148)

That's NS_IMAGELIB_ERROR_NO_DECODER, so this is probably just a harmless exception.
Comment 16 Jan Wrobel 2008-05-25 15:43:49 PDT
(In reply to comment #15)
> (In reply to comment #13)
> 
> > 1) Component does not have requested interface arg 0
> > [nsITraceableHttpChannel.listener], NS_NOINTERFACE
> > chrome://firecache/content/fireCache.js (95)
> 
> What interface is being requested, and from who? The line number here (95?)
> doesn't seem to match up to the right line in the code... But it sounds like
> when getting subject.listener, the current listener doesn't support
> nsIStreamListener?
> 

Line market (!) bellow raises this exception (subject in observe() is a
nsHttpChannel object).
Is it possible that listener doesn't support nsIStreamListener if it is declared  as nsCOMPtr<nsIStreamListener> mListener; in nsHttpChannel.h?


observe: function(subject, topic, data) {
        if (topic == "http-on-examine-response") {
            try {
                subject.QueryInterface(Ci.nsITraceableHttpChannel);     
                var originalListener = subject.listener;   //(!)
                subject.listener = new TracingListener(originalListener);
            }
            catch (err)
            {
               //...
            }
        }
    }
Comment 17 Jan Honza Odvarko [:Honza] 2008-06-03 13:35:38 PDT
Created attachment 323608 [details] [diff] [review]
nsITraceableHttpChannel interface + nsStreamListenerWrapper

After some other analysis, I think that the exception on the line:

var originalListener = subject.listener;   //(!)

...is because of the same reason as described in:
https://bugzilla.mozilla.org/show_bug.cgi?id=433711

I have followed debugger from the moment when nsHttpChannel::GetListener method exits and I have ended up with NativeInterface2JSObject method, which returns false for the GetListener's return value (which is again the nsXMLHttpRequest).
It just isn't possible to directly expose nsXMLHttpRequest object pointer to javascript. 

So, I have created another patch (including the nsITraceableHttpChannel implementation) with a wrapper for the nsIStreamListener that is returned from the nsHttpChannel::GetListener. This seems to solve the exception.

Could anybody please test my patch and verify that?

Honza
Comment 18 Jan Wrobel 2008-06-06 10:31:48 PDT
Unfortunately, I won't have access to my development machine for more than 2 weeks. If I manage to setup all devel tools and build Firefox on my laptop, I'll test it and write an automatic test for it before I get back home.  But if I fail, than I can do it only after 21th of June.

(In reply to comment #17)
> Created an attachment (id=323608) [details]
> nsITraceableHttpChannel interface + nsStreamListenerWrapper
> 
> After some other analysis, I think that the exception on the line:
> 
> var originalListener = subject.listener;   //(!)
> 
> ...is because of the same reason as described in:
> https://bugzilla.mozilla.org/show_bug.cgi?id=433711
> 
> I have followed debugger from the moment when nsHttpChannel::GetListener method
> exits and I have ended up with NativeInterface2JSObject method, which returns
> false for the GetListener's return value (which is again the nsXMLHttpRequest).
> It just isn't possible to directly expose nsXMLHttpRequest object pointer to
> javascript. 
> 
> So, I have created another patch (including the nsITraceableHttpChannel
> implementation) with a wrapper for the nsIStreamListener that is returned from
> the nsHttpChannel::GetListener. This seems to solve the exception.
> 
> Could anybody please test my patch and verify that?
> 
> Honza
> 

Comment 19 junk man 2008-06-17 07:25:12 PDT
Disclaimer...relatively new to advanced mozilla programming.

I'd like to make mods to a web-page before it's displayed
(ala greasemonkey).

I'm writing an extension and copied the TracingListener
javascript code above and getting an error on line:
		subject.QueryInterface(Components.interfaces.nsITraceableHttpChannel); 

using FF 3.0.

help!
Comment 20 Jan Wrobel 2008-06-28 14:10:38 PDT
Created attachment 327287 [details] [diff] [review]
unittest for nsITraceableHttpChannel interface
Comment 21 Jan Wrobel 2008-06-28 14:19:25 PDT
(In reply to comment #17)
> So, I have created another patch (including the nsITraceableHttpChannel
> implementation) with a wrapper for the nsIStreamListener that is returned from
> the nsHttpChannel::GetListener. This seems to solve the exception.
> 
> Could anybody please test my patch and verify that?
> 

I tested Honza's modification and I confirm that it works well. Is is possible to replace channel's listener in Javascript during XMLHttpRequests.

Comment 22 Jan Honza Odvarko [:Honza] 2008-06-30 00:14:51 PDT
Comment on attachment 323608 [details] [diff] [review]
nsITraceableHttpChannel interface + nsStreamListenerWrapper

Attachment #316900 [details] [diff] should be marked as obsolete (I am not authorized to do that).
Comment 23 Jan Honza Odvarko [:Honza] 2008-07-01 04:36:30 PDT
Created attachment 327583 [details] [diff] [review]
 nsITraceableHttpChannel interface + nsStreamListenerWrapper 2

The nsITraceableHttpChannel.idl file was not included in the previous patch. Here is a new fixed version.
Comment 24 John J. Barton 2008-07-08 21:21:36 PDT
We are starting to plan Firebug 1.3. The nsITraceableHttpChannel could solve the most significant outstanding bug in Firebug, if it were available. Is there any plan or ETA for this function?  Thanks!
Comment 25 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-07-09 05:33:06 PDT
Comment on attachment 327583 [details] [diff] [review]
 nsITraceableHttpChannel interface + nsStreamListenerWrapper 2

Asking bz to take a look too; it's a small patch, might be eligible for combined r+sr, and I'm not sure which of biesi and bz have more short-term cycles these days.

(We'd love to get this into 1.9.0.2, because it would let Firebug address a really major problem for them.)
Comment 26 Rob Campbell [:rc] (:robcee) 2008-07-11 08:07:46 PDT
I'm taking a look at writing some simple xpcshell tests for this, fyi. I hope to have something early next week.
Comment 27 Jan Wrobel 2008-07-13 02:35:17 PDT
(In reply to comment #26)
> I'm taking a look at writing some simple xpcshell tests for this, fyi. I hope
> to have something early next week.
> 

One xpcshell test is attached to this bug report (https://bugzilla.mozilla.org/attachment.cgi?id=327287)
Maybe you will find it useful. 
Comment 28 Rob Campbell [:rc] (:robcee) 2008-07-14 08:06:18 PDT
(In reply to comment #27)
> 
> One xpcshell test is attached to this bug report
> (https://bugzilla.mozilla.org/attachment.cgi?id=327287)
> Maybe you will find it useful. 

It looks like just the thing. Thanks, Jan!
Comment 29 Rob Campbell [:rc] (:robcee) 2008-07-14 10:04:55 PDT
looks like you've done all the hard work already, Jan. I installed the unittest and it ran without a hitch, leaving the following in test_traceable_http_channel.js.log:

*** test pending
*** run_test
*** test pending
*** test finished
*** running event loop
*** observer http-on-examine-response
*** tracing listener onDataAvailable
*** data received
*** test finished
*** exiting
*** PASS ***

The only quibble I could really find was some hard tabs instead of spaces in the unittest code.

Is there anything left to do here? Is this test sufficient?
Comment 30 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-07-16 15:06:15 PDT
Comment on attachment 327583 [details] [diff] [review]
 nsITraceableHttpChannel interface + nsStreamListenerWrapper 2

>Index: netwerk/protocol/http/public/nsITraceableHttpChannel.idl

There doesn't seem to be anything HTTP-specific about this interface other than the inheritance (which I think should be from nsISupports).  Why not just nsITraceableChannel?  Then document when the listener can be set?

>+    attribute nsIStreamListener listener;

The only valid use case is taking a reference to the old listener, then setting a new one, right?  In that case, perhaps:

  nsIStreamListener setNewListener(nsIStreamListener aListener);

is a better signature?

>Index: netwerk/protocol/http/src/nsHttpChannel.cpp
>+//-----------------------------------------------------------------------------
>+// nsStreamListenerWrapper <private>
>+//-----------------------------------------------------------------------------

I understand the need for this, but abhor it.  Please file a bug to remove it, and mark that bug dependent on bug 433711.

nsStreamListenerWrapper(nsIStreamListener *listener) { 
>+        mListener = listener;

Please use an initializer.


>+nsStreamListenerWrapper::OnStartRequest(nsIRequest *request,
>+    NS_ENSURE_TRUE(mListener, NS_ERROR_NOT_INITIALIZED);

>+nsStreamListenerWrapper::OnStopRequest(nsIRequest *request,

If the former got called and threw, the latter will _also_ be called.

So either remove the NS_ENSURE_TRUE (and add an assert in the constructor and use the unsafe forwarding macro) or add null-checks in all three methods (and hence just use the safe forwarding macro).

I prefer the former, myself.

>+nsHttpChannel::GetListener(nsIStreamListener **aListener)

>+    *aListener = wrapper;
>+    NS_IF_ADDREF(*aListener);
>+    return NS_OK;

You want:

  wrapper.forget(aListener);

And do you really want to return NS_OK on allocation failure here?  I guess with the forget() approach at least you're not doing an explicit null-check and soon we'll have failure-free allocation...

>+nsHttpChannel::SetListener(nsIStreamListener *aListener)
>+{
>+    mListener = aListener;

I'm not happy with allowing this at arbitrary times, and most particularly not after we have already done an OnDataAvailable call.  I would, in fact, argue that the only valid times to do this are up until we've called OnStartRequest.  Once we have done that, no more changing the listener.  If the plan is to use this from on-examine-response, then perhaps it should even be restricted to being changed before we enter OnStartRequest.
Comment 31 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-07-16 15:06:35 PDT
Oh, and we need tests here....
Comment 32 Jan Wrobel 2008-07-17 17:39:49 PDT
Created attachment 330143 [details] [diff] [review]
nsITraceablChannel interface

Changes suggested by Boris.
Comment 33 Jan Wrobel 2008-07-17 18:11:23 PDT
(In reply to comment #30)
> (From update of attachment 327583 [details] [diff] [review])
> >Index: netwerk/protocol/http/public/nsITraceableHttpChannel.idl
> 
> There doesn't seem to be anything HTTP-specific about this interface other than
> the inheritance (which I think should be from nsISupports).  Why not just
> nsITraceableChannel?  Then document when the listener can be set?

Interface renamed and moved to netwerk/base/public/

> >+    attribute nsIStreamListener listener;
> 
> The only valid use case is taking a reference to the old listener, then setting
> a new one, right?  In that case, perhaps:
> 
>   nsIStreamListener setNewListener(nsIStreamListener aListener);
> 
> is a better signature?

It won't work. New listener has to be initialized with the old one to be able to pass calls to it. To do it, interface has to allow to get listener before setting a new one.

> 
> >Index: netwerk/protocol/http/src/nsHttpChannel.cpp
> >+//-----------------------------------------------------------------------------
> >+// nsStreamListenerWrapper <private>
> >+//-----------------------------------------------------------------------------
> 
> I understand the need for this, but abhor it.  Please file a bug to remove it,
> and mark that bug dependent on bug 433711.

bug 445902 added.

> nsStreamListenerWrapper(nsIStreamListener *listener) { 
> >+        mListener = listener;
> 
> Please use an initializer.

Done.

> >+nsStreamListenerWrapper::OnStartRequest(nsIRequest *request,
> >+    NS_ENSURE_TRUE(mListener, NS_ERROR_NOT_INITIALIZED);
> 
> >+nsStreamListenerWrapper::OnStopRequest(nsIRequest *request,
> 
> If the former got called and threw, the latter will _also_ be called.
> 
> So either remove the NS_ENSURE_TRUE (and add an assert in the constructor and
> use the unsafe forwarding macro) or add null-checks in all three methods (and
> hence just use the safe forwarding macro).
> 
> I prefer the former, myself.

2 missing null-checks added.

> >+nsHttpChannel::GetListener(nsIStreamListener **aListener)
> 
> >+    *aListener = wrapper;
> >+    NS_IF_ADDREF(*aListener);
> >+    return NS_OK;
> 
> You want:
> 
>   wrapper.forget(aListener);

Changed.

> And do you really want to return NS_OK on allocation failure here?  I guess
> with the forget() approach at least you're not doing an explicit null-check and
> soon we'll have failure-free allocation...

Fixed.

> 
> >+nsHttpChannel::SetListener(nsIStreamListener *aListener)
> >+{
> >+    mListener = aListener;
> 
> I'm not happy with allowing this at arbitrary times, and most particularly not
> after we have already done an OnDataAvailable call.  I would, in fact, argue
> that the only valid times to do this are up until we've called OnStartRequest. 
> Once we have done that, no more changing the listener.  If the plan is to use
> this from on-examine-response, then perhaps it should even be restricted to
> being changed before we enter OnStartRequest.
> 

Done. Now it is only possible to replace listener before OnStartRequest. 
Added comment to the interface that it is recommended to allow listener replacement only before OnStartRequest.

Tests added to the main patch (previously it was separate file and it was reviewed by Rob Campbell). Added test to assert that listener can't be replaced after OnStartRequest().
Comment 34 Jan Wrobel 2008-07-17 18:18:15 PDT
Honza,

If there are some additional comments or suggestions to this patch can you answer them? Tomorrow morning I'm going on 3 weeks vacation without Internet access, so I can continue to work on this patch only after I'm back.

Cheers,
Jan 
Comment 35 Jan Honza Odvarko [:Honza] 2008-07-18 00:13:54 PDT
Sure. 
I hope it will be already fixed when you get back ;-)
Honza
Comment 36 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-07-24 10:52:52 PDT
Comment on attachment 330143 [details] [diff] [review]
nsITraceablChannel interface

>Index: netwerk/base/public/nsITraceableChannel.idl
>+ * Traceable channel interface. Channel implementing this interface
>+ * gives possibility to intercept its traffic by inserting intermediate
>+ * stream listeners.

"A channel implementing this interface allows one to intercept its data by inserting intermediate stream listeners."

>+     * Channel's listener. Observer is able to get original channel's listener

There are various minor grammar nits here, but let's hold off on those until we sort out the signature.

> It won't work. New listener has to be initialized with the old one to be able
> to pass calls to it. To do it, interface has to allow to get listener before
> setting a new one.

I don't see why the initialization can't follow the setting.  Is that actually a serious problem for some planned consumer?

Ideally the way this interface would work is that you're pass in an nsIFoo of some sort that happens to implement nsIStreamListener and the channel itself would handle the initialization or something.  But that requires adding one more interface, so isn't ideal.  I'd much prefer the signature I suggested here.

>Index: netwerk/protocol/http/src/nsHttpChannel.cpp

> 2 missing null-checks added.

I was pretty serious about the FORWARD_SAFE macros.  Please use them.

>+nsStreamListenerWrapper::OnStartRequest(nsIRequest *request,
>+                                    nsISupports *context)

Indentation issues here, but this code should be going away anyway.

>Index: netwerk/test/unit/test_traceable_channel.js
>+  this.onDataAvailable = function(request, context, inputStream,
>+                                  offset, count) {
>+    var data = binaryInputStream.readBytes(count);
>+    do_check_eq(originalBody, data);

There is no guarantee that the entire response body of the original HTTP request will be available at this point.  So this test could randomly fail if it's not all there.  Please fix that.

>+    var memoryService = Cc["@mozilla.org/xpcom/memory-service;1"].
>+      createInstance(Components.interfaces.nsIMemory);

Why bother?  Just pass null.

>+  this.QueryInterface = function(iid) {

What about nsIRequestObserver?  And why bother QIing to nsIWeakReference here

>+    dump("*** observer " + this.topic + "\n");

Please take that out.  If you want to assert that the topic is correct, do that instead.

>+  dump("*** data received\n");

Again, take this out.

>+  dump("*** run_test\n");

And this.
Comment 37 Jan Honza Odvarko [:Honza] 2008-08-04 23:49:56 PDT
Created attachment 332312 [details] [diff] [review]
nsITraceableChannel + testcase - refactored.

> "A channel implementing this interface allows one to intercept its data by
> inserting intermediate stream listeners."
OK, comment changed.

> I don't see why the initialization can't follow the setting.  Is that actually
> a serious problem for some planned consumer?
I don't see problems either (the test-case adapted and works) - setNewListener used.

> I was pretty serious about the FORWARD_SAFE macros. Please use them.
Done

> Indentation issues here, but this code should be going away anyway.
Code removed.

> There is no guarantee that the entire response body of the original HTTP
> request will be available at this point.  So this test could randomly fail if
> it's not all there.  Please fix that.
Done

> Why bother?  Just pass null.
Memory service removed.

> What about nsIRequestObserver?  And why bother QIing to nsIWeakReference here
Done, nsIRequestObserver used and nsIWeakReference removed.

> Please take that out.  If you want to assert that the topic is correct, 
> do that instead.
Done, unnecessary dumps removed.

Honza
Comment 38 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-08-05 13:05:54 PDT
Comment on attachment 332312 [details] [diff] [review]
nsITraceableChannel + testcase - refactored.

>+++ netwerk/base/public/nsITraceableChannel.idl	4 Aug 2008 22:59:14 -0000
>+     * Channel's listener. Observer is able to get original channel's listener
>+     * and replace it with its own version.

"Replace the channel's listener with a new one, and return the listener the channel used to have"

> New listener intercepts
>+     * OnStartRequest, OnDataAvailable and OnStopRequest calls and can pass
>+     * them to the original listener after examination.

"The new listener ...."  Also, this should be a "must", not a "can", at least for OnStartRequest and OnStopRequest, to avoid a contract violation.

>+     * If multiple observers replace channel's listener the chain of listeners

"If multiple callers replace the channel's listener, a chain of listeners"

>+     * is created. Last registered listener is called first, it passes calls

"The last listener passed to setNewListener ..."

>+     * to original channel's listener. Observer has no way to control at which

" to the original listener.  The caller of setNewListener .... "

>+     * Note: Observer should not delay passing OnStartRequest to the

"Note: the listener passed to setNewListener must not delay ..."

>+     * Note2: Channel may set restriction when listener can be replaced.

"Note 2: A channel ...."

>+     * It is not recommended to allow listener replacement after OnStartRequest
>+     * was called.

s/was/has been/

With those comment nits, looks great.
Comment 39 Jan Honza Odvarko [:Honza] 2008-08-05 13:36:42 PDT
Created attachment 332410 [details] [diff] [review]
Final nsITraceableChannel patch.

Cool! 

Comments changed.

Could anybody please commit the patch? I don't have the right permissions to do that :-(

Honza
Comment 40 Rob Campbell [:rc] (:robcee) 2008-08-05 13:47:17 PDT
I believe we still need a review on this in addition to the SR (thanks Boris!). Anybody have any recommendations on a reviewer?
Comment 41 Jan Honza Odvarko [:Honza] 2008-08-05 13:53:00 PDT
Christian Biesinger?
Comment 42 Rob Campbell [:rc] (:robcee) 2008-08-11 07:15:48 PDT
repinged biesi via email for the review request. Not sure if we can squeak this in under the 3.0.2 timeframe, but we'd like to make it for that.
Comment 43 Christian :Biesinger (don't email me, ping me on IRC) 2008-08-12 00:56:53 PDT
Comment on attachment 332410 [details] [diff] [review]
Final nsITraceableChannel patch.

+++ netwerk/base/public/nsITraceableChannel.idl	5 Aug 2008 20:31:15 -0000
+     * The last listener passed to setNewListener is called first, it passes calls 
+     * to the original listener. The caller of setNewListener has no way to control 
+     * at which place in the chain its listener is placed.

those two sentences seem to contradict each other. just remove the first one?

+     * Note: The caller of setNewListener must not delay passing OnStartRequest to 
+     * the original listener to avoid breaking features such as "save as".

I don't think this should mention features that are entirely unrelated to necko. I.e. remove the "to avoid ..." part.

+++ netwerk/protocol/http/src/nsHttpChannel.cpp	5 Aug 2008 20:31:26 -0000
+class nsStreamListenerWrapper : public nsIStreamListener

this REALLY needs a comment why it exists because that's very nonobvious.

+    virtual ~nsStreamListenerWrapper() {};

remove the semicolon, and make it nonvirtual and private

+++ netwerk/test/unit/test_traceable_channel.js	5 Aug 2008 20:31:28 -0000
+  this.onStartRequest = function(request , context) {

no space before the comma

+    try{

add a space before the {

+    do_throw("replaced channel's listener after onStartRequest.");

s/after/during/

+    if ( iid.equals(Components.interfaces.nsIStreamListener) ||
+         iid.equals(Components.interfaces.nsIRequestObserver) ||
+         iid.equals(Components.interfaces.nsISupports)
+         )

remove the space after the (

also, those functions really should be set on the .prototype instead, for this one and the RequestObserver

request observer is not really a good name IMO, it makes it sound like it is an implementation of nsIRequestObserver

+  httpserver.registerPathHandler("/testdir", test_handler);

why not use one of the default handlers?
Comment 44 Samuel Sidler (old account; do not CC) 2008-08-14 16:15:22 PDT
We won't block on this given that there's no patch landed on trunk. It'll need to bake for a while first...
Comment 45 Jan Wrobel 2008-08-17 15:55:37 PDT
Created attachment 334194 [details] [diff] [review]
nsITraceableChannel patch with changes suggested by Christian
Comment 46 Jan Wrobel 2008-08-17 16:04:53 PDT
(In reply to comment #43)
> (From update of attachment 332410 [details] [diff] [review])
> +++ netwerk/base/public/nsITraceableChannel.idl 5 Aug 2008 20:31:15 -0000
> +     * The last listener passed to setNewListener is called first, it passes
> calls 
> +     * to the original listener. The caller of setNewListener has no way to
> control 
> +     * at which place in the chain its listener is placed.
> 
> those two sentences seem to contradict each other. just remove the first one?

Done.

> +     * Note: The caller of setNewListener must not delay passing
> OnStartRequest to 
> +     * the original listener to avoid breaking features such as "save as".
> 
> I don't think this should mention features that are entirely unrelated to
> necko. I.e. remove the "to avoid ..." part.

Done.

> +++ netwerk/protocol/http/src/nsHttpChannel.cpp 5 Aug 2008 20:31:26 -0000
> +class nsStreamListenerWrapper : public nsIStreamListener
> 
> this REALLY needs a comment why it exists because that's very nonobvious.

Added following comment: 
// Wrapper class to make replacement of nsHttpChannel's listener
// from JavaScript possible. It is workaround for bug 433711.

> +    virtual ~nsStreamListenerWrapper() {};
> 
> remove the semicolon, and make it nonvirtual and private

Done.

> +++ netwerk/test/unit/test_traceable_channel.js 5 Aug 2008 20:31:28 -0000
> +  this.onStartRequest = function(request , context) {
> 
> no space before the comma

Done.

> +    try{
> 
> add a space before the {

Done.

> 
> +    do_throw("replaced channel's listener after onStartRequest.");
> 
> s/after/during/

Done.

> +    if ( iid.equals(Components.interfaces.nsIStreamListener) ||
> +         iid.equals(Components.interfaces.nsIRequestObserver) ||
> +         iid.equals(Components.interfaces.nsISupports)
> +         )
> 
> remove the space after the (

Done.

> also, those functions really should be set on the .prototype instead, for this
> one and the RequestObserver

Done.

> request observer is not really a good name IMO, it makes it sound like it is an
> implementation of nsIRequestObserver

Changed to HttpResponseExaminer

> +  httpserver.registerPathHandler("/testdir", test_handler);
> 
> why not use one of the default handlers?

Can you point me to some example code? I can't find any test that uses default handlers (maybe I'm missing something).

Comment 47 Samuel Sidler (old account; do not CC) 2008-08-17 18:52:17 PDT
Do we normally add new interfaces on stable branches? I think this might have to wait for 1.9.1 and not make it into 1.9.0.x. Please renominate if you disagree.
Comment 48 Christian :Biesinger (don't email me, ping me on IRC) 2008-08-18 00:15:21 PDT
(In reply to comment #46)
> > why not use one of the default handlers?
> 
> Can you point me to some example code? I can't find any test that uses default
> handlers (maybe I'm missing something).

Just http://localhost:4444/ should work

But on second thought, since you want the comparison with originalBody, it's probably better to use your own handler.
Comment 49 Rob Campbell [:rc] (:robcee) 2008-08-19 06:25:11 PDT
(In reply to comment #47)
> Do we normally add new interfaces on stable branches? I think this might have
> to wait for 1.9.1 and not make it into 1.9.0.x. Please renominate if you
> disagree.

sam, this isn't an interface change, it's an interface addition. And a harmless one at that which will provide firebug and perhaps other debugging extensions a means to safely cache network data. It's Important.
Comment 50 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-08-19 06:32:39 PDT
Comment on attachment 334194 [details] [diff] [review]
nsITraceableChannel patch with changes suggested by Christian

Advancing bz's sr, requesting approval for compatible interface addition to help Firebug and other network-inespecting extensions avoid dangerous double loads.
Comment 51 Samuel Sidler (old account; do not CC) 2008-08-19 16:52:22 PDT
(In reply to comment #49)
> sam, this isn't an interface change, it's an interface addition. And a harmless
> one at that which will provide firebug and perhaps other debugging extensions a
> means to safely cache network data. It's Important.

Ah, sorry. Misunderstood. Let's get this landed on mozilla-central and let it bake a couple days before taking on 1.9.0.x.
Comment 52 Reed Loden [:reed] (use needinfo?) 2008-08-19 21:22:14 PDT
This doesn't apply on trunk. Please unbitrot the patch and request check-in again.
Comment 53 Rob Campbell [:rc] (:robcee) 2008-08-20 07:54:23 PDT
Honza: Can you take a look at what's needed to fix this up to land in mozilla-central? The differences should be minimal (I hope).
Comment 54 Jan Honza Odvarko [:Honza] 2008-08-20 07:58:03 PDT
I have been just chatting with Jan Wrobel. He is looking at it.
Comment 55 Reed Loden [:reed] (use needinfo?) 2008-08-20 08:01:49 PDT
Why did you remove wanted1.9.0.x?
Comment 56 Rob Campbell [:rc] (:robcee) 2008-08-20 08:04:44 PDT
(In reply to comment #55)
> Why did you remove wanted1.9.0.x?

Yeah, after we worked so hard to get it! :)

I suspect that was an accidental deflagging.

jan o & jan w: Thanks!
Comment 57 Jan Wrobel 2008-08-20 23:41:49 PDT
Created attachment 334837 [details] [diff] [review]
nsITraceableChannel patch against the trunk

This patch was generated against Mercurial mozilla-central trunk. No changes in a code, just different line numbers.
Comment 58 Rob Campbell [:rc] (:robcee) 2008-08-21 09:23:44 PDT
I was waiting to pounce and check this in this morning, but the tree's still closed (and likely will be for a good chunk of the day). I can say that this patch applies cleanly to mozilla-central and is ready to go.
Comment 59 Boris Zbarsky [:bz] (Out June 25-July 6) 2008-08-25 10:27:29 PDT
I pushed changeset 310ccce6ad48, after making some changes from comment 38 that never actually got made.
Comment 60 Samuel Sidler (old account; do not CC) 2008-08-25 11:53:07 PDT
Not really blocking this release, but we'll take a nice roll-up patch that include the changes made on checkin. Which patch actually went in? There are several that aren't obsoleted.
Comment 61 Jan Honza Odvarko [:Honza] 2008-08-25 11:59:12 PDT
This is the latest and greatest:
https://bugzilla.mozilla.org/attachment.cgi?id=334837
Comment 62 Rob Campbell [:rc] (:robcee) 2008-08-27 07:33:33 PDT
reopening as this hasn't yet been landed on branch.

code's frozen for 3.0.2 now, we're going to want this for 3.0.3 so as soon as the tree re-opens we'd like this checked in there.
Comment 63 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-08-27 07:37:11 PDT
Normally, when it's checked in on trunk, the bug should be marked fixed, no matter if the patch still needs to be checked in on branch.
Comment 64 Mike Shaver (:shaver -- probably not reading bugmail closely) 2008-08-27 07:41:02 PDT
Yes, this is FIXED on trunk; we'll add the fixed1.9.0.2 keyword when it lands on branch.  (Or fixed1.9.0.3 now that we've missed 1.9.0.2. :( )

We need a branch patch produced based on bz's listed changeset, since per his comment there are changes there that weren't in the patches attached (unaddressed requests from comment 38).  Rob, can you get that put together so that we can get this into CVS ASAP, and stop worrying about it?
Comment 65 Jan Honza Odvarko [:Honza] 2008-08-27 08:21:39 PDT
I don't see what is actually missing in the final patch (https://bugzilla.mozilla.org/attachment.cgi?id=334837)

All the proposed changes from comment #38 have been made (all related to a comment in the nsITraceableChannel.idl). Notice that some other changes have been proposed in comment #43 (also done), which had impact on the comments in the .idl file as well. Is this the problem?

Honza
Comment 66 Rob Campbell [:rc] (:robcee) 2008-08-27 10:08:15 PDT
Created attachment 335722 [details] [diff] [review]
nsITraceableChannel patch against the trunk

this is what got committed in boris' push. Patch applies cleanly against branch.
Comment 67 Rob Campbell [:rc] (:robcee) 2008-08-27 12:43:30 PDT
I ran a local branch build and ran make check and mochitests. No errors.

Passed: 50567
Failed: 0
Todo: 1154 
Comment 68 Mike Beltzner [:beltzner, not reading bugmail] 2008-08-27 12:54:28 PDT
Comment on attachment 335722 [details] [diff] [review]
nsITraceableChannel patch against the trunk

a=beltzner when the tree opens for 1.9.0.3
Comment 69 John J. Barton 2008-08-27 22:42:42 PDT
I would appreciate it if someone can tell me which version of Firefox this fix will be available in.  For sure in 3.1, not in 3.0.2, and maybe in 3.0.3 is what I understand from reading here.
Comment 70 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-08-28 03:29:11 PDT
Indeed, very likely to be included into 3.0.3, afaics.
Comment 71 Rob Campbell [:rc] (:robcee) 2008-08-28 09:33:18 PDT
(In reply to comment #69)
> I would appreciate it if someone can tell me which version of Firefox this fix
> will be available in.  For sure in 3.1, not in 3.0.2, and maybe in 3.0.3 is
> what I understand from reading here.

very definitely in 3.0.3.
Comment 72 Mike Flynn 2008-09-02 10:46:36 PDT
Is this not fixed for FF 2?
Comment 73 Shawn Wilsher :sdwilsh 2008-09-02 11:50:07 PDT
(In reply to comment #72)
> Is this not fixed for FF 2?
This won't be fixed for Firefox 2.
Comment 74 Ralf Vitasek 2008-09-05 08:09:25 PDT
are there any nightly builds available that already contain this fix?
i've installed 3.0.3pre nightly from Sept. 5th. today

but Firebug still complains:
"This problem will disappear when https://bugzilla.mozilla.org/show_bug.cgi?id=430155 is shipped."
Comment 75 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-09-05 08:20:54 PDT
Yes, these nightly builds (3.alpha builds) have the fix: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/
Comment 76 Ralf Vitasek 2008-09-05 09:00:09 PDT
ok, then it seems the current Firebug beta isn't really ready for this fix as the same problems shows in that 3.1 nightly.

so annoying that this killer feature for web developers is in such a state atm.
guess i have to wait quite a while longer *sigh*
Comment 77 John J. Barton 2008-09-05 09:24:37 PDT
I'm confused. This is my understanding with respect to this bug:
3.0.1: shipped, not fixed
3.0.2: still pre, not fixed.
3.0.3: build does not exist; if it did, fixed.
3.1: alpha (2?), fixed.
I've played with 3.1a1 and its too buggy for Firebug use. If a nightly build of 3.0.3 is available (and if the fix is in there), Firebug team will create at least an alpha release using the new feature.

And of course the current Firebug 1.2.0 does not use this feature which hasn't shipped.
Comment 78 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-09-05 09:28:26 PDT
There is no fixed1.9.0.3 in the keywords, so it is not in there.
That is not even possible currently, since Firefox3.0.2 is not even out yet.
You're just going to have to be patient. What you could do is make a Firefox 3 tryserver build with the patch included, perhaps.
Comment 79 Ralf Vitasek 2008-09-05 09:32:17 PDT
yeah sure, i just wanted to to give it a try because the message sounded like it was already prepared for the new interface.

i'm not blaming FF nor Firebug. it's just an annoying everyday issue i was hoping to get around asap.
Comment 80 Jan Honza Odvarko [:Honza] 2008-09-07 06:59:26 PDT
I have downloaded (FF 3.1b1pre) from http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-trunk/ and tested with Firebug 1.3a1 (r1008).

The new FB source cache (based on nsITraceableChannel) implementation is automatically used in this case and worked for me. 

> ok, then it seems the current Firebug beta isn't really ready for this fix as
> the same problems shows in that 3.1 nightly.
Ralf, could you please test it with the same configuration and let me know how whether it's better?

Honza
Comment 81 Rob Campbell [:rc] (:robcee) 2008-09-08 13:01:03 PDT
in two parts:

Checking in base/public/Makefile.in;
/cvsroot/mozilla/netwerk/base/public/Makefile.in,v  <--  Makefile.in
new revision: 1.128; previous revision: 1.127
done
Checking in protocol/http/src/nsHttpChannel.cpp;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,v  <--  nsHttpChannel.cpp
new revision: 1.334; previous revision: 1.333
done
Checking in protocol/http/src/nsHttpChannel.h;
/cvsroot/mozilla/netwerk/protocol/http/src/nsHttpChannel.h,v  <--  nsHttpChannel.h
new revision: 1.97; previous revision: 1.96
done


RCS file: /cvsroot/mozilla/netwerk/base/public/nsITraceableChannel.idl,v
done
Checking in base/public/nsITraceableChannel.idl;
/cvsroot/mozilla/netwerk/base/public/nsITraceableChannel.idl,v  <--  nsITraceableChannel.idl
initial revision: 1.1
done
RCS file: /cvsroot/mozilla/netwerk/test/unit/test_traceable_channel.js,v
done
Checking in test/unit/test_traceable_channel.js;
/cvsroot/mozilla/netwerk/test/unit/test_traceable_channel.js,v  <--  test_traceable_channel.js
initial revision: 1.1
done
Comment 82 [On PTO until 6/29] 2008-10-23 15:49:57 PDT
I need someone who understands this issue or how to expose this in Firebug to test that it is fixed for Firefox 3.0.4, which we will begin building next week. The nightly build for 1.9 can be used from ftp://ftp.mozilla.org/pub/firefox/nightly/latest-mozilla1.9.0/.

Can one of you who looked at this before take a look with the one of those nightly builds? Thanks.
Comment 83 Sjon Hortensius 2008-11-05 02:40:26 PST
Tested using firefox-3.0.5pre.en-US.win32.installer.exe and Firebug 1.3.0b2

The bug seems to be fixed and Firebug properly exposes the response from the POST request
Comment 84 Sjon Hortensius 2008-11-05 02:45:36 PST
Firefox 3.0.4pre combined with Firebug 1.3.0b2 works as well by the way
Comment 85 [On PTO until 6/29] 2008-11-05 11:37:00 PST
Thanks, Sjon. I'll mark this as verified. I appreciate the help.
Comment 86 Rasmus Schultz 2008-11-11 08:02:17 PST
Will this fix be included in Firefox 2? ... there are still developers who need to target both Firefox 2 and 3, and the Firebug double-load issue remains with the latest version of Firefox 2 and Firebug - it makes to very difficult to some AJAX actions that can only be loaded once...
Comment 87 Samuel Sidler (old account; do not CC) 2008-11-11 09:39:55 PST
We won't take this in 2.0.0.x. (Note that there are only two more scheduled releases before it's EOLed.)
Comment 88 Rob Campbell [:rc] (:robcee) 2008-11-18 09:47:00 PST
awesome! Thanks for the QA on this Sjon and Al.
Comment 89 Eric Shepherd [:sheppy] 2009-02-17 14:17:34 PST
Documented:

https://developer.mozilla.org/En/NsITraceableChannel

Note You need to log in before you can comment on or make changes to this bug.