Closed Bug 1733558 Opened 4 months ago Closed 4 months ago

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

Categories

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

Desktop
All
defect

Tracking

()

RESOLVED FIXED
95 Branch
Tracking Status
firefox94 --- wontfix
firefox95 --- fixed

People

(Reporter: Gijs, Assigned: Gijs)

Details

Attachments

(1 file)

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.

Pushed by gijskruitbosch@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c04e9ee53e53
stop duplicating append redirect history entry logic everywhere, r=ckerschb,necko-reviewers,valentin

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

Severity: -- → S3
Priority: -- → P2
Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 95 Branch

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.

Flags: needinfo?(gijskruitbosch+bugs)
Flags: needinfo?(gijskruitbosch+bugs)
You need to log in before you can comment on or make changes to this bug.