Closed Bug 1761248 Opened 3 years ago Closed 24 days ago

Expose API to report Early hints information for devtools

Categories

(Core :: Networking: HTTP, enhancement, P3)

Product:
Core
Component:
Networking: HTTP
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
134 Branch
Tracking Flags:
Tracking Status
firefox134 --- fixed

People

(Reporter: manuel, Assigned: omansfeld)

References

(Blocks 1 open bug)

Details

(Whiteboard: [necko-triaged])

Attachments

(1 file, 2 obsolete files)

Bug 1761248 - Expose API to display early hint info in devtools, r=#necko
48 bytes, text/x-phabricator-request
Details | Review

Probably need to add it to the correct load groupd and possibly the loadingNode (if available?). Example of a custom request: /devtools/server/actors/network-monitor/network-content.js#69-89

Severity: -- → N/A
Priority: -- → P2
Whiteboard: [necko-triaged]

The problem with this seems to be, that we don't have the loadingNode at the stage, where the preload happens. It is just too early for it to exist yet. (Requesting aLoadInfo->LoadingNode() returns a nullptr). Therefore resolving this might need a refactor.

Unassigning me for now. I do not plan to work on this, unless someone can tell me how to refactor. And for me that isn't high priority right now.

Assignee: mbucher → nobody
Status: ASSIGNED → NEW

I've looked this a bit.
I tried to turn on network.early-hints.enabled and go to https://early-hints.fastlylabs.com/ with devtool opened.
From the log, I can see that Firefox received 103 response.

HTTP/2 103
origin-trial: AkLE+IZgEkDmkx/CXL+LCZusUG5cQH2CqPMkyD5lfTDuKf78sg+Ua6q5SC5SO++Dh3TCKkaWIdEwQYcOdf5ENQsAAAByeyJvcmlnaW4iOiJodHRwczovL2Vhcmx5LWhpbnRzLmZhc3RseWxhYnMuY29tOjQ0MyIsImZlYXR1cmUiOiJFYXJseUhpbnRzUHJlbG9hZEZvck5hdmlnYXRpb24iLCJleHBpcnkiOjE2MzY1MDIzOTl9
link: </hinted.png>; rel=preload; as=image
X-Firefox-Spdy: h2

We also send another request to fetch hinted.png.

[Parent 50908: Main Thread]: D/nsHttp Creating nsHttpChannel [this=1167b6000]
[Parent 50908: Main Thread]: E/nsHttp HttpBaseChannel::Init [this=1167b6000]
[Parent 50908: Main Thread]: E/nsHttp host=early-hints.fastlylabs.com port=-1
[Parent 50908: Main Thread]: E/nsHttp uri=https://early-hints.fastlylabs.com/hinted.png
[Parent 50908: Main Thread]: E/nsHttp nsHttpChannel::Init [this=1167b6000]

In _httpResponseExaminer, I can see that devtool did get this http request by adding the log after this line.
The reason that why we can't see ``hinted.pngshown in devtool UI is that [NetworkUtils.matchRequest](https://searchfox.org/mozilla-central/rev/bf6f194694c9d1ae92847f3d4e4c15c2486f3200/devtools/server/actors/network-monitor/network-observer.js#383) returns false. I think we need to figure out howNetworkUtils.matchRequest` works and try to make this preload request match the filter.
Marc, not sure my investigation matches yours. Could you take a look? Thanks.

Flags: needinfo?(mleclair)

Hi Honza,

As this WIP patch shows, we are trying to show 103 Early Hints response in devtools.
The WIP patch implements the first step, which is adding a new http activity sub type ACTIVITY_SUBTYPE_EH_RESPONSE_HEADER. With this new subtype, necko can send the content of early hint header as what we do currently.
The problem right now is that we don't know how to modify network-observer.js to show the early hint response.
Could you provide us some guideline to help us move this bug forward?

Thanks.

Flags: needinfo?(odvarko)

Bomsy, could you please help out here, thank you.

