Get header source from the backend

RESOLVED FIXED in Firefox 38

Status

()

Firefox
Developer Tools: Netmonitor
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Honza, Assigned: Honza)

Tracking

unspecified
Firefox 38
x86_64
Windows 7
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox38 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Assignee)

Description

3 years ago
It should be possible to get also the raw header source for HTTP requests/responses from the backend.

The source can be sent as a response to the existing "getRequestHeader" packet sent to "netEvent" actor.

E.g:

{
  from: "actor-id",
  headers: [],
  headersSize: 0,
  source: String  // <--- NEW
}

(note that 'source' field isn't currently part of the HAR spec)

Or it could be sent as part of new request type.

Thoughts?

Honza
(Assignee)

Comment 1

3 years ago
@Victor: what do you think (should I CC someone else yet?)

Honza
Flags: needinfo?(vporof)
Yes, it would definitely be useful, and it's a real blocker for some bugs ("show actual raw http headers" etc.). Currently, we reconstruct everything on the frontend side, but that's not such a great thing to do.
Flags: needinfo?(vporof)
(Assignee)

Comment 3

3 years ago
Created attachment 8560543 [details] [diff] [review]
bug1130318-1.patch

What about this?

Honza
Assignee: nobody → odvarko
Attachment #8560543 - Flags: review?(vporof)
Comment on attachment 8560543 [details] [diff] [review]
bug1130318-1.patch

Review of attachment 8560543 [details] [diff] [review]:
-----------------------------------------------------------------

Very nice. Needs a test and making the headers source a long string actor.

::: toolkit/devtools/server/actors/webconsole.js
@@ +1722,5 @@
>      return {
>        from: this.actorID,
>        headers: this._request.headers,
>        headersSize: this._request.headersSize,
> +      headersSource: this._request.headersSource,

I like this. Maybe rename headersSource to rawHeaders, and it should be a long string actor, not a plain string, to avoid sending too much data over the protocol.

@@ +1781,5 @@
>      return {
>        from: this.actorID,
>        headers: this._response.headers,
>        headersSize: this._response.headersSize,
> +      headersSource: this._response.headersSource,

Ditto.

@@ +1913,5 @@
>      this._response.httpVersion = aInfo.httpVersion;
>      this._response.status = aInfo.status;
>      this._response.statusText = aInfo.statusText;
>      this._response.headersSize = aInfo.headersSize;
> +    this._response.headersSource = aInfo.headersSource;

I don't think we should send this here. The other two requests are enough.
Attachment #8560543 - Flags: review?(vporof) → feedback?
Attachment #8560543 - Flags: feedback? → feedback+
(Assignee)

Comment 5

3 years ago
(In reply to Victor Porof [:vporof][:vp] from comment #4)

> I like this. Maybe rename headersSource to rawHeaders, and it should be a
> long string actor, not a plain string, to avoid sending too much data over
> the protocol.
Fixed now. The source is sent on demand (getRequestHeaders/getResponseHeaders) and it's a LongStringActor.

> Needs a test
Is there an existing test I could start from?

Honza
(Assignee)

Comment 6

3 years ago
Created attachment 8562007 [details] [diff] [review]
bug1130318-2.patch

Ah, here is the patch...

Honza
Attachment #8560543 - Attachment is obsolete: true
Attachment #8562007 - Flags: review?(vporof)
Comment on attachment 8562007 [details] [diff] [review]
bug1130318-2.patch

Review of attachment 8562007 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with a test
Attachment #8562007 - Flags: review?(vporof) → review+
(In reply to Jan Honza Odvarko [:Honza] from comment #5)
> (In reply to Victor Porof [:vporof][:vp] from comment #4)
> Is there an existing test I could start from?
> 
> Honza

toolkit/devtools/webconsole/test/test_network_get.html and/or test_network_post.html?
(Assignee)

Comment 9

3 years ago
Created attachment 8562755 [details] [diff] [review]
bug1130318-3.patch

Both tests extended to cover also rawHeaders field.

Honza
Attachment #8562007 - Attachment is obsolete: true
Attachment #8562755 - Flags: review?(vporof)
Attachment #8562755 - Flags: review?(vporof) → review+
(Assignee)

Comment 10

3 years ago
Pushed to try: https://tbpl.mozilla.org/?tree=Try&rev=d57e8e46f502
Looks good, I thing the patch is ready to land.

Honza
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/a76b4f207fa7
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/a76b4f207fa7
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox38: --- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 38
You need to log in before you can comment on or make changes to this bug.