Open Bug 529536 Opened 15 years ago Updated 2 years ago

Get response body for redirected requests

Categories

(Core :: Networking, defect, P5)

Product:
Core
Component:
Networking
Type:
defect

Tracking

()

People

(Reporter: Honza, Unassigned)

Details

(Whiteboard: [firebug-p1][necko-would-take])

Attachments

(2 files)

Test extension
4.10 KB, application/x-xpinstall
Details
wip1
11.49 KB, patch
Details | Diff | Splinter Review 
This bug was originally reported here:
http://code.google.com/p/fbug/issues/detail?id=2434

The problem is to get response for requests that are being redirected.
Here is an example scenario:

1) Firefox loads page process1.php. It returns: "Process1->Process2"
and redirects to process2.php.

2) Firefox loads page process2.php. It returns: "Process2->Process3"
and redirects to process3.php.

3) Firefox loads page process3.php and it returns: "Process3 Finish!"

Firebug is using nsITraceableChannel to register a nsIStreamListner every
time the on-http-examine-response event is received (three times in
this case).

Event if there's a new channel for each redirect, each new channel gets the listener from the previous channel (since that's where the data needs to go in the end from the final channel) and so, the listener is getting only "Process3 Finish!" in all three stream-listeners.

Is it possible to get even "Process1->Process2" and "Process2->Process3" responses?

New APIs would have to be probably designed.

Honza
Whiteboard: [firebug-p2]
Michal, do you think you could take a look at this?

Online test case is here:
http://getfirebug.com/tests/issues/2571/c.html

1) load the page
2) click the button
3) verify responses using Firebug's Net panel.

Honza
This bug (no content on redirect) impacts YSlow, as it is unable to analyze the raw (original) page contents for certain rules such as yjsbottom and other rules that relies on the page html document (rawDoc). It wouldn't break the rule but might potentially report wrong score.

An easy way to test this is to emulate a mobile browser using a UA switcher, and then go to sites such as www.bing.com or www.cbssports.com.

Firebug 1.5.x works OK.  The bug was introduced in the 1.6 version, and is still present as of 1.7X.0.a11.

Is there a workaround possible to address this?
It looks to me like the problem is in firebug-channel-listener.js.

            // Don't register listener twice (redirects, see also bug529536).
            // xxxHonza: I don't know any way how to find out that a listener
            // has been already registered for the channel. So, use the redirection limit
            // to see that the channel has been redirected and so, listener is there.
            if (request.redirectionLimit < redirectionLimit)
            {
                if (FBTrace.DBG_CACHE)
                    FBTrace.sysout("tabCache.ChannelListener.onStartRequest; redirected request " +
                        request.redirectionLimit + " (max=" + redirectionLimit + ")");
                return;
            }

This code is not allowing it to create a listener on the redirected request, even though there is actually no listener set yet.

If I comment out the return, it therefore creates the listener, and goes on get teh content for the redirect as expected. 

It seems that onStartRequest is never called for the original redirected request, only for the subsequent redirect request.
So the redirectionLimit does not seem to be the correct way to tell if a listener has been created or not.
@Michal: this issue has been reported long time ago.
Could you please take a look at it? (perhaps it isn't so hard to fix?)

Thanks
Honza
(In reply to Jonathan Wylie from comment #3)
> It looks to me like the problem is in firebug-channel-listener.js.
Here is what Boris wrote:

"There's a new channel for each redirect, but each new channel gets the listener from the previous channel (since that's where the data needs to go in the end from the final channel). Response bodies from redirects are not generally passed on to the necko consumer unless the redirect fails, just like the consumer doesn't get OnStartRequest/OnStopRequest for redirects."

See the entire thread here:
https://groups.google.com/forum/?hl=en&fromgroups#!topic/mozilla.dev.platform/QVOklL2OGPo

Honza
Attached file Test extension — 
I am attaching a simple test extension that's using nsITraceableChannel to intercept HTTP responses.

Here is the source:
https://github.com/firebug/manual-tests/tree/master/mozilla/bug529536

STR:
1) Install the extension
2) Load http://www.janodvarko.cz/firebug/bugzilla/bug529536/issue4009.html
3) Click the button on the page to generate redirects

---

The extension registers a listener for every request to get its response. It's actually wrong since in case of redirect the response is got several times (it corresponds to Boris's explanation in comment #5)

---

Fixing this bug is important for proper HTTP monitoring (devtools).

