1733558 - nsILoadInfo::AppendRedirectHistoryEntry should take an nsIChannel and construct the entry itself

Status: RESOLVED FIXED

(Core :: DOM: Navigation, defect, P2)

Description

[:Gijs (he/him)]

There are 4 non-test consumers of appendRedirectHistoryEntry in tree: https://searchfox.org/mozilla-central/search?q=appendredirecthistoryentry

	netwerk/base/nsBaseChannel.cpp
93	newLoadInfo->AppendRedirectHistoryEntry(entry, isInternalRedirect); // found in nsBaseChannel::Redirect
	netwerk/ipc/DocumentLoadListener.cpp
1409	redirectLoadInfo->AppendRedirectHistoryEntry(entry, true); // found in mozilla::net::DocumentLoadListener::SerializeRedirectData
	netwerk/protocol/http/HttpBaseChannel.cpp
3952	newLoadInfo->AppendRedirectHistoryEntry(entry, isInternalRedirect); // found in mozilla::net::HttpBaseChannel::CloneLoadInfoForRedirect
	netwerk/protocol/http/HttpChannelChild.cpp
1550	mLoadInfo->AppendRedirectHistoryEntry(entry, false); // found in mozilla::net::HttpChannelChild::CleanupRedirectingChannel

Looking at these consumers:

https://searchfox.org/mozilla-central/rev/eecd2dd4ba630bea4ce103623a4bfb398065b64b/netwerk/base/nsBaseChannel.cpp#93
https://searchfox.org/mozilla-central/rev/eecd2dd4ba630bea4ce103623a4bfb398065b64b/netwerk/ipc/DocumentLoadListener.cpp#1409
https://searchfox.org/mozilla-central/rev/eecd2dd4ba630bea4ce103623a4bfb398065b64b/netwerk/protocol/http/HttpBaseChannel.cpp#3952
https://searchfox.org/mozilla-central/rev/eecd2dd4ba630bea4ce103623a4bfb398065b64b/netwerk/protocol/http/HttpChannelChild.cpp#1550

what's striking is that they all do the same thing to determine the URL to add: they ask the script security manager for the channel's URI principal (the http channels use the channel's own GetURIPrincipal helper for this, which just defers to the script security manager anyway, and has no other callers).

The ones in http code also both check the computed referrer and remote address off the channel.

So ISTM we could just push the construction of the entry into AppendRedirectHistoryEntry itself, and have it take an nsIChannel as an argument. As a bonus, this makes it easier to call this method from JS code, because it is likely to have a channel reference, but cannot itself construct the native redirect history entry objects, and any JS implementation is not going to be threadsafe.

I have a patch for this.

Comment 1

[:Gijs (he/him)]

Attachment: Bug 1733558 - stop duplicating append redirect history entry logic everywhere, r?ckerschb!,#necko-reviewers!

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c04e9ee53e53
stop duplicating append redirect history entry logic everywhere, r=ckerschb,necko-reviewers,valentin

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

Changing severity to S3 because of triaging a bug which has a patch which just landed to autoland.

Comment 4

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/c04e9ee53e53

Comment 5

[Release mgmt bot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:Gijs, is this bug important enough to require an uplift?
If not please set status_beta to wontfix.

For more information, please visit auto_nag documentation.