Closed Bug 630733 Opened 9 years ago Closed 9 years ago

web console does not report data which the server did send (301, 302 and 303)

Categories

(DevTools :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: kdevel, Assigned: msucan)

Details

(Whiteboard: [patchclean:0205])

Attachments

(1 file, 1 obsolete file)

User-Agent:       
Build Identifier: 4.0bpre11

The web console does not report the server response's headers in case of 301, 302 and 304 server responses but that of the subsequent request instead.

Reproducible: Always

Steps to Reproduce:
1. open web console
2. click at 301, 302 and/or 303 link on http://www.vogtner.de/header/
3. inspect server response
Actual Results:  
Web console displays response headers of the subsequent request's servers response, (a 404 response).

Expected Results:  
Display headers of response to the initial request (i.e. Location header).

maybe related to Bug 599725
Assignee: nobody → mihai.sucan
Priority: -- → P1
Priority: P1 → --
Version: unspecified → Trunk
Confirmed. This seems to be really a bug, good find! Thanks for the report and for the nice test location!

This bug is definitely related to bug 599725 and I may have a hunch for what is happening. I hope to have a patch soon.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Attached patch proposed fix (obsolete) — Splinter Review
Proposed fix and mochitest.

The problem is caused by the code that tries to find the response headers from the httpResponseExaminer. The ResponseListener onStopRequest() method used the aRequest argument to find the associated list of headers that come from httpResponseExaminer. The httpActivity starts with the request channel of the 301 Moved Permanently request, and it ends in onStopRequest() with the channel of the 404 Not Found request, due to the redirect. The solution is to use the already stored httpActivity.channel, which still points to the 301 request channel - this way we can properly map from the ResponseListener.onStopRequest() method to the headers stored by the httpResponseExaminer.
Attachment #509803 - Flags: feedback?(rcampbell)
Status: NEW → ASSIGNED
Whiteboard: [patchclean:0204]
Looks good. A small issue remains: If you enable "Log Request and Response Body" the 30x request lists the 404's response body as its own.
Comment on attachment 509803 [details] [diff] [review]
proposed fix

-      if (item.channel === aRequest) {
+      if (item.channel === this.httpActivity.channel) {

This is it, huh? Were we not matching aRequest to an item's channel previously?

No comments on the tests. They look fine.

You should run this through try server on all platforms if you haven't already. F+ with good results there.
Attachment #509803 - Flags: feedback?(rcampbell) → feedback+
(In reply to comment #4)
> Comment on attachment 509803 [details] [diff] [review]
> proposed fix
> 
> -      if (item.channel === aRequest) {
> +      if (item.channel === this.httpActivity.channel) {
> 
> This is it, huh? Were we not matching aRequest to an item's channel previously?

Yep.

> No comments on the tests. They look fine.
> 
> You should run this through try server on all platforms if you haven't already.
> F+ with good results there.

Thanks for the feedback+! Will push the updated patch to the try server, and post the results here.

I am going to update the patch to solve the second issue reported by Stefan - the body... :)
Updated the patch. Now for redirect requests we do not show any body, even if body logging is enabled. We do this because the response listener always gets the data from the redirect target request - there seems nothing I can do to get the "original data" (the data coming from the redirect request). It seems things are handled at a lower level in Gecko. Chromium's Web Inspector also does not show the content for redirect requests, and Firebug also fails to do it. So, that seems to be a pattern. ;)

Let me know if this patch is fine. I am going to push it to the try server now.
Attachment #509803 - Attachment is obsolete: true
Attachment #510028 - Flags: feedback?(rcampbell)
Whiteboard: [patchclean:0204] → [patchclean:0205]
Comment on attachment 510028 [details] [diff] [review]
[checked-in] updated patch

-      if (item.channel === aRequest) {
+      if (item.channel === this.httpActivity.channel) {

Didn't I see this in another bug already?

Looks good.
Attachment #510028 - Flags: feedback?(rcampbell) → feedback+
(In reply to comment #8)
> Comment on attachment 510028 [details] [diff] [review]
> updated patch
> 
> -      if (item.channel === aRequest) {
> +      if (item.channel === this.httpActivity.channel) {
> 
> Didn't I see this in another bug already?

Yes, you did see this, in this bug, actually. In an older version of the patch.

I updated the patch to also cover the problem reported in comment #3. Hence, I asked for feedback again (there have been quite some changes).

> Looks good.

Thanks for the feedback+!
Comment on attachment 510028 [details] [diff] [review]
[checked-in] updated patch

Asking for review from Dolske.
Attachment #510028 - Flags: review?(dolske)
Comment on attachment 510028 [details] [diff] [review]
[checked-in] updated patch

rs=me
Attachment #510028 - Flags: review?(dolske)
Attachment #510028 - Flags: review+
Attachment #510028 - Flags: approval2.0+
Thanks for the review+ and approval, Dolske!
Whiteboard: [patchclean:0205] → [patchclean:0205][checkin]
Comment on attachment 510028 [details] [diff] [review]
[checked-in] updated patch

http://hg.mozilla.org/mozilla-central/rev/d50fdfb0426d
Attachment #510028 - Attachment description: updated patch → [checked-in] updated patch
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: [patchclean:0205][checkin] → [patchclean:0205]
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.