Refactor LoadInfo arguments to be more self contained

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: ckerschb, Assigned: ckerschb)

Tracking

(Blocks 1 bug)

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

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.
Assignee: nobody → mozilla
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!
Flags: needinfo?(bent.mozilla)
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.
Flags: needinfo?(bent.mozilla)
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #2)
> IPDL uniions

Should have said "IPDL unions or IPDL structs".
Hey Ben, as discussed in person yesterday, I think this is what we want, right? Or am I missing something?
Attachment #8624521 - Flags: review?(bent.mozilla)
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
Attachment #8624521 - Flags: review?(bent.mozilla) → review+
Thanks Ben for the speedy review. Incorporated all of your nits; carrying over r+ from bent.
Attachment #8624521 - Attachment is obsolete: true
Attachment #8624579 - Flags: review+
Target Milestone: --- → mozilla41
https://hg.mozilla.org/mozilla-central/rev/f25d12a330d6
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
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.
(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.
(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.
Blocks: 1175803
You need to log in before you can comment on or make changes to this bug.