1580159 - [network markers] The "REDIRECT" marker should be in Redirect3Complete instead of Redirect1Begin

Status: NEW

(Core :: Gecko Profiler, defect, P2)

Description

[Julien Wajsberg [:julienw]]

This comes from a comment by Honza in [1].

Quoting Honza:

move this here: https://searchfox.org/mozilla-central/rev/250f5cc9fb8bdcbb6b23d2a06acfd48addb2f99b/netwerk/protocol/http/HttpChannelChild.cpp#2085

after Redirect1Begin, OnStopRequest can still potentially be called for this channel.

Honza, I'd love to have more specifics about when this can happen.

[1] https://phabricator.services.mozilla.com/D42831

Comment 1

[Julien Wajsberg [:julienw]]

And also:

move to https://searchfox.org/mozilla-central/rev/250f5cc9fb8bdcbb6b23d2a06acfd48addb2f99b/netwerk/protocol/http/nsHttpChannel.cpp#274 AutoRedirectVetoNotifier::RedirectSucceeded

Comment 2

[Honza Bambas (:mayhemer)]

(In reply to Julien Wajsberg [:julienw] from comment #0)

This comes from a comment by Honza in [1].

Quoting Honza:

move this here: https://searchfox.org/mozilla-central/rev/250f5cc9fb8bdcbb6b23d2a06acfd48addb2f99b/netwerk/protocol/http/HttpChannelChild.cpp#2085

after Redirect1Begin, OnStopRequest can still potentially be called for this channel.

Honza, I'd love to have more specifics about when this can happen.

[1] https://phabricator.services.mozilla.com/D42831

Redirect1Begin only starts a so called "redirect vetoing process", where a line of listeners, both global (=seeing all requests) and assigned on the "old" channel via notification callbacks, may examine the redirect, delay it, or cancel it (aka veto it). In case of vetoing (CPS checking failure, whatever other reason) we will not follow the redirect and deliver the 30X response instead to the channel consumer (the one that asyncopened the channel). Hence, OnStopRequest on the old child channel still may be called in that case.

(In reply to Julien Wajsberg [:julienw] from comment #1)

And also:

move to https://searchfox.org/mozilla-central/rev/250f5cc9fb8bdcbb6b23d2a06acfd48addb2f99b/netwerk/protocol/http/nsHttpChannel.cpp#274 AutoRedirectVetoNotifier::RedirectSucceeded

RedirectSucceeded() is called after all redirect checking steps passed and we have successfully asyncopened the "new" channel we are redirecting to. Opening it is the last step that has to succeed to say that "the redirect was a success". If that fails, we will still call OnStopRequest on the "old" child channel. Note that the place where you are taking the marker is only one of places we are starting a redirection.

Comment 3

[Honza Bambas (:mayhemer)]

It would also be good to state exactly what these markers are about to capture, so I'm not advising something that I technically see as correct, but informatively is wrong.

Comment 4

[Julien Wajsberg [:julienw]]

A marker STATUS_REDIRECT is the end pair of a STATUS_START for a redirected request (reminder: the end pair can either be STATUS_REDIRECT or STATUS_STOP).
Clearly we can't have both a STATUS_REDIRECT and a STATUS_STOP for the same STATUS_START. So I think you're right...

Can you elaborate on the place where you are taking the marker is only one of places we are starting a redirection.? Also remember that we create a new marker STATUS_START for the new channel as part of the redirect process (this was our discussion at the last all hands :) ).

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Julien Wajsberg [:julienw] from comment #4)

the place where you are taking the marker is only one of places we are starting a redirection.?

there are I think 4 places we start a redirection. search for SetupReplacementChannel calls.

Also remember that we create a new marker STATUS_START for the new channel as part of the redirect process (this was our discussion at the last all hands :) ).

that's correct!

so ideally this should look like, in the order of capturing:
STATUS_START for channel 1
STATUS_START for channel 2
STATUS_REDIRECT for channel 1
STATUS_STOP for channel 2

where channel 1 is redirected to channel 2 and channel 2 delivers the content (gets 200).

in case channel 1 redirection is stopped (vetoed), it should look like:

STATUS_START for channel 1
STATUS_STOP for channel 1

and no marker for channel 2, at all.

then, what I suggest seems right.

one more note: the STATUS_START marker must be captured when asyncOpen returns OK, not if it fails, because then OnStopRequest will never be called.

Comment 6

[Julien Wajsberg [:julienw]]

Makes sense, thanks for the feedback!