1175803 - Store redirect chain within loadInfo

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

Whenever a channel gets redirected we should store the channel uri within the loadInfo, so that at all times throughout the lifetime of a channel we can retrace all the redirects a channel went through.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirect_chain_in_loadinfo.patch

Comment 2

[Christoph Kerschbaumer [:ckerschb]]

Hey Ben, any chance you can take a look at the patch and provide some feedback? In particular I have two questions:

a) Within LoadInfoToLoadInfoArgs(nsILoadInfo *aLoadInfo,...) what is the best way to write into aOptionalLoadInfoArgs? Currently I have to create a dummy so I can somehow make the mType sanity check shut up, but I am pretty sure there is a better way of doing this, right?

b) The code in the patch by itself seems to work. Now I performed a little bit of testing. E.g. when querying the redirectedChain from the loadInfo for example in nsMixedContentBlocker::AsyncOnChannelRedirect (see inline code unerneath), then I run into the following problem (see stacktrace). I am not sure what causes the duplicate release, any ideas? Maybe it has indeed to do with the problem explained in (a) and has to do with scoping of the dummy variable on the stack.


Assertion failure: int32_t(mRefCnt) > 0 (dup release), at /home/ckerschb/moz/mc/xpcom/ds/nsArray.cpp:48
#01: nsArrayCC::Release() (/home/ckerschb/moz/mc/xpcom/ds/nsArray.cpp:48)
#02: ~nsCOMPtr (/home/ckerschb/moz/mc-obj-dbg/xpcom/ds/../../dist/include/nsCOMPtr.h:391)
#03: ~LoadInfo (/home/ckerschb/moz/mc/netwerk/base/LoadInfo.cpp:105)
#04: mozilla::LoadInfo::Release() (/home/ckerschb/moz/mc/netwerk/base/LoadInfo.cpp:107)
#05: NS_ProxyRelease(nsIEventTarget*, nsISupports*, bool) (/home/ckerschb/moz/mc/xpcom/glue/nsProxyRelease.cpp:38)
#06: NS_ReleaseOnMainThread(nsISupports*, bool) (/home/ckerschb/moz/mc-obj-dbg/netwerk/base/../../dist/include/nsProxyRelease.h:121)
#07: nsresult NS_ReleaseOnMainThread<nsILoadInfo>(nsCOMPtr<nsILoadInfo>&, bool) (/home/ckerschb/moz/mc-obj-dbg/netwerk/base/../../dist/include/nsProxyRelease.h:80)
#08: ~HttpBaseChannel (/home/ckerschb/moz/mc/netwerk/protocol/http/HttpBaseChannel.cpp:109)
#09: ~HttpChannelChild (/home/ckerschb/moz/mc/netwerk/protocol/http/HttpChannelChild.cpp:197)
#10: ~HttpChannelChild (/home/ckerschb/moz/mc/netwerk/protocol/http/HttpChannelChild.cpp:195)
#11: mozilla::net::HttpChannelChild::Release() (/home/ckerschb/moz/mc/netwerk/protocol/http/HttpChannelChild.cpp:228)
#12: mozilla::net::HttpChannelChild::ReleaseIPDLReference() (/home/ckerschb/moz/mc/netwerk/protocol/http/HttpChannelChild.cpp:271)
#13: mozilla::net::NeckoChild::DeallocPHttpChannelChild(mozilla::net::PHttpChannelChild*) (/home/ckerschb/moz/mc/netwerk/ipc/NeckoChild.cpp:97)
#14: mozilla::net::PNeckoChild::RemoveManagee(int, mozilla::ipc::IProtocol*) (/home/ckerschb/moz/mc-obj-dbg/ipc/ipdl/./PNeckoChild.cpp:1103)
#15: non-virtual thunk to mozilla::net::PNeckoChild::RemoveManagee(int, mozilla::ipc::IProtocol*) (/home/ckerschb/moz/mc-obj-dbg/ipc/ipdl/UnifiedProtocols14.cpp:1228)
#16: mozilla::net::PHttpChannelChild::Send__delete__(mozilla::net::PHttpChannelChild*) (/home/ckerschb/moz/mc-obj-dbg/ipc/ipdl/./PHttpChannelChild.cpp:355)
#17: mozilla::net::HttpChannelChild::OnStopRequest(nsresult const&, mozilla::net::ResourceTimingStruct const&) (/home/ckerschb/moz/mc/netwerk/protocol/http/HttpChannelChild.cpp:796)
#18: mozilla::net::HttpChannelChild::RecvOnStopRequest(nsresult const&, mozilla::net::ResourceTimingStruct const&) (/home/ckerschb/moz/mc/netwerk/protocol/http/HttpChannelChild.cpp:741)
#19: mozilla::net::PHttpChannelChild::OnMessageReceived(IPC::Message const&) (/home/ckerschb/moz/mc-obj-dbg/ipc/ipdl/./PHttpChannelChild.cpp:616)
#20: mozilla::dom::PContentChild::OnMessageReceived(IPC::Message const&) (/home/ckerschb/moz/mc-obj-dbg/ipc/ipdl/./PContentChild.cpp:5415)
#21: mozilla::ipc::MessageChannel::DispatchAsyncMessage(IPC::Message const&) (/home/ckerschb/moz/mc/ipc/glue/MessageChannel.cpp:1279)
#22: mozilla::ipc::MessageChannel::DispatchMessage(IPC::Message const&) (/home/ckerschb/moz/mc/ipc/glue/MessageChannel.cpp:1198)
#23: mozilla::ipc::MessageChannel::OnMaybeDequeueOne() (/home/ckerschb/moz/mc/ipc/glue/MessageChannel.cpp:1182)
#24: void DispatchToMethod<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)()>(mozilla::ipc::MessageChannel*, bool (mozilla::ipc::MessageChannel::*)(), Tuple0 const&) (/home/ckerschb/moz/mc/ipc/chromium/src/base/tuple.h:388)
#25: RunnableMethod<mozilla::ipc::MessageChannel, bool (mozilla::ipc::MessageChannel::*)(), Tuple0>::Run() (/home/ckerschb/moz/mc/ipc/chromium/src/base/task.h:311)
#26: mozilla::ipc::MessageChannel::RefCountedTask::Run() (/home/ckerschb/moz/mc-obj-dbg/ipc/glue/../../dist/include/mozilla/ipc/MessageChannel.h:446)
#27: mozilla::ipc::MessageChannel::DequeueTask::Run() (/home/ckerschb/moz/mc-obj-dbg/ipc/glue/../../dist/include/mozilla/ipc/MessageChannel.h:463)
#28: MessageLoop::RunTask(Task*) (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:365)
#29: MessageLoop::DeferOrRunPendingTask(MessageLoop::PendingTask const&) (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:375)
#30: MessageLoop::DoWork() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:459)
#31: mozilla::ipc::DoWorkRunnable::Run() (/home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:221)
#32: nsThread::ProcessNextEvent(bool, bool*) (/home/ckerschb/moz/mc/xpcom/threads/nsThread.cpp:849)
#33: NS_ProcessNextEvent(nsIThread*, bool) (/home/ckerschb/moz/mc/xpcom/glue/nsThreadUtils.cpp:265)
#34: mozilla::ipc::MessagePump::Run(base::MessagePump::Delegate*) (/home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:95)
#35: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:290)
#36: MessageLoop::RunInternal() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:235)
#37: MessageLoop::RunHandler() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:228)
#38: MessageLoop::Run() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:201)
#39: nsBaseAppShell::Run() (/home/ckerschb/moz/mc/widget/nsBaseAppShell.cpp:165)
#40: XRE_RunAppShell (/home/ckerschb/moz/mc/toolkit/xre/nsEmbedFunctions.cpp:778)
#41: mozilla::ipc::MessagePumpForChildProcess::Run(base::MessagePump::Delegate*) (/home/ckerschb/moz/mc/ipc/glue/MessagePump.cpp:259)
#42: MessageLoop::RunInternal() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:235)
#43: MessageLoop::RunHandler() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:228)
#44: MessageLoop::Run() (/home/ckerschb/moz/mc/ipc/chromium/src/base/message_loop.cc:201)
#45: XRE_InitChildProcess (/home/ckerschb/moz/mc/toolkit/xre/nsEmbedFunctions.cpp:614)
#46: content_process_main(int, char**) (/home/ckerschb/moz/mc/ipc/app/../contentproc/plugin-container.cpp:236)
#47: main (/home/ckerschb/moz/mc/ipc/app/MozillaRuntimeMain.cpp:11)
#48: __libc_start_main (/build/buildd/eglibc-2.19/csu/libc-start.c:321)