Honza
Whiteboard: [firebug-p2] → [firebug-p1]
This won't be that simple.  We suspend the source (process1.php) channel before we get answer from the redirect veto callbacks.  And when we get green light, we just cancel the source channel.  It means we never even load the content of the response (no OnDataAvailable for it).  There could potentially be added some code to http channel that would push the content to the tracer, but we would need to push it *just* to the tracer and not to the final consumer.
fwiw I don't love that cancel().. it can result in us abandoning an otherwise reusable persistent connection in order to save reading a few bytes from the network.. and in general things with redirects are only going to have small response bodies.

it would probably be a good thing to just drain the data so the connection could be reused in this case. (and if a trace channel wants to see it that's ok as a side effect.)
(In reply to Patrick McManus [:mcmanus] from comment #8)
> fwiw I don't love that cancel().. it can result in us abandoning an
> otherwise reusable persistent connection in order to save reading a few
> bytes from the network.. and in general things with redirects are only going
> to have small response bodies.
> 
> it would probably be a good thing to just drain the data so the connection
> could be reused in this case. (and if a trace channel wants to see it that's
> ok as a side effect.)

I'll check what exactly happens and maybe change the code according you reasonable proposal.
Assignee: nobody → honzab.moz
OS: Windows Vista → All
Hardware: x86 → All
@Honza Bambas: you mentioned in Portland that this bug could be relatively straightforward to fix these days. Just pinging, so it isn't forgotten...

Honza
Flags: needinfo?(honzab.moz)
If we are ok with not canceling the channel load that has been redirected (in general what would make things simpler) then it's doable.

The nsITracableChannel may be extended to receive a notification that the channel redirects.  The callback would return a new nsIStreamListener implementation that would receive the onstart/ondata/onstop for the 30X response.

What are plans on nsITracableChannel anyway?
Flags: needinfo?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #11)
> If we are ok with not canceling the channel load that has been redirected
> (in general what would make things simpler) then it's doable.
OK for me.

> The nsITracableChannel may be extended to receive a notification that the
> channel redirects.  The callback would return a new nsIStreamListener
> implementation that would receive the onstart/ondata/onstop for the 30X
> response.
> 
> What are plans on nsITracableChannel anyway?
I don't know about any planned/suggested changes.

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #12)
> > The nsITracableChannel may be extended to receive a notification that the
> > channel redirects.  The callback would return a new nsIStreamListener
> > implementation that would receive the onstart/ondata/onstop for the 30X
> > response.
> > 
> > What are plans on nsITracableChannel anyway?
> I don't know about any planned/suggested changes.

I've heard about EOL for it.
(In reply to Honza Bambas (:mayhemer) from comment #13)
> I've heard about EOL for it.
Any links to existing threads?
What should be used instead?

Honza
(In reply to Jan Honza Odvarko [:Honza] from comment #14)
> (In reply to Honza Bambas (:mayhemer) from comment #13)
> > I've heard about EOL for it.
> Any links to existing threads?
> What should be used instead?
> 
> Honza

I thought I heard that from you.
(In reply to Honza Bambas (:mayhemer) from comment #15)
> I thought I heard that from you.
Ah, that sounds like a misunderstanding.
nsITraceableChannel is the only way how to get the response body so far.

Honza
Status: NEW → ASSIGNED
Attached patch wip1 — — Splinter Review 
For reference.  This doesn't work, and I'm starting to be afraid this won't be that easy.

There is no problem to *not* cancel the channel whom redirect is being followed (at least no net xpcshell test is failing on that).

The real problem is that we don't notify OnStart/OnData to a newly set listener.


@Honza, please see the architecture and tell me if you are OK with this approach (a new "http-on-response-redirected" main thread sync notification + use of the setNewListener method a second time.)
Flags: needinfo?(odvarko)
In the near future I won't have time to work on this.
Assignee: honzab.moz → nobody
Status: ASSIGNED → NEW
The new "http-on-response-redirected" looks good.

Too bad you don't have time to work on it.

Honza
Flags: needinfo?(odvarko)
Whiteboard: [firebug-p1] → [firebug-p1][necko-would-take]
@mayhemer: any chance to get this fixed?
I've been waiting for it since 2009 ;-)

Honza
Flags: needinfo?(honzab.moz)
Not immediately.  I'll ask if any other necko team member could work on this.
Flags: needinfo?(honzab.moz)
Honza, this has been decided as a non-priority.  I'm not going to work on this soon.
Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258
Priority: -- → P5
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: