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

RESOLVED FIXED

Status

()

Core
Networking
--
enhancement
RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: Jan Wrobel, Assigned: Honza)

Tracking

({dev-doc-complete, verified1.9.0.4})

Trunk
dev-doc-complete, verified1.9.0.4
Points:
---
Bug Flags:
wanted1.9.1 +
wanted1.9.0.x +
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [firebug-p1])

Attachments

(4 attachments, 8 obsolete attachments)

3.19 KB, application/javascript
Details
4.03 KB, application/x-xpinstall
Details
16.14 KB, patch
Jan Wrobel
: review+
Details | Diff | Splinter Review
12.53 KB, patch
Details | Diff | Splinter Review
(Reporter)

Description

9 years ago
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.
(Reporter)

Comment 1

9 years ago
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.
Attachment #316900 - Flags: review?(bzbarsky)
(Reporter)

Updated

9 years ago
Version: unspecified → Trunk
Comment on attachment 316900 [details] [diff] [review]
nsITraceableHttpChannel interface and its implementation in nsHttpChannel.

Christian is better qualified to review this than I am...
Attachment #316900 - Flags: review?(bzbarsky) → review?(cbiesinger)

Updated

9 years ago
Whiteboard: [firebug-p1]
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.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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?
Component: General → Networking
Product: Firefox → Core
QA Contact: general → networking
(Reporter)

Comment 5

9 years ago
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.



(Assignee)

Comment 6

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





(Reporter)

Comment 7

9 years ago
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.
(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.
(Assignee)

Comment 9

9 years ago
Jan: Are you working on the automated test? I have no experiences with that yet, but I can help if you would need.
(Reporter)

Comment 10

9 years ago
Yes, I'm working on the test. It should be ready tomorrow or on Saturday. 
(Assignee)

Comment 11

9 years ago
You rock!

Comment 12

9 years ago
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? 
(Assignee)

Comment 13

9 years ago
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
(Reporter)

Comment 14

9 years ago
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?


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

Comment 16

9 years ago
(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)
            {
               //...
            }
        }
    }
(Assignee)

Comment 17

9 years ago
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
Flags: wanted1.9.1+
(Reporter)

Comment 18

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

9 years ago
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!
(Reporter)

Comment 20

9 years ago
Created attachment 327287 [details] [diff] [review]
unittest for nsITraceableHttpChannel interface
(Reporter)

Comment 21

9 years ago
(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.

(Assignee)

Comment 22

9 years ago
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).
Attachment #323608 - Flags: review?(cbiesinger)
Attachment #316900 - Attachment is obsolete: true
Attachment #316900 - Flags: review?(cbiesinger)
(Assignee)

Comment 23

9 years ago
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.
Attachment #323608 - Attachment is obsolete: true
Attachment #327583 - Flags: review?(cbiesinger)
Attachment #323608 - Flags: review?(cbiesinger)

Comment 24

9 years ago
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 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.)
Attachment #327583 - Flags: superreview?(bzbarsky)
I'm taking a look at writing some simple xpcshell tests for this, fyi. I hope to have something early next week.
(Reporter)

Comment 27