// ---- snip ----

@@ -209,16 +210,50 @@ LogMixedContentMessage(MixedContentTypes
 NS_IMETHODIMP
 nsMixedContentBlocker::AsyncOnChannelRedirect(nsIChannel* aOldChannel,
                                               nsIChannel* aNewChannel,
                                               uint32_t aFlags,
                                               nsIAsyncVerifyRedirectCallback* aCallback)
 {
   nsAsyncRedirectAutoCallback autoCallback(aCallback);
 
+  {
+    nsCOMPtr<nsIURI> oldURI, newURI;
+    aOldChannel->GetURI(getter_AddRefs(oldURI));
+    aNewChannel->GetURI(getter_AddRefs(newURI));
+    nsAutoCString oldSpec, newSpec;
+    oldURI->GetSpec(oldSpec);
+    newURI->GetSpec(newSpec);
+    fprintf(stderr, "\n\n mixedCOntent {\n");
+    fprintf(stderr, "  old: %s\n", oldSpec.get());
+    fprintf(stderr, "  new: %s\n", newSpec.get());
+  }
+
+
+  { // debug
+    nsCOMPtr<nsIArray> redirectChain;
+    nsCOMPtr<nsILoadInfo> loadInfo;
+    aNewChannel->GetLoadInfo(getter_AddRefs(loadInfo));
+    nsresult rv = loadInfo->GetRedirectChain(getter_AddRefs(redirectChain));
+    NS_ENSURE_SUCCESS(rv, rv);
+    uint32_t length = 0;
+    rv = redirectChain->GetLength(&length);
+    NS_ENSURE_SUCCESS(rv, rv);
+
+    nsCOMPtr<nsIURI> uri;
+    nsAutoCString spec;
+    for (uint32_t i = 0; i < length; i++) {
+      rv = redirectChain->QueryElementAt(i, NS_GET_IID(nsIURI),
+                                         getter_AddRefs(uri));
+
+      uri->GetSpec(spec);
+      fprintf(stderr, "\n\n MIXED: %s\n", spec.get());
+    }
+  }
+
   if (!aOldChannel) {

Comment 3

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirect_chain_in_loadinfo.patch

Oh boy, the problem was that we have to use NS_IF_ADDREF within ::GetRedirecthChain. Anyway, it works now correctly!

@Jonas: This is what we had in mind, right? First, add the redirect chain to the loadInfo and second, clean up the loadInfo serialization so we do not serialize any loadInfo in case there is none. More precisely, do not use default arguments, but rather use a null loadInfo.

@Bent: Maybe you can still answer the following question in addition to your review of the patch:
> a) Within LoadInfoToLoadInfoArgs(nsILoadInfo *aLoadInfo,...) what is the
> best way to write into aOptionalLoadInfoArgs? Currently I have to create a
> dummy so I can somehow make the mType sanity check shut up, but I am pretty
> sure there is a better way of doing this, right?

@Steve: Currently nsBaseChannel.cpp as well as HttpBaseChannel.cpp are the only two places where we propagate the loadInfo after a redirect. As discussed briefly on IRC but now to make sure once again. Once we set up a new replacement channel for the old one it is fine to actually set the redirect uri on the loadInfo because the first channel becomes unused (freed/cleaned up) shortly after the redirect, right?

Comment 4

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8625029 [details] [diff] [review]
bug_1175803_store_redirect_chain_in_loadinfo.patch

Review of attachment 8625029 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/LoadInfo.h
@@ +79,5 @@
>    nsCOMPtr<nsIURI> mBaseURI;
>    uint64_t mInnerWindowID;
>    uint64_t mOuterWindowID;
>    uint64_t mParentOuterWindowID;
> +  nsCOMPtr<nsIMutableArray> mRedirectChain;

I kind'a hate nsIMutableArray. nsTArray<nsIURI> would be so much nicer to use.

The constructor part would be easy, just make the constructor take an nsTArray<nsIURI>* argument and use SwapElements to avoid any extra copying.

And having a C++ getter which returns a const nsTArray<nsIURI> argument would work great.

The only tricky part is the scriptable GetRedirectChain function. I think there are ways in xpidl to make that doable. Mind checking with bz or mrbkap if there's a reasonable way to do that?

Comment 5

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirect_chain_in_loadinfo.patch

(In reply to Jonas Sicking (:sicking) from comment #4)
> I kind'a hate nsIMutableArray. nsTArray<nsIURI> would be so much nicer to
> use.

'hate' is such a brutal word ;-)
Nevertheless, I totally agree, nsTArray is much nicer to use.
 
> The constructor part would be easy, just make the constructor take an
> nsTArray<nsIURI>* argument and use SwapElements to avoid any extra copying.

That sounds reasonable to me.

> And having a C++ getter which returns a const nsTArray<nsIURI> argument
> would work great.

So, we have two options, either we use:
* [ptr] native nsIURIArray(nsTArray<nsCOMPtr<nsIURI> >);
which is not refcounted, or we use
* [ref] ...
instead; but then we would have to copy the arguments in the getter function which is not that great in my opinion. See for example:
http://mxr.mozilla.org/mozilla-central/source/netwerk/dns/nsIDNSRecord.idl#57

> The only tricky part is the scriptable GetRedirectChain function. I think
> there are ways in xpidl to make that doable.

I haven't found that in our code. All uses of nsTArray are paired with [noscript]. I am not sure if that is possible at all.

> Mind checking with bz or mrbkap if there's a reasonable way to do that?

Boris, Blake are there ways to do that?

Comment 6

[Boris Zbarsky [:bzbarsky]]

You basically have two options for a scriptable getter here.

Option 1 is to use a method with an XPIDL array return value.  Something like:

  void getRedirectChain([optional] out unsigned long aLength,
                        [array, size_is(aLength), retval] out nsIURI aChain);

Your C++ signature will then be:

  NS_IMETHODIMP LoadInfo::GetRedirectChain(uint32_t* aLength, nsIURI*** aChain) {
    ...
  }

and you'd set *aLength, then malloc a buffer of size "sizeof(nsIURI*) * (*aLength)", cast it to nsIURI**, assign to *aChain, then walk through the buffer, treating at as nsIURI*[] and assign your nsIURI* values into it, addreffing as you go.  You can see this sort of thing in inDOMUtils::GetAllStyleSheets, for example.

Option 2 is to use a method or readonly attribute with a jsval return value:

  [implicit_jscontext]
  readonly attribute jsval redirectChain;

Your C++ signature will then be:

  NS_IMETHODIMP LoadInfo::GetRedirectChain(JSContext* aCx, JS::MutableHandle<JS::Value> aChain) {
    ...
  }

In this case you have to do the conversion to JS yourself, but I'm pretty sure this should work, once you include mozilla/dom/ToJSValue.h:

  if (!ToJSValue(aCx, mRedirectChain, aChain)) {
    return NS_ERROR_OUT_OF_MEMORY;
  }
  return NS_OK;

In either case, you probably want to use a binaryname here that maps to a C++ name like GetRedirectChainFromScript or something.  And you should have a noscript, nostdcall, notxpcom thing that just returns a "const nsTArray<nsCOMPtr<nsIURI>>&" for C++ consumers.  Then you won't need "don't modify this" comments and whatnot.

Comment 7

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirect_chain_in_loadinfo.patch

(In reply to Boris Zbarsky [:bz] from comment #6)
> Option 2 is to use a method or readonly attribute with a jsval return value:
>   [implicit_jscontext]
>   readonly attribute jsval redirectChain;
Yes, that's exactly what I want.
 
> In either case, you probably want to use a binaryname here that maps to a
> C++ name like GetRedirectChainFromScript or something.
And yes again, that's exactly what we should do.


@reviewers: I added some questions within Comment 3 which would be great if we can get answered - Thanks!

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 8625981 [details] [diff] [review]
bug_1175803_store_redirect_chain_in_loadinfo.patch

Review of attachment 8625981 [details] [diff] [review]:
-----------------------------------------------------------------

I only looked at the BackgroundUtils and IPDL files, I hope that's all you wanted me to look at here, let me know otherwise!

::: ipc/glue/BackgroundUtils.cpp
@@ +225,3 @@
>  {
> +  if (!aLoadInfo) {
> +    // if there is no loadInfo, then there is nothing to serialize

Here you should set aOptionalLoadInfoArgs to void_t

@@ +230,5 @@
>  
> +  // we need this dummy, otherwise the sanitycheck for mType fails
> +  // when do get_LoadInfoArgs() underneath().
> +  mozilla::net::LoadInfoArgs dummy;
> +  *aOptionalLoadInfoArgs = dummy;

Yeah, this isn't very elegant, but it can be made a little better (and safer) I think if you reverse the steps:

  PrincipalInfo requestingPrincipalInfo;
  rv = PrincipalToPrincipalInfo(aLoadInfo->LoadingPrincipal(),
                                &requestingPrincipalInfo);
  NS_ENSURE_SUCCESS(rv, rv);

  PrincipalInfo triggeringPrincipalInfo;
  rv = PrincipalToPrincipalInfo(aLoadInfo->TriggeringPrincipal(),
                                &triggeringPrincipalInfo);
  NS_ENSURE_SUCCESS(rv, rv);

  nsTArray<URIParams> redirectChain;
  for (const nsCOMPtr<nsIURI>& uri : aLoadInfo->RedirectChain()) {
    URIParams* params = redirectChain.AppendElement();
    SerializeURI(uri, params);
  }

  *aOptionalLoadInfoArgs =
    mozilla::net::LoadInfoArgs(
      requestingPrincipalInfo,
      triggeringPrincipalInfo,
      aLoadInfo->GetSecurityFlags(),
      aLoadInfo->GetContentPolicyType(),
      aLoadInfo->GetInnerWindowID(),
      aLoadInfo->GetOuterWindowID(),
      aLoadInfo->GetParentOuterWindowID(),
      redirectChain);

This pattern has the added benefit that when you add or remove members from the IPDL struct this code will stop compiling.

@@ +266,5 @@
> +    *outLoadInfo = nullptr;
> +    return NS_OK;
> +  }
> +
> +  mozilla::net::LoadInfoArgs loadInfoArgs = aOptionalLoadInfoArgs.get_LoadInfoArgs();

Make sure to make this a const ref:

  const mozilla::net::LoadInfoArgs& loadInfoArgs

@@ +278,5 @@
>    NS_ENSURE_SUCCESS(rv, rv);
>  
> +  nsTArray< nsCOMPtr<nsIURI> > redirectChain;
> +  for (uint32_t i = 0; i < loadInfoArgs.redirectChain().Length(); i++) {
> +    nsCOMPtr<nsIURI> redirectURI = DeserializeURI(loadInfoArgs.redirectChain()[i]);

This can be:

  for (const URIParams& uriParams : loadInfoArgs.redirectChain()) {
    nsCOMPtr<nsIURI> redirectURI = DeserializeURI(uriParams);
    // ...

Comment 9

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirect_chain_in_loadinfo.patch

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #8)
>   nsTArray<URIParams> redirectChain;
>   for (const nsCOMPtr<nsIURI>& uri : aLoadInfo->RedirectChain()) {
>     URIParams* params = redirectChain.AppendElement();
>     SerializeURI(uri, params);
>   }
> 
>   *aOptionalLoadInfoArgs =
>     mozilla::net::LoadInfoArgs(
>       requestingPrincipalInfo,
>       triggeringPrincipalInfo,
>       aLoadInfo->GetSecurityFlags(),
>       aLoadInfo->GetContentPolicyType(),
>       aLoadInfo->GetInnerWindowID(),
>       aLoadInfo->GetOuterWindowID(),
>       aLoadInfo->GetParentOuterWindowID(),
>       redirectChain);
> 
> This pattern has the added benefit that when you add or remove members from
> the IPDL struct this code will stop compiling.

Thanks Ben - exactly what I was looking for - carrying over initial r+ from bent.

Comment 10

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8627439 [details] [diff] [review]
bug_1175803_store_redirect_chain_in_loadinfo.patch

Still need Steve and finally Jonas to sign off on this patch.

Comment 11

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8627439 [details] [diff] [review]
bug_1175803_store_redirect_chain_in_loadinfo.patch

Review of attachment 8627439 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/BackgroundUtils.cpp
@@ +220,5 @@
>  }
>  
>  nsresult
>  LoadInfoToLoadInfoArgs(nsILoadInfo *aLoadInfo,
> +                       mozilla::net::OptionalLoadInfoArgs* aOptionalLoadInfoArgs)

Mind the 80-char limit.

::: ipc/glue/BackgroundUtils.h
@@ +68,5 @@
>   * Convert a LoadInfo to LoadInfoArgs struct.
>   */
>  nsresult
>  LoadInfoToLoadInfoArgs(nsILoadInfo *aLoadInfo,
> +                       mozilla::net::OptionalLoadInfoArgs* outOptionalLoadInfoArgs);

Same here

@@ +74,5 @@
>  /**
>   * Convert LoadInfoArgs to a LoadInfo.
>   */
>  nsresult
> +LoadInfoArgsToLoadInfo(const mozilla::net::OptionalLoadInfoArgs& aOptionalLoadInfoArgs,

And here.

::: netwerk/base/LoadInfo.cpp
@@ +232,5 @@
> +
> +NS_IMETHODIMP
> +LoadInfo::GetRedirectChain(JSContext* aCx, JS::MutableHandle<JS::Value> aChain)
> +{
> +  if (!ToJSValue(aCx, mRedirectChain, aChain)) {

Whoa! ToJSValue is cool!!

::: netwerk/base/LoadInfo.h
@@ +24,5 @@
>  
>  namespace ipc {
>  // we have to forward declare that function so we can use it as a friend.
>  nsresult
> +LoadInfoArgsToLoadInfo(const mozilla::net::OptionalLoadInfoArgs& aLoadInfoArgs,

And here

Comment 12

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Looks great!

Comment 13

[Steve Workman [:sworkman] (INACTIVE)]

Pat, if we add a URI to an array in LoadInfo in a redirect, should we remove it during a fallback? FYI, I don't know or understand much about the fallback code in nsHttpChannel - it looks like it's something to do with AppCache and I never really worked with that code. Can you shed some light on this?

Comment 14

[Patrick McManus [:mcmanus]]

honza is going to give you the best answer about fallback.

please note that HttpBaseChannel has a list of redirects already, in the form of an array of nsIPrincipal (mRedirects) and nsIRedirectHistory - is it possible to consolidate these things together?

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Can you put the "make loadInfoArgs optional" part in a separate patch so that we don't have to keep re-reviewing that?

Comment 16

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #15)
> Can you put the "make loadInfoArgs optional" part in a separate patch so
> that we don't have to keep re-reviewing that?

Yes, I filed a separate bug (Bug 1179505) and will split the patch.

Comment 17

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Patrick McManus [:mcmanus] from comment #14)
> honza is going to give you the best answer about fallback.
> 
> please note that HttpBaseChannel has a list of redirects already, in the
> form of an array of nsIPrincipal (mRedirects) and nsIRedirectHistory - is it
> possible to consolidate these things together?

Yes, that sounds reasonable, thanks for pointing that out. Still would need the answer about what to do in the fallback case.

Comment 18

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #0)
> Whenever a channel gets redirected we should store the channel uri within
> the loadInfo, so that at all times throughout the lifetime of a channel we
> can retrace all the redirects a channel went through.

As I understand there is LoadInfo <=> loading document (and its URI) 1-1 binding, right?  Hence the redirect chain you want is what the default (document) loading channel has gone through, yes?  The default load group's channel holds the redirect chain information (as Pat pointer out) but there is no access to it from LoadInfo AFAICS except some crazy jumps from windows to docs to docshell to load groups.  We might share (refcount) the redirect array between HttpBaseChannel and LoadInfo object and let the channel fill it (it has LoadInfo reference)

(In reply to Steve Workman [:sworkman] (please use needinfo) from comment #13)
> Pat, if we add a URI to an array in LoadInfo in a redirect, should we remove
> it during a fallback? FYI, I don't know or understand much about the
> fallback code in nsHttpChannel - it looks like it's something to do with
> AppCache and I never really worked with that code. Can you shed some light
> on this?

Yep.  Internally we consider this a redirect since we stop propagating any load error to the upper level and we do redirect to a different content set as a fallback for the failed URL (URL namespace, to be exact.)  However, in the address bar we show the original URI.  So, it's your call for your case here whether to consider fallback as a redirect or not.  But I personally would not special case it and rather add it to the list.

Does it answer the question?

Comment 19

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirectchain_within_loadinfo_part1.patch

Honza, thanks for answering our questions. Basically what we want is to move the redirectChain that Monica introduced within Bug 974018 into the LoadInfo. This first patch here just sets up the redirectChain within LoadInfo.

Comment 20

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirectchain_within_loadinfo_part2.patch

This second patch replaces nsIRedirectHistory completely. Nicely Monica added a testcase (test_redirect_history) which I slightly modified to query the redirectchain from the loadInfo. Still works as expected.

Also, we are using nsTArray internally but nsExternalHelperAppService internally uses an nsIArray. I am happy to update mRedirects to be an nsTarray as well, but I think we should leave it as an nsIArray.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8630119 [details] [diff] [review]
bug_1175803_store_redirectchain_within_loadinfo_part1.patch

Review of attachment 8630119 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/BackgroundUtils.cpp
@@ +245,5 @@
> +  PrincipalInfo redirectedPrincipal;
> +  for (const nsCOMPtr<nsIPrincipal>& principal : aLoadInfo->RedirectChain()) {
> +    rv = PrincipalToPrincipalInfo(principal, &redirectedPrincipal);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    redirectChain.AppendElement(redirectedPrincipal);

This is doing extra copying. I think you can simply do

rv = PrincipalToPrincipalInfo(principal, redirectChain.AppendElement());
NS_ENSURE_SUCCESS(rv, rv);

@@ +287,5 @@
> +  for (const PrincipalInfo& principalInfo : loadInfoArgs.redirectChain()) {
> +    nsCOMPtr<nsIPrincipal> redirectedPrincipal =
> +      PrincipalInfoToPrincipal(principalInfo, &rv);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    redirectChain.AppendElement(redirectedPrincipal);

Do

redirectChain.AppendElement(redirectedPrincipal.forget());

to save a couple of addref/releases.

Comment 22

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 8630120 [details] [diff] [review]
bug_1175803_store_redirectchain_within_loadinfo_part2.patch

Review of attachment 8630120 [details] [diff] [review]:
-----------------------------------------------------------------

Only concern I have is when exactly to add the redirect to the list. There's probably always going to be a risk that we add something to the redirect chain and then the redirect is cancelled, which will look odd when inspecting the pre-redirect nsIChannel. So I think that's something that we'll simply have to live with as long as the redirect chain lives on the loadinfo.

Comment 23

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Jonas Sicking (:sicking) from comment #22)
> Comment on attachment 8630120 [details] [diff] [review]
> bug_1175803_store_redirectchain_within_loadinfo_part2.patch
> 
> Review of attachment 8630120 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Only concern I have is when exactly to add the redirect to the list. There's
> probably always going to be a risk that we add something to the redirect
> chain and then the redirect is cancelled, which will look odd when
> inspecting the pre-redirect nsIChannel. So I think that's something that
> we'll simply have to live with as long as the redirect chain lives on the
> loadinfo.

Yes, agreed that's not that great. Monica had a comment in her patch which also pointed that out. I think we have to live with that - doesn't sound too bad to me actually.

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Just make sure to document if the new principal is added before or after nsIObserverService and nsIChannelEventSink notifications. I suspect that we'll want to add the principal before those are notified.

Comment 25

[Honza Bambas (:mayhemer)]

Comment on attachment 8630120 [details] [diff] [review]
bug_1175803_store_redirectchain_within_loadinfo_part2.patch

Review of attachment 8630120 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsBaseChannel.cpp
@@ +91,5 @@
> +  nsCOMPtr<nsIPrincipal> uriPrincipal;
> +  nsIScriptSecurityManager *sm =
> +    nsContentUtils::GetSecurityManager();
> +  sm->GetChannelURIPrincipal(this, getter_AddRefs(uriPrincipal));
> +  mLoadInfo->AppendRedirectedPrincipal(uriPrincipal);

I think here you have similar problem as in nsHttpChannel.  This is too soon..

::: netwerk/protocol/http/HttpBaseChannel.cpp
@@ +2256,5 @@
> +  // Add the initial uri of the channel to the redirectChain within the
> +  // loadInfo and set the loadInfo on the new channel. Please note,
> +  // that mLoadInfo of this object becomes unused. No need to create
> +  // a copy of the loadInfo.
> +  mLoadInfo->AppendRedirectedPrincipal(GetURIPrincipal());

not a good place.  we have a better one: http://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/netwerk/protocol/http/nsHttpChannel.cpp#l202

When 'succeeded' is true we are definitely about to redirect (it's a point of no return unless an internal error occurs)

::: uriloader/exthandler/nsExternalHelperAppService.cpp
@@ +1991,5 @@
> +      NS_ENSURE_SUCCESS(rv, rv);
> +      LOG(("nsExternalAppHandler: Got %u redirects\n", loadInfo->RedirectChain().Length()));
> +
> +      nsCOMPtr<nsIMutableArray> redirectChain;
> +      for (const nsCOMPtr<nsIPrincipal>& principal : loadInfo->RedirectChain()) {

must this go via nsCOMPtr<> ?

@@ +1992,5 @@
> +      LOG(("nsExternalAppHandler: Got %u redirects\n", loadInfo->RedirectChain().Length()));
> +
> +      nsCOMPtr<nsIMutableArray> redirectChain;
> +      for (const nsCOMPtr<nsIPrincipal>& principal : loadInfo->RedirectChain()) {
> +        redirectChain->AppendElement(principal, false);

you are missing creation of the nsIMutableArray instance ;)

Comment 26

[Honza Bambas (:mayhemer)]

Comment on attachment 8630119 [details] [diff] [review]
bug_1175803_store_redirectchain_within_loadinfo_part1.patch

Review of attachment 8630119 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/LoadInfo.cpp
@@ +98,5 @@
>  {
>    MOZ_ASSERT(mLoadingPrincipal);
>    MOZ_ASSERT(mTriggeringPrincipal);
> +
> +  mRedirectChain.SwapElements(aRedirectChain);

hmm.. are you sure about the swap here?  the caller may not count with it.  or this needs to be well documented or commented here there is a limited number of isolated consumers that do count with this.

::: netwerk/base/LoadInfo.h
@@ +61,5 @@
>             nsContentPolicyType aContentPolicyType,
>             uint64_t aInnerWindowID,
>             uint64_t aOuterWindowID,
> +           uint64_t aParentOuterWindowID,
> +           nsTArray<nsCOMPtr<nsIPrincipal>>& aRedirectChain);

> >

::: netwerk/base/nsILoadInfo.idl
@@ +11,5 @@
>  interface nsINode;
>  interface nsIPrincipal;
>  interface nsIURI;
>  
> +[ref] native constnsIPrincipalArray(const nsTArray<nsCOMPtr<nsIPrincipal>>);

nit: maybe const_nsIPrincipalArray ?

@@ +258,5 @@
> +  [implicit_jscontext]
> +  readonly attribute jsval redirectChain;
> +
> +  /**
> +   * A C++-friendly version of redirectChain.

please make a fat note here that it's a reference to the array that is held by the object, so it has the same lifetime!

Comment 27

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirectchain_within_loadinfo_part1.patch

Carrying over r+ from Jonas and Honza.

(In reply to Honza Bambas (:mayhemer) from comment #26)
> > +  mRedirectChain.SwapElements(aRedirectChain);
> 
> hmm.. are you sure about the swap here?  the caller may not count with it. 
> or this needs to be well documented or commented here there is a limited
> number of isolated consumers that do count with this.
>
I don't have a strong opinion whether to use swapElements with a nice comment on top or copy the elements over. Jonas, what do you prefer?

Comment 28

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirectchain_within_loadinfo_part2.patch

Thanks Honza for your help.

(In reply to Honza Bambas (:mayhemer) from comment #25)
> ::: netwerk/base/nsBaseChannel.cpp
> @@ +91,5 @@
> > +  nsCOMPtr<nsIPrincipal> uriPrincipal;
> > +  nsIScriptSecurityManager *sm =
> > +    nsContentUtils::GetSecurityManager();
> > +  sm->GetChannelURIPrincipal(this, getter_AddRefs(uriPrincipal));
> > +  mLoadInfo->AppendRedirectedPrincipal(uriPrincipal);
> 
> I think here you have similar problem as in nsHttpChannel.  This is too
> soon..

I am not that familiar with Necko code, what would be the best place to do that? Do we want it at all? What channels are we dealing here with? Maybe the httpchannel covers all the redirects we are interested in?

> ::: netwerk/protocol/http/HttpBaseChannel.cpp
> > +  mLoadInfo->AppendRedirectedPrincipal(GetURIPrincipal());
> 
> not a good place.  we have a better one:
> http://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/netwerk/protocol/
> http/nsHttpChannel.cpp#l202
> 
> When 'succeeded' is true we are definitely about to redirect (it's a point
> of no return unless an internal error occurs)

I added the redirect code there, but I am not sure if it would more sense to actually account for (or branch on) succeed. Please let me know how you would like to have that handled.
 
> > +      nsCOMPtr<nsIMutableArray> redirectChain;
> 
> you are missing creation of the nsIMutableArray instance ;)

Oh - that is not that crisp - thanks!

Comment 29

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #28)
> Created attachment 8630536 [details] [diff] [review]
> bug_1175803_store_redirectchain_within_loadinfo_part2.patch
> 
> Thanks Honza for your help.
> 
> (In reply to Honza Bambas (:mayhemer) from comment #25)
> > ::: netwerk/base/nsBaseChannel.cpp
> > @@ +91,5 @@
> > > +  nsCOMPtr<nsIPrincipal> uriPrincipal;
> > > +  nsIScriptSecurityManager *sm =
> > > +    nsContentUtils::GetSecurityManager();
> > > +  sm->GetChannelURIPrincipal(this, getter_AddRefs(uriPrincipal));
> > > +  mLoadInfo->AppendRedirectedPrincipal(uriPrincipal);
> > 
> > I think here you have similar problem as in nsHttpChannel.  This is too
> > soon..
> 
> I am not that familiar with Necko code, what would be the best place to do
> that? 

I was hoping you find this out yourself.

> Do we want it at all? 

I really don't know but assume yes.  Have to check.

> What channels are we dealing here with? 

This is a base class for many final channel implementations just check the code.

> Maybe
> the httpchannel covers all the redirects we are interested in?

Maybe.  I would so far nor bother with nsBaseChannel now, we can do it in a followup.

> 
> > ::: netwerk/protocol/http/HttpBaseChannel.cpp
> > > +  mLoadInfo->AppendRedirectedPrincipal(GetURIPrincipal());
> > 
> > not a good place.  we have a better one:
> > http://hg.mozilla.org/mozilla-central/annotate/e7e69cc8c07b/netwerk/protocol/
> > http/nsHttpChannel.cpp#l202
> > 
> > When 'succeeded' is true we are definitely about to redirect (it's a point
> > of no return unless an internal error occurs)
> 
> I added the redirect code there, but I am not sure if it would more sense to
> actually account for (or branch on) succeed. Please let me know how you
> would like to have that handled.

It's the whole point!  The 'succeeded' flag tells you we proceed with the redirect.  You know, redirects can be vetoed.  In that case you MUST NOT add the principal to the redirect array.  Hence the 'succeeded' flag use is mandatory.

Comment 30

[Honza Bambas (:mayhemer)]

Comment on attachment 8630536 [details] [diff] [review]
bug_1175803_store_redirectchain_within_loadinfo_part2.patch

Review of attachment 8630536 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +212,5 @@
> +    sm->GetChannelURIPrincipal(mChannel, getter_AddRefs(uriPrincipal));
> +    rv = loadInfo->AppendRedirectedPrincipal(uriPrincipal);
> +    if (NS_WARN_IF(NS_FAILED(rv))) {
> +      return;
> +    }

why don't you just:

mChannel->mLoadInfo->AppendRedirectedPrincipal(mChannel->GetURIPrincipal());

??

Comment 31

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Honza Bambas (:mayhemer) from comment #30)
> Comment on attachment 8630536 [details] [diff] [review]
> bug_1175803_store_redirectchain_within_loadinfo_part2.patch
> 
> Review of attachment 8630536 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: netwerk/protocol/http/nsHttpChannel.cpp
> @@ +212,5 @@
> > +    sm->GetChannelURIPrincipal(mChannel, getter_AddRefs(uriPrincipal));
> > +    rv = loadInfo->AppendRedirectedPrincipal(uriPrincipal);
> > +    if (NS_WARN_IF(NS_FAILED(rv))) {
> > +      return;
> > +    }
> 
> why don't you just:
> 
> mChannel->mLoadInfo->AppendRedirectedPrincipal(mChannel->GetURIPrincipal());
> 
> ??

It's not 100% certain that mChannel's mLoadInfo is not a nullptr. As for the second part, I agree, that is the nicer solution! I will incorporate that change!

But more importantly, what can we do about the redirect in nsbaseChannel?

Comment 32

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirectchain_within_loadinfo_part2.patch

Hey Honza, thanks for all your help. Agreed, lets worry about the nsBaseChannel case in a followup and get this code landed first. Agreed?

Comment 33

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #27)
> I don't have a strong opinion whether to use swapElements with a nice
> comment on top or copy the elements over. Jonas, what do you prefer?

I guess I don't feel strongly. It seems silly not to since all current callers are fine with it and it's faster.

So I'd recommend sticking a comment on the ctor. But if someone feels strongly that copying is better I can live with that.

Comment 34

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #31)
> (In reply to Honza Bambas (:mayhemer) from comment #30)
> > Comment on attachment 8630536 [details] [diff] [review]
> > bug_1175803_store_redirectchain_within_loadinfo_part2.patch
> > 
> > Review of attachment 8630536 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: netwerk/protocol/http/nsHttpChannel.cpp
> > @@ +212,5 @@
> > > +    sm->GetChannelURIPrincipal(mChannel, getter_AddRefs(uriPrincipal));
> > > +    rv = loadInfo->AppendRedirectedPrincipal(uriPrincipal);
> > > +    if (NS_WARN_IF(NS_FAILED(rv))) {
> > > +      return;
> > > +    }
> > 
> > why don't you just:
> > 
> > mChannel->mLoadInfo->AppendRedirectedPrincipal(mChannel->GetURIPrincipal());
> > 
> > ??
> 
> It's not 100% certain that mChannel's mLoadInfo is not a nullptr. 

It didn't appear in your previous version of the patch.  Why that time mLoadInfo was non-null and now (here) it may be null?

> As for the
> second part, I agree, that is the nicer solution! I will incorporate that
> change!
> 
> But more importantly, what can we do about the redirect in nsbaseChannel?

The Redirect() method is used to redirect when the virtual override of OpenContentStream returns a channel and not a stream.  OpenContentStream is where the specific derived channel behavior is started, where the data source is instantiated and given to nsBaseChannel to pump it to the channel consumer.  It can open either a stream or a new channel.  In the letter case it needs to "redirect" to that new channel.  But that redirect is an internal redirect that should not be inserted to the chain.

This BTW opens a new question: should internal redirects be put to load info as well?  I think not, since redirects are also proxy connection fail-overs and so on, ending up at the same URL.  Really not something you want to have in the chain.

Comment 35

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Honza Bambas (:mayhemer) from comment #34)
> It didn't appear in your previous version of the patch.  Why that time
> mLoadInfo was non-null and now (here) it may be null?

Agreed, that peace was error prone, the latest version however handles it correctly. Thanks for pointing that out.

> > As for the
> > second part, I agree, that is the nicer solution! I will incorporate that
> > change!
> > 
> > But more importantly, what can we do about the redirect in nsbaseChannel?
> 
> The Redirect() method is used to redirect when the virtual override of
> OpenContentStream returns a channel and not a stream.  OpenContentStream is
> where the specific derived channel behavior is started, where the data
> source is instantiated and given to nsBaseChannel to pump it to the channel
> consumer.  It can open either a stream or a new channel.  In the letter case
> it needs to "redirect" to that new channel.  But that redirect is an
> internal redirect that should not be inserted to the chain.
> 
> This BTW opens a new question: should internal redirects be put to load info
> as well?  I think not, since redirects are also proxy connection fail-overs
> and so on, ending up at the same URL.  Really not something you want to have
> in the chain.

Thanks. You are absolutely right, we do not want internal redirects to end up in the redirect chain, hence I agree with your previous comment, we should land this patch and file a follow up to handle nsBaseChannel. Do you agree?

Comment 36

[Christoph Kerschbaumer [:ckerschb]]

Oh, forgot to clear my needinfo!

Comment 37

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirectchain_within_loadinfo_part2.patch

Hey Honza, thanks for all your help in figuring out where we should append the principal to the redirectChain. I just discussed things in person with Jonas, and we agreed that we *do* want internal redirects end up in the loadInfo as well. Hence I think the right place to add the principal to the redirectchain is within nsBaseChannel::ContinueRedirect() shortly before we call asyncOpen on the redirected channel. Would that be an acceplate place?

Comment 38

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #37)
> Created attachment 8630780 [details] [diff] [review]
> bug_1175803_store_redirectchain_within_loadinfo_part2.patch
> 
> Hey Honza, thanks for all your help in figuring out where we should append
> the principal to the redirectChain. I just discussed things in person with
> Jonas, and we agreed that we *do* want internal redirects end up in the
> loadInfo as well. Hence I think the right place to add the principal to the
> redirectchain is within nsBaseChannel::ContinueRedirect() shortly before we
> call asyncOpen on the redirected channel. Would that be an acceplate place?

The exact place should be around [1].  

- When mOpenRedirectChannel is true you must add the current URL to the list before you call AsyncOpen.  But when it fails, you have to remove it again.  
- When mOpenRedirectChannel is false you simply add the current URL all the time.

[1] http://hg.mozilla.org/mozilla-central/annotate/9b902b7669ae/netwerk/base/nsBaseChannel.cpp#l153

Comment 39

[Honza Bambas (:mayhemer)]

Comment on attachment 8630780 [details] [diff] [review]
bug_1175803_store_redirectchain_within_loadinfo_part2.patch

Review of attachment 8630780 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsBaseChannel.cpp
@@ +153,5 @@
> +    nsCOMPtr<nsIPrincipal> uriPrincipal;
> +    nsIScriptSecurityManager *sm = nsContentUtils::GetSecurityManager();
> +    sm->GetChannelURIPrincipal(this, getter_AddRefs(uriPrincipal));
> +    loadInfo->AppendRedirectedPrincipal(uriPrincipal);
> +  }

According my previous suggestion you have to remove the principal when AsyncOpen bellow failed.


Best IMO would be to give the new channel a LoadInfo copy with the added principal and leave an intact LoadInfo on the "old" channel.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +201,5 @@
>  
> +    // Add the initial uri of the channel to the redirectChain within
> +    // the loadInfo of the new channel.
> +    if (succeeded && mChannel->mLoadInfo) {
> +        mChannel->mLoadInfo->AppendRedirectedPrincipal(mChannel->GetURIPrincipal());

I haven't realized one bad thing about this approach: it will be set after the channel has been successfully AsyncOpen'ed.  I'm not sure this is too late.  In the previous comment I've suggested (more logical to me) to add this info before AsyncOpen call, so that the channel and its observers can examine the redirect chain already, and remove the principal when AsyncOpen() returned with a failure.

Jonas, what do you think?

Comment 40

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirectchain_within_loadinfo_part2.patch

Hey Honza, once again thanks so much for your patience and help in figuring out where to add the principal to the redirectChain.

> I haven't realized one bad thing about this approach: it will be set after
> the channel has been successfully AsyncOpen'ed.  I'm not sure this is too
> late.  In the previous comment I've suggested (more logical to me) to add
> this info before AsyncOpen call, so that the channel and its observers can
> examine the redirect chain already, and remove the principal when
> AsyncOpen() returned with a failure.
> 
> Jonas, what do you think?

We have had a meeting with Jonas earlier today and decided it would be ideal to append the principal to the redirectChain once the redirectChannel opened successfully. In the nsHttpBaseChannel this is exactly where you suggested to put it. In the nsHttpChannel it seems there are several places where we have to add it. I added it everywhere where we call mRedirectChannel->AsyncOpen(). It's also shortly after we set the originalURI, so I think that makes sense.

The good thing about that approach is:
* we are consistent throughout the baseChannel and the httpchannel
* no need to copy the loadInfo, and also
* no need to modify/remove elements from the redirectChain in case the redirectchannel does not get opened successfully.

How do you feel about that approach?

Comment 41

[Christoph Kerschbaumer [:ckerschb]]

CC'ing francois just to make sure we are not missing anything obvious. He is maintaining application reputation which nsIRedirectHistory was implemented in the first place.

Comment 42

[Honza Bambas (:mayhemer)]

(In reply to Christoph Kerschbaumer [:ckerschb] from comment #40)
> Created attachment 8631271 [details] [diff] [review]
> bug_1175803_store_redirectchain_within_loadinfo_part2.patch
> 
> Hey Honza, once again thanks so much for your patience and help in figuring
> out where to add the principal to the redirectChain.
> 
> > I haven't realized one bad thing about this approach: it will be set after
> > the channel has been successfully AsyncOpen'ed.  I'm not sure this is too
> > late.  In the previous comment I've suggested (more logical to me) to add
> > this info before AsyncOpen call, so that the channel and its observers can
> > examine the redirect chain already, and remove the principal when
> > AsyncOpen() returned with a failure.
> > 
> > Jonas, what do you think?
> 
> We have had a meeting with Jonas earlier today and decided it would be ideal
> to append the principal to the redirectChain once the redirectChannel opened
> successfully. 

I.e. after successful AsyncOpen() call on the redirect channel, right?  Just to be sure ;) 

> In the nsHttpBaseChannel this is exactly where you suggested
> to put it. 

Yep, before/around the line at [1].

> In the nsHttpChannel it seems there are several places where we
> have to add it. 

Absolutely not.  If you are OK after AsyncOpen, then the place I've suggested (in AutoRedirectVetoNotifier::ReportRedirectResult when succeeded is true) is the one and only and most correct.

> I added it everywhere where we call
> mRedirectChannel->AsyncOpen(). It's also shortly after we set the
> originalURI, so I think that makes sense.
> 
> The good thing about that approach is:
> * we are consistent throughout the baseChannel and the httpchannel
> * no need to copy the loadInfo, and also
> * no need to modify/remove elements from the redirectChain in case the
> redirectchannel does not get opened successfully.
> 
> How do you feel about that approach?

Technically yes, just please fix the last nits according this comment.

[1] http://hg.mozilla.org/mozilla-central/annotate/9b902b7669ae/netwerk/base/nsBaseChannel.cpp#l159

Comment 43

[Honza Bambas (:mayhemer)]

Comment on attachment 8631271 [details] [diff] [review]
bug_1175803_store_redirectchain_within_loadinfo_part2.patch

Review of attachment 8631271 [details] [diff] [review]:
-----------------------------------------------------------------

r-, see previous comment

Comment 44

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirectchain_within_loadinfo_part2.patch

(In reply to Honza Bambas (:mayhemer) from comment #42)
> I.e. after successful AsyncOpen() call on the redirect channel, right?  Just
> to be sure ;) 

Hey Honza, that is correct. I rearranged the code yet again, but I think this is finally what we want. Thanks again for your help in figuring out where to append the uris after a redirect.

Comment 45

[Christoph Kerschbaumer [:ckerschb]]

Comment on attachment 8631738 [details] [diff] [review]
bug_1175803_store_redirectchain_within_loadinfo_part2.patch

Review of attachment 8631738 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/base/nsBaseChannel.cpp
@@ +156,5 @@
>      nsresult rv = mRedirectChannel->AsyncOpen(mListener, mListenerContext);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    // Append the initial uri of the channel to the redirectChain
> +    // after the channel got openend successfully.
> +    nsCOMPtr<nsILoadInfo> loadInfo = mRedirectChannel->GetLoadInfo();

Instead to query the loadInfo from mRedirectChannel we could also use mLoadInfo. I think the difference is only semantically important where using mLoadInfo is definitely faster. I think using mLoadInfo is nicer, but up to you. Please let me know.

Comment 46

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175803_store_redirectchain_within_loadinfo_part3.patch

When running test_redirect_history.js in the child [1] I realized that the loadInfo is not in sync between child and parent. I chatted with mrbkap in person and he suggested that it's probably easiest to just also append the principal to the redirectHistory in httpchannelchild. This patch accomplishes that, but I am still not sure what we do for the nsbaseChannel case. Ideally we would have the same loadInfo in the parent and in the child.

Honza, mrbkap how do you feel about that? What would you suggest?

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/test/unit_ipc/test_redirect_history_wrap.js#6

Comment 47

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8633790 [details] [diff] [review]
bug_1175803_store_redirectchain_within_loadinfo_part3.patch

Review of attachment 8633790 [details] [diff] [review]:
-----------------------------------------------------------------

This is the easiest way to do this. If we need to handle the more generic case, then we might have to have actors for the loadinfos so we can update the loadinfo in the parent and the child at the same time.

Comment 48

[Honza Bambas (:mayhemer)]

Comment on attachment 8633790 [details] [diff] [review]
bug_1175803_store_redirectchain_within_loadinfo_part3.patch

Review of attachment 8633790 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1241,5 @@
>    if (NS_FAILED(rv))
>      NS_WARNING("CompleteRedirectSetup failed, HttpChannelChild already open?");
>  
> +  if (mLoadInfo) {
> +    mLoadInfo->AppendRedirectedPrincipal(GetURIPrincipal());

just do this when NS_SUCCEEDED(rv)

Comment 49

[Christoph Kerschbaumer [:ckerschb]]

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/89aaf05db4abc6a2efd09ed899ffb1d28e4f8fd9
changeset:  89aaf05db4abc6a2efd09ed899ffb1d28e4f8fd9
user:       Christoph Kerschbaumer <[e-mail]>
date:       Sun Jul 19 19:11:03 2015 -0700
description:
Bug 1175803 - Store redirect chain within loadInfo - part 1 (r=sicking,mayhemer)

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/26120c7952694e4ed77f9d1adafb96abc88d02d0
changeset:  26120c7952694e4ed77f9d1adafb96abc88d02d0
user:       Christoph Kerschbaumer <[e-mail]>
date:       Sun Jul 19 19:43:09 2015 -0700
description:
Bug 1175803 - Store redirect chain within loadInfo - part 2 (r=sicking,mayhemer)

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/b52e8d0e9672631b9795e2b47b783320806c8a65
changeset:  b52e8d0e9672631b9795e2b47b783320806c8a65
user:       Christoph Kerschbaumer <[e-mail]>
date:       Sun Jul 19 19:11:38 2015 -0700
description:
Bug 1175803 - Store redirect chain within loadInfo - part 3 e10s (r=mrbkap,mayhemer)

Comment 50

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/89aaf05db4ab
https://hg.mozilla.org/mozilla-central/rev/26120c795269
https://hg.mozilla.org/mozilla-central/rev/b52e8d0e9672