1175352 - Refactor LoadInfo arguments to be more self contained

Status: RESOLVED FIXED

(Core :: DOM: Security, defect)

Description

[Christoph Kerschbaumer [:ckerschb]]

As Jonas pointed out in
> https://bugzilla.mozilla.org/show_bug.cgi?id=1139297#c85
we should refactor loadinfo arguments to be more self contained.

Comment 1

[Christoph Kerschbaumer [:ckerschb]]

Ben, Jonas indicated you have some suggestions on how we could improve on the loadInfo arguments? I am happy to hear them and incorporate them. Thanks!

Comment 2

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

Either an IPDL struct or a C-style struct with a custom serializer should be fine. If you need to incorporate this struct into other IPDL uniions then you should do the former, otherwise I don't think it matters too much which route you choose.

Comment 3

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

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #2)
> IPDL uniions

Should have said "IPDL unions or IPDL structs".

Comment 4

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175352_refactor_loadinfo_args.patch

Hey Ben, as discussed in person yesterday, I think this is what we want, right? Or am I missing something?

Comment 5

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

Comment on attachment 8624521 [details] [diff] [review]
bug_1175352_refactor_loadinfo_args.patch

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

Looks fine!

::: ipc/glue/BackgroundUtils.cpp
@@ +13,5 @@
>  #include "nsNetUtil.h"
>  #include "nsNullPrincipal.h"
>  #include "nsServiceManagerUtils.h"
>  #include "nsString.h"
> +#include "mozilla/net/NeckoChannelParams.h"

Nit: Alphabetize

@@ +228,5 @@
> +  if (aLoadInfo) {
> +    rv = PrincipalToPrincipalInfo(aLoadInfo->LoadingPrincipal(),
> +                                  &requestingPrincipalInfo);
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    aLoadInfoArgs->requestingPrincipalInfo() = requestingPrincipalInfo;

Could just do:

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

That would avoid the temporaries here, and below.

@@ +231,5 @@
> +    NS_ENSURE_SUCCESS(rv, rv);
> +    aLoadInfoArgs->requestingPrincipalInfo() = requestingPrincipalInfo;
> +
> +    PrincipalToPrincipalInfo(aLoadInfo->TriggeringPrincipal(),
> +                             &triggeringPrincipalInfo);

Check for failure

::: netwerk/protocol/ftp/FTPChannelChild.cpp
@@ +196,3 @@
>    nsCOMPtr<nsILoadInfo> loadInfo;
>    GetLoadInfo(getter_AddRefs(loadInfo));
> +  mozilla::ipc::LoadInfoToLoadInfoArgs(loadInfo, &loadInfoArgs);

Check for failure?

@@ +196,4 @@
>    nsCOMPtr<nsILoadInfo> loadInfo;
>    GetLoadInfo(getter_AddRefs(loadInfo));
> +  mozilla::ipc::LoadInfoToLoadInfoArgs(loadInfo, &loadInfoArgs);
> +  openArgs.loadInfo() = loadInfoArgs;

|LoadInfoToLoadInfoArgs(loadInfo, &openArgs.loadInfo())| to avoid the temporary.

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +1678,5 @@
>    }
>    openArgs.cacheKey() = cacheKey;
>  
> +  LoadInfoArgs loadInfoArgs;
> +  mozilla::ipc::LoadInfoToLoadInfoArgs(mLoadInfo, &loadInfoArgs);

Check for failure?

@@ +1679,5 @@
>    openArgs.cacheKey() = cacheKey;
>  
> +  LoadInfoArgs loadInfoArgs;
> +  mozilla::ipc::LoadInfoToLoadInfoArgs(mLoadInfo, &loadInfoArgs);
> +  openArgs.loadInfo() = loadInfoArgs;

You can avoid the temporary

::: netwerk/protocol/websocket/WebSocketChannelChild.cpp
@@ +483,5 @@
>    // Corresponding release in DeallocPWebSocket
>    AddIPDLReference();
>  
> +  LoadInfoArgs loadInfoArgs;
> +  LoadInfoToLoadInfoArgs(mLoadInfo, &loadInfoArgs);

Check for failure

Comment 6

[Christoph Kerschbaumer [:ckerschb]]

Attachment: bug_1175352_refactor_loadinfo_args.patch

Thanks Ben for the speedy review. Incorporated all of your nits; carrying over r+ from bent.

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f25d12a330d6

Comment 8

[Ryan VanderMeulen [:RyanVM]]

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

Comment 9

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

Comment on attachment 8624579 [details] [diff] [review]
bug_1175352_refactor_loadinfo_args.patch

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

::: ipc/glue/BackgroundUtils.cpp
@@ +239,5 @@
> +  }
> +
> +  // use default values if no loadInfo is provided
> +  rv = PrincipalToPrincipalInfo(nsContentUtils::GetSystemPrincipal(),
> +                                &aLoadInfoArgs->requestingPrincipalInfo());

This does not seem wise.

Why not serialize to something that deserializes to null on the other side?

I.e. something that makes LoadInfoArgsToLoadInfo return null rather than a LoadInfo object.

Comment 10

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

(In reply to Jonas Sicking (:sicking) from comment #9)
> I.e. something that makes LoadInfoArgsToLoadInfo return null rather than a
> LoadInfo object.

This is easy to do, you just make an IPDL union with void_t.

Comment 11

[Christoph Kerschbaumer [:ckerschb]]

(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #10)
> (In reply to Jonas Sicking (:sicking) from comment #9)
> > I.e. something that makes LoadInfoArgsToLoadInfo return null rather than a
> > LoadInfo object.
> 
> This is easy to do, you just make an IPDL union with void_t.

I will get that fixed. I am in the process of adding the redirect chain to the loadInfo (Bug 1175803) where I am touching BackgroundUtils.cpp anyways. So I think it makes sense that we get that fixed with that patch. Setting dependency here.