9 years ago
(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. 
(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!
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 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.
Attachment #327583 - Flags: superreview?(bzbarsky) → superreview-
Oh, and we need tests here....
(Reporter)

Comment 32

9 years ago
Created attachment 330143 [details] [diff] [review]
nsITraceablChannel interface

Changes suggested by Boris.
Attachment #327287 - Attachment is obsolete: true
(Reporter)

Comment 33

9 years ago
(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().
(Reporter)

Comment 34

9 years ago
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 
(Assignee)

Comment 35

9 years ago
Sure. 
I hope it will be already fixed when you get back ;-)
Honza
(Assignee)

Updated

9 years ago
Attachment #330143 - Flags: superreview?(bzbarsky)
Attachment #330143 - Flags: review?(cbiesinger)
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.
Attachment #330143 - Flags: superreview?(bzbarsky) → superreview-
(Assignee)

Comment 37

9 years ago
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
Assignee: nobody → odvarko
Attachment #327583 - Attachment is obsolete: true
Attachment #330143 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #332312 - Flags: superreview?(bzbarsky)
Attachment #327583 - Flags: review?(cbiesinger)
Attachment #330143 - Flags: review?(cbiesinger)
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.
Attachment #332312 - Flags: superreview?(bzbarsky) → superreview+
(Assignee)

Comment 39

9 years ago
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
I believe we still need a review on this in addition to the SR (thanks Boris!). Anybody have any recommendations on a reviewer?
(Assignee)

Comment 41

9 years ago
Christian Biesinger?
(Assignee)

Updated

9 years ago
Attachment #332410 - Flags: review?(cbiesinger)

Updated

9 years ago
Flags: wanted1.9.0.x?
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.
Flags: blocking1.9.0.2?
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?
Attachment #332410 - Flags: review?(cbiesinger) → review+
We won't block on this given that there's no patch landed on trunk. It'll need to bake for a while first...
Flags: blocking1.9.0.2? → blocking1.9.0.2-
(Reporter)

Comment 45

9 years ago
Created attachment 334194 [details] [diff] [review]
nsITraceableChannel patch with changes suggested by Christian
(Reporter)

Comment 46

9 years ago
(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).

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.
Flags: wanted1.9.0.x? → wanted1.9.0.x-
(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.
(Assignee)

Updated

9 years ago
Attachment #334194 - Flags: review+
(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.
Flags: blocking1.9.0.2- → blocking1.9.0.2?
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.
Attachment #334194 - Flags: superreview+
Attachment #334194 - Flags: approval1.9.0.2?

Updated

9 years ago
Flags: blocking1.9.0.2? → blocking1.9.0.3?
Keywords: checkin-needed

Updated

9 years ago
Flags: blocking1.9.0.2?
Flags: wanted1.9.0.x- → wanted1.9.0.x?
(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.
This doesn't apply on trunk. Please unbitrot the patch and request check-in again.
Keywords: checkin-needed
Flags: wanted1.9.0.x? → wanted1.9.0.x+
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).
(Assignee)

Comment 54

9 years ago
I have been just chatting with Jan Wrobel. He is looking at it.
Flags: wanted1.9.0.x+
Why did you remove wanted1.9.0.x?
Flags: wanted1.9.0.x?
(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!
Flags: wanted1.9.0.x? → wanted1.9.0.x+
(Reporter)

Comment 57

9 years ago
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.
Attachment #334194 - Attachment is obsolete: true
Attachment #334194 - Flags: approval1.9.0.2?
(Reporter)

Updated

9 years ago
Attachment #334837 - Flags: review+
Keywords: checkin-needed
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.
I pushed changeset 310ccce6ad48, after making some changes from comment 38 that never actually got made.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
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.
Flags: blocking1.9.0.2?
(Assignee)

Updated

9 years ago
Attachment #332410 - Attachment is obsolete: true
(Assignee)

Updated

9 years ago
Attachment #332312 - Attachment is obsolete: true
(Assignee)

Comment 61

9 years ago
This is the latest and greatest:
https://bugzilla.mozilla.org/attachment.cgi?id=334837
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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?
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago9 years ago
Resolution: --- → FIXED
(Assignee)

Comment 65

9 years ago
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
Created attachment 335722 [details] [diff] [review]
nsITraceableChannel patch against the trunk

this is what got committed in boris' push. Patch applies cleanly against branch.
I ran a local branch build and ran make check and mochitests. No errors.

Passed: 50567
Failed: 0
Todo: 1154 
Comment on attachment 335722 [details] [diff] [review]
nsITraceableChannel patch against the trunk

a=beltzner when the tree opens for 1.9.0.3
Attachment #335722 - Flags: approval1.9.0.3+

Comment 69

9 years ago
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.
Indeed, very likely to be included into 3.0.3, afaics.
(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

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

Comment 74

9 years ago
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."
Yes, these nightly builds (3.alpha builds) have the fix: http://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/latest-mozilla-central/

Comment 76

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

9 years ago
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.
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

9 years ago
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.
(Assignee)

Comment 80

9 years ago
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
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
Keywords: fixed1.9.0.3
Keywords: dev-doc-needed
Flags: blocking1.9.0.4?
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

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

9 years ago
Firefox 3.0.4pre combined with Firebug 1.3.0b2 works as well by the way
Thanks, Sjon. I'll mark this as verified. I appreciate the help.
Keywords: fixed1.9.0.4 → verified1.9.0.4

Comment 86

9 years ago
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...
We won't take this in 2.0.0.x. (Note that there are only two more scheduled releases before it's EOLed.)
awesome! Thanks for the QA on this Sjon and Al.
Documented:

https://developer.mozilla.org/En/NsITraceableChannel
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.