1717047 - [network markers] Support all redirections in the parent process

Status: RESOLVED FIXED

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

Description

[Julien Wajsberg [:julienw]]

Currently we're missing out a lot of redirections:

• STS
• proxy
• from addons

In this bug I want to experiment moving adding LOAD_REDIRECT markers directly in SetupReplacementChannel which is used in all redirections.

I'll probably also reuse asuth's code from https://phabricator.services.mozilla.com/D112675, because after having a redirect marker when redirecting to InterceptedChannel we also need a new id and a new "start" marker.

This may not be the best solution for the end users, but we may want to hide or merge internal redirections in the UI later on.

Comment 1

[Julien Wajsberg [:julienw]]

Attachment: WIP: Bug 1717047 - [profiler, netwerk] Support all redirections in the parent process r=valentin

This moves the adding of the end marker for redirects in nsHttpChannel
to SetupReplacementChannel, so that all redirects are properly caught.
Without that, some requests will show as unfinished in the profiler
frontend.

Some of the redirects are internal and we may be able to flag and ignore
them in the frontend, but that's work for the future.

Comment 2

[Julien Wajsberg [:julienw]]

Attachment: WIP: Bug 1717047 - [netwerk] Generate a new channel id when redirecting to the service worker r=valentin

This ensures that the new intercepted channel gets a new id, so that we
can properly match start and end markers in the frontend.

Depends on D118823

Comment 3

[Julien Wajsberg [:julienw]]

Attachment: WIP: Bug 1717047 - [profiler, netwerk] Insert a new "start" network marker for intercepted channels r=valentin

Following the change in the previous commit where we generate a new id
for intercepted channels, we now need a "start" marker for this channel.

Depends on D118824

Comment 4

[Julien Wajsberg [:julienw]]

Attachment: WIP: Bug 1717047 - [profiler, netwerk] Update existing tests to account for the new changes r=valentin

Now we need to update all existing tests related to service workers,
because they all have an extra redirect marker.

Depends on D118825

Comment 5

[Julien Wajsberg [:julienw]]

Attachment: WIP: Bug 1717047 - [profiler, netwerk] Add a test for the STS case r=canaltinova

This adds a new profiler test for handling the http->https case that we
wouldn't catch before this patchset.

Depends on D118826

Comment 6

[Julien Wajsberg [:julienw]]

try is https://treeherder.mozilla.org/jobs?repo=try&revision=49d1149be00b6ff7feadaacae9a5a81b8f84c5d6

Comment 7

[Julien Wajsberg [:julienw]]

Attachment: Bug 1717047 - [profiler, netwerk] Support all redirections in the parent process r=canaltinova,#necko-reviewers

This moves the adding of the end marker for redirects in nsHttpChannel
to SetupReplacementChannel, so that all redirects are properly caught.
Without that, some requests will show as unfinished in the profiler
frontend.

Some of the redirects are internal and we may be able to flag and ignore
them in the frontend, but that's work for the future.

Because all redirections get a "REDIRECT" end marker, including the
internal redirection to the service worker's intercepted channel, we now
need an additional "START" marker there as well.

All existing profiler tests related to service workers needed to be
updated because there's an extra redirect marker in all these requests,
as well as several pairs of markers that all have the same id.

This also adds a new profiler test for handling the http->https case
that we wouldn't catch before this patch.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/932f7478dc2a
[profiler, netwerk] Support all redirections in the parent process r=canaltinova,necko-reviewers,dragana

Comment 9

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/932f7478dc2a

Comment 10

[Sandor Molnar[:smolnar]]

Backed out for causing bug 1719684

Backout link: https://hg.mozilla.org/integration/autoland/rev/94d7910fbb31106daa0395424d67211b7c806e69

Comment 11

[Julien Wajsberg [:julienw]]

Thanks for the backout!

Comment 12

[Alexandru Michis [:malexandru]]

Backout merged to central: https://hg.mozilla.org/mozilla-central/rev/94d7910fbb31106daa0395424d67211b7c806e69

Comment 13

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:julienw, could you have a look please?
For more information, please visit auto_nag documentation.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/00803501007e
[profiler, netwerk] Support all redirections in the parent process r=canaltinova,necko-reviewers,dragana

Comment 15

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/00803501007e