430155 - new nsHttpChannel interface to allow examination of HTTP data before it is passed to the channel's creator.

Status: RESOLVED FIXED

(Core :: Networking, enhancement)

Description

[Jan Wrobel]

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]

Attachment: 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 [:bzbarsky]]

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

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

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]

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] (always need-info? me)]

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]

Attachment: 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]

(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] (always need-info? me)]

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]

Yes, I'm working on the test. It should be ready tomorrow or on Saturday. 

Comment 11

[Jan Honza Odvarko [:Honza] (always need-info? me)]

You rock!

Comment 12

[John J. Barton]

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] (always need-info? me)]

Attachment: 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]

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

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

(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] (always need-info? me)]

Attachment: 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]

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]

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]

Attachment: unittest for nsITraceableHttpChannel interface

Comment 21

[Jan Wrobel]

(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] (always need-info? me)]

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] (always need-info? me)]

Attachment: 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]

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 emeritus)]

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

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]

(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)]

(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)]

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 [:bzbarsky]]

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 [:bzbarsky]]

Oh, and we need tests here....

Comment 32

[Jan Wrobel]

Attachment: nsITraceablChannel interface

Changes suggested by Boris.

Comment 33

[Jan Wrobel]

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

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] (always need-info? me)]

Sure. 
I hope it will be already fixed when you get back ;-)
Honza

Comment 36

[Boris Zbarsky [:bzbarsky]]

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] (always need-info? me)]

Attachment: 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 [:bzbarsky]]

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] (always need-info? me)]

Attachment: 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)]

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] (always need-info? me)]

Christian Biesinger?

Comment 42

[Rob Campbell [:rc] (:robcee)]

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

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

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]

Attachment: nsITraceableChannel patch with changes suggested by Christian

Comment 46

[Jan Wrobel]

(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)]

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

(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)]

(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 emeritus)]

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

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

This doesn't apply on trunk. Please unbitrot the patch and request check-in again.

Comment 53

[Rob Campbell [:rc] (:robcee)]

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] (always need-info? me)]

I have been just chatting with Jan Wrobel. He is looking at it.

Comment 55

[Reed Loden [:reed]]

Why did you remove wanted1.9.0.x?

Comment 56

[Rob Campbell [:rc] (:robcee)]

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

Attachment: 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)]

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 [:bzbarsky]]

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

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] (always need-info? me)]

This is the latest and greatest:
https://bugzilla.mozilla.org/attachment.cgi?id=334837

Comment 62

[Rob Campbell [:rc] (:robcee)]

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 (dead)]

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 emeritus)]

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] (always need-info? me)]

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

Attachment: nsITraceableChannel patch against the trunk

this is what got committed in boris' push. Patch applies cleanly against branch.

Comment 67

[Rob Campbell [:rc] (:robcee)]

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

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]

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 (dead)]

Indeed, very likely to be included into 3.0.3, afaics.

Comment 71

[Rob Campbell [:rc] (:robcee)]

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

Is this not fixed for FF 2?

Comment 73

[Shawn Wilsher :sdwilsh]

(In reply to comment #72)
> Is this not fixed for FF 2?
This won't be fixed for Firefox 2.

Comment 74

[Ralf Vitasek]

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 (dead)]

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]

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]

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 (dead)]

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]

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] (always need-info? me)]

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

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

[Al Billings [:abillings - ex-MoCo]]

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]

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]

Firefox 3.0.4pre combined with Firebug 1.3.0b2 works as well by the way

Comment 85

[Al Billings [:abillings - ex-MoCo]]

Thanks, Sjon. I'll mark this as verified. I appreciate the help.

Comment 86

[Rasmus Schultz]

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

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

awesome! Thanks for the QA on this Sjon and Al.

Comment 89

[Eric Shepherd [:sheppy]]

Documented:

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