Flags: needinfo?(odvarko) → needinfo?(hmanilla)

Note that the early hint response from server would be like:

Server response:

  HTTP/1.1 103 Early Hints
  Link: </style.css>; rel=preload; as=style
  Link: </script.js>; rel=preload; as=script

  HTTP/1.1 200 OK
  Date: Fri, 26 May 2017 10:02:11 GMT
  Content-Length: 1234
  Content-Type: text/html; charset=utf-8
  Link: </style.css>; rel=preload; as=style
  Link: </script.js>; rel=preload; as=script

  <!doctype html>
  [... rest of the response body is omitted from the example ...]

This means that we get 103 Early Hints first, and then get 200 OK for the same http request. However, it seems that we only support to show only one status for each request in network request list. With my local test, looks like the 103 status is replaced by 200 OK later. I am not sure how easy to modify our devtool code to support this.

Hi Kershaw,
Looking at your WIP, its quite close. I'll pull the patch and have a look to see what stops it from showing.

(In reply to Kershaw Chang [:kershaw] from comment #7)

Note that the early hint response from server would be like:

Server response:

  HTTP/1.1 103 Early Hints
  Link: </style.css>; rel=preload; as=style
  Link: </script.js>; rel=preload; as=script

  HTTP/1.1 200 OK
  Date: Fri, 26 May 2017 10:02:11 GMT
  Content-Length: 1234
  Content-Type: text/html; charset=utf-8
  Link: </style.css>; rel=preload; as=style
  Link: </script.js>; rel=preload; as=script

  <!doctype html>
  [... rest of the response body is omitted from the example ...]

This means that we get 103 Early Hints first, and then get 200 OK for the same http request. However, it seems that we only support to show only one status for each request in network request list. With my local test, looks like the 103 status is replaced by 200 OK later. I am not sure how easy to modify our devtool code to support this.

Yes we do not support multiple statues in for one request, it'll need a bit of work to support that, especially if we need statues to update as they happen.
An alternative (at least for now) which should just work would be to show two requests , one for the 103 and 200 status when the requests come in.
Then the raw response header for the 103 early hint request can just show the above. Will that work?

Flags: needinfo?(hmanilla)

Yes we do not support multiple statues in for one request, it'll need a bit of work to support that, especially if we need statues to update as they happen.
An alternative (at least for now) which should just work would be to show two requests , one for the 103 and 200 status when the requests come in.
Then the raw response header for the 103 early hint request can just show the above. Will that work?

Thanks for the feedback, :bomsy.
Looks like it's not easy to support this. I think maybe it's not worth to spend a lot of time just for showing 103 response in devtool.
We can file a follow up bug and deal with this later.

(In reply to Kershaw Chang [:kershaw] from comment #3)

I've looked this a bit.
I tried to turn on network.early-hints.enabled and go to https://early-hints.fastlylabs.com/ with devtool opened.
From the log, I can see that Firefox received 103 response.

HTTP/2 103
origin-trial: AkLE+IZgEkDmkx/CXL+LCZusUG5cQH2CqPMkyD5lfTDuKf78sg+Ua6q5SC5SO++Dh3TCKkaWIdEwQYcOdf5ENQsAAAByeyJvcmlnaW4iOiJodHRwczovL2Vhcmx5LWhpbnRzLmZhc3RseWxhYnMuY29tOjQ0MyIsImZlYXR1cmUiOiJFYXJseUhpbnRzUHJlbG9hZEZvck5hdmlnYXRpb24iLCJleHBpcnkiOjE2MzY1MDIzOTl9
link: </hinted.png>; rel=preload; as=image
X-Firefox-Spdy: h2

We also send another request to fetch hinted.png.

[Parent 50908: Main Thread]: D/nsHttp Creating nsHttpChannel [this=1167b6000]
[Parent 50908: Main Thread]: E/nsHttp HttpBaseChannel::Init [this=1167b6000]
[Parent 50908: Main Thread]: E/nsHttp host=early-hints.fastlylabs.com port=-1
[Parent 50908: Main Thread]: E/nsHttp uri=https://early-hints.fastlylabs.com/hinted.png
[Parent 50908: Main Thread]: E/nsHttp nsHttpChannel::Init [this=1167b6000]

In _httpResponseExaminer, I can see that devtool did get this http request by adding the log after this line.
The reason that why we can't see hinted.png shown in devtool UI is that NetworkUtils.matchRequest returns false.
I think we need to figure out how NetworkUtils.matchRequest works and try to make this preload request match the filter.
Marc, not sure my investigation matches yours. Could you take a look? Thanks.

I've looked into NetworkUtils.matchRequest more. There are two reasons that this function returns false for an early hint request:

  1. The early hint request uses a system principal to load for now and this will be fixed in bug 1780822.
  2. The early hint request doesn't have a browsingContext.
Depends on: 1780822

After bug 1780822 landed, I think all we need is just the browsing context id.
We can get the browsing context id from the load info here.

Blocks: 1802501
No longer blocks: earlyhints
Summary: Add early hint preload to devtools → Expose API to report Early hints information for devtools

Comment on attachment 9313038 [details]
WIP: Bug 1761248 - [devtools] Show the early hint header information in devtools

Revision D167254 was moved to bug 1811455. Setting attachment 9313038 [details] to obsolete.

Attachment #9313038 - Attachment is obsolete: true
Flags: needinfo?(mleclair)

Hi Kershaw, Marc,
Is there any other blocker on this bug?
The current patch attached seems like it is close. If the patch is in the right direction, i can pick it up and try to complete it, (If someone can mentor).

I was looking to do the devtools part of the work in Bug 1802501 which is blocked by this.

Flags: needinfo?(mleclair)
Flags: needinfo?(kershaw)

(In reply to Hubert Boma Manilla (:bomsy) from comment #15)

Hi Kershaw, Marc,
Is there any other blocker on this bug?
The current patch attached seems like it is close. If the patch is in the right direction, i can pick it up and try to complete it, (If someone can mentor).

I was looking to do the devtools part of the work in Bug 1802501 which is blocked by this.

The attached patch is a good start, but it still require some works.
After bug 1828882 landed, this bug is all about showing 103 Early Hint response in devtools. However, as I said in comment #9, I am not sure if we want to spend time on this. It looks like Chrome doesn't support this for now, but they do show 101 response in devtools.

Flags: needinfo?(mleclair)
Priority: P2 → P3
Flags: needinfo?(kershaw)

Hi Kershaw,
Any updates on this API?

Flags: needinfo?(kershaw)

Sorry for the delay.
I've asked Oskar to work on this.

Flags: needinfo?(kershaw) → needinfo?(omansfeld)
Assignee: nobody → omansfeld
Flags: needinfo?(omansfeld)

Hey, just a question for clarification after reading up on the bug history:

This is now only for exposing the early hints info to devtools and then the devtools integration will be done by Bomsy in bug 1811455, correct?

Flags: needinfo?(hmanilla)

I've just submitted a new WIP patch that should expose the Early Hint header info. For testing purposes I put dump(extraStringData)'s in NetworkObserver.sys.mjs into the different switch-cases here, which, when visiting https://early-hints.fastlylabs.com results in:

early hint at devtools:
HTTP/2 103
origin-trial: AkLE+IZgEkDmkx/CXL+LCZusUG5cQH2CqPMkyD5lfTDuKf78sg+Ua6q5SC5SO++Dh3TCKkaWIdEwQYcOdf5ENQsAAAByeyJvcmlnaW4iOiJodHRwczovL2Vhcmx5LWhpbnRzLmZhc3RseWxhYnMuY29tOjQ0MyIsImZlYXR1cmUiOiJFYXJseUhpbnRzUHJlbG9hZEZvck5hdmlnYXRpb24iLCJleHBpcnkiOjE2MzY1MDIzOTl9
link: </hinted.png>; rel=preload; as=image
X-Firefox-Spdy: h2

response header at devtools:
HTTP/2 200
server: GitHub.com
content-type: image/png
[...]

As you can see both the early hint info and the following 200 response for the resource are exposed to devtools. In contrary to comment #7 it seems like they show up separately, but maybe I'm missing something here.

I couldn't figure out how to make the 103 actually show up in the Netmonitor though. The 200 shows up (but is not displayed correctly as per bug 1838457).

Is this enough for you to continue work on the devtool side or do you need anything else?

(Also needinfo'ing :kershaw so he can take a look at the patch and maybe chime in regarding the responses showing up separately)

Flags: needinfo?(kershaw)
Attachment #9287180 - Attachment is obsolete: true

(In reply to Oskar Mansfeld from comment #19)

Hey, just a question for clarification after reading up on the bug history:

This is now only for exposing the early hints info to devtools and then the devtools integration will be done by Bomsy in bug 1811455, correct?

Yes this is correct.

Flags: needinfo?(hmanilla)

(In reply to Oskar Mansfeld from comment #21)

I've just submitted a new WIP patch that should expose the Early Hint header info. For testing purposes I put dump(extraStringData)'s in NetworkObserver.sys.mjs into the different switch-cases here, which, when visiting https://early-hints.fastlylabs.com results in:

early hint at devtools:
HTTP/2 103
origin-trial: AkLE+IZgEkDmkx/CXL+LCZusUG5cQH2CqPMkyD5lfTDuKf78sg+Ua6q5SC5SO++Dh3TCKkaWIdEwQYcOdf5ENQsAAAByeyJvcmlnaW4iOiJodHRwczovL2Vhcmx5LWhpbnRzLmZhc3RseWxhYnMuY29tOjQ0MyIsImZlYXR1cmUiOiJFYXJseUhpbnRzUHJlbG9hZEZvck5hdmlnYXRpb24iLCJleHBpcnkiOjE2MzY1MDIzOTl9
link: </hinted.png>; rel=preload; as=image
X-Firefox-Spdy: h2

response header at devtools:
HTTP/2 200
server: GitHub.com
content-type: image/png
[...]

As you can see both the early hint info and the following 200 response for the resource are exposed to devtools. In contrary to comment #7 it seems like they show up separately, but maybe I'm missing something here.

I couldn't figure out how to make the 103 actually show up in the Netmonitor though. The 200 shows up (but is not displayed correctly as per bug 1838457).

Is this enough for you to continue work on the devtool side or do you need anything else?

if we got both events on the devtools end seperately, it should be ok for devtools. I'll test out the patch again in a bit and get back to you.

Thanks

Flags: needinfo?(hmanilla)

Hi Oskar,
Apologies for the delayed response.
I tested the patch and i see both early hint and response notifications. I also confirm that both notifications are from the same channel.
The patch gives what we need for basic early hint notification.

Thanks for working on this. Is there anything else needed to finish and land this from our end?

Flags: needinfo?(hmanilla) → needinfo?(omansfeld)
Attachment #9431894 - Attachment description: WIP: Bug 1761248 - Expose API to display early hint info in devtools → Bug 1761248 - Expose API to display early hint info in devtools, r=#necko

Perfect, I just removed the dump() statements that were for testing and opened this for review. I think this can be landed independently from the devtools part, or do you want to land it as a stack?

Flags: needinfo?(omansfeld) → needinfo?(hmanilla)

(In reply to Oskar Mansfeld from comment #25)

Perfect, I just removed the dump() statements that were for testing and opened this for review. I think this can be landed independently from the devtools part, or do you want to land it as a stack?

It's fine. lets land it independently.

Flags: needinfo?(hmanilla)
Flags: needinfo?(kershaw)
Pushed by kjang@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0821a9b45984 Expose API to display early hint info in devtools, r=necko-reviewers,devtools-reviewers,bomsy,kershaw

https://hg.mozilla.org/mozilla-central/rev/0821a9b45984

Status: NEW → RESOLVED
Closed: 24 days ago
status-firefox134: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 134 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: