776174 - e10s: cleanup IPC nsILoadContext code

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Jason Duell]

We have a *lot* of boilerplate cut and paste code for nsILoader support.

We can get rid of much of it by creating an IPC::LoadContext class that handles the marshalling of nsILoadContext fields. See bug 566799 for how we did this with IPC::URI (which was more complex than this IIRC).

We may be able to also centralize the Getter access methods in the various channelParent classes, but so far I've been stymied XPCOM inheritance issues when I've tried.

Comment 2

[Jason Duell]

Now more than a cleanup bug: we also need to call GetExtendedOrigin on parent rather than trusting value child sends us.

Comment 3

[Jason Duell]

Attachment: docshell changes v1

Not sure which docshell peer knows e10s stuff best.  Bsmedberg?

This patch creates two classes:

IPC::LoadContext: for serializing an nsILoadContent across IPDL.

LoadContextParent:  an object that lives in ParentChannels, captures the result from IPC::LoadContext during RecvAsyncOpen, and then acts as a nsILoadContext that parent channels can hand off during GetInterface.

Mostly this patch will let us get rid of a ton of duplicate code that's currently scattered across all the necko channels to do this same thing (next patch).

But there's also one substantive change: we need to call GetExtendedOrigin() on the parent (rather than trust whatever the child gives us: talk to cjones if you need convincing :)

The reason these new classes live in /docshell rather than /netwerk is that biesi was not happy about necko knowing about ScriptSecurityManager, which we need to do GetExtendedOrigin.  So we decided these might as well live next to nsILoadContext.idl.

Comment 4

[Jason Duell]

Attachment: necko changes v1

Comment 5

[Jason Duell]

Oh, in case it's not obvious: IPC:LoadContext needs to be a different class than LoadContextParent solely because the latter is an XPCOM object and needs to be refcounted, and IPDL doesn't do refcounting.  Seemed a Bad Idea to try to finesse that with hacks.

Comment 6

[Josh Matthews [:jdm]]

Comment on attachment 646105 [details] [diff] [review]
necko changes v1

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

This is super. Most of my comments are more to do with names chosen in the previous patch, so I'll make them there.

::: netwerk/protocol/http/HttpChannelParent.h
@@ +107,5 @@
>    bool mSentRedirect1Begin          : 1;
>    bool mSentRedirect1BeginFailed    : 1;
>    bool mReceivedRedirect2Verify     : 1;
>  
> +  nsCOMPtr<nsILoadContext> mLoadContext;

I think this will pack better if placed above the bools.

::: netwerk/protocol/wyciwyg/WyciwygChannelParent.h
@@ +44,5 @@
>    virtual void ActorDestroy(ActorDestroyReason why);
>  
>    nsCOMPtr<nsIWyciwygChannel> mChannel;
>    bool mIPCClosed;
> +  nsCOMPtr<nsILoadContext> mLoadContext;

This too should pack better above the bool.

Comment 7

[Josh Matthews [:jdm]]

Comment on attachment 646103 [details] [diff] [review]
docshell changes v1

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

::: docshell/base/IpcLoadContext.h
@@ +11,5 @@
> +#include "IPC/IPCMessageUtils.h"
> +#include "nsIIPCSerializable.h"
> +#include "nsIChannel.h"
> +#include "nsIWebSocketChannel.h"
> +#include "nsILoadContext.h"

I believe these three headers can be moved to the cpp file and replaced with forward declarations.

@@ +33,5 @@
> +  LoadContext(nsIWebSocketChannel *aChannel);
> +
> +  void Init(nsILoadContext *aLoadContext);
> +
> +  bool IsNull() const {

Given how this method is used in the other patch, I think IsNotNull would be clearer to read.

::: docshell/base/LoadContextParent.h
@@ +8,5 @@
> +#define LoadContextParent_h
> +
> +#include "IpcLoadContext.h"
> +
> +class LoadContextParent : public nsILoadContext

The Parent suffix had me thinking we were creating a full-on IPC actor pair. Can we use something like (but not necessarily) LoadContextStub instead?

Comment 8

[Benjamin Smedberg]

Comment on attachment 646103 [details] [diff] [review]
docshell changes v1

I'm going on PTO starting tomorrow, and I think smaug would be a better person to review anyway.

Comment 9

[Jason Duell]

> The Parent suffix had me thinking we were creating a full-on IPC actor pair. Can we use something like (but not necessarily) LoadContextStub instead?

Yeah I thought of that too.  I'll try to think of a better name. "Stub" ain't great either :)

> replaced with forward declarations.

I actually started doing this and then realized these aren't IDL .h files, and it would save very little compilation time IIUC.  I can do it if you want...

> IsNotNull

OK, sure.

Comment 10

[Jason Duell]

> The Parent suffix had me thinking we were creating a full-on IPC actor pair.

I still like 'Parent' here because it's clear about where it lives.  I think people will figure out quickly enough that it's not an IPDL actor, and is just used by ParentChannel classes.

LoadContextStub is the only other name I can think of offhand. LoadContextImpersonator? There's already a (very different) "shim" loadcontext class elsewhere in the tree.   Maybe ollie has an opinion...

Comment 11

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

Do we actually need extendedOrigin on nsILoadContext? It seems like appcache doesn't need it, and I would expect that the cookie code won't be able to use it either.

Let me know if I'm wrong.

Comment 12

[Jason Duell]

Cookies don't need extendedOrigin.   I can remove the field from nsILoadContext if you think it needs removing.  I'd like to keep the rest of this patch as it cleans up the code considerably.   We could also put LoadChannelParent in /necko if we remove GetExtendedOrigin (though I suspect we may want to keep it in /docshell in case any other reasons pop up that require us to grab ScriptSecurityManager or some other thing that necko shouldn't/doesn't know about).

So for now I guess I'd like to just +r and land this patch then do a followup to remove GetExtendedOrigin.

Comment 13

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

Sounds good.

Comment 14

[Jason Duell]

Now that extendedOrigin will go away (in followup per current plan), this isn't a blocker.  Would still be nice to land...

Comment 15

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

Comment on attachment 646103 [details] [diff] [review]
docshell changes v1


Just some nits.

>+  LoadContext(nsIChannel *aChannel);
>+  LoadContext(nsIWebSocketChannel *aChannel);
>+
>+  void Init(nsILoadContext *aLoadContext);
* goes with type.

>+
>+  bool IsNull() const {
{ to next line

>+    return mIsNull;
>+  }

>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>+  {
>+    if (!ReadParam(aMsg, aIter, &aResult->mIsNull) ||
>+        !ReadParam(aMsg, aIter, &aResult->mIsContent)  ||
>+        !ReadParam(aMsg, aIter, &aResult->mUsePrivateBrowsing)  ||
>+        !ReadParam(aMsg, aIter, &aResult->mAppId)  ||
>+        !ReadParam(aMsg, aIter, &aResult->mIsInBrowserElement))
>+      return false;
if (expr) {
  stmt;
}



>+// ParentChannels should never hand this class out during GetInterface if IsNull
>+#define ABORT_IF_NULL                                                          \
>+  if (mIsNull) {                                                               \
>+    NS_RUNTIMEABORT("LoadContextParent handed out but mIsNull!");              \
>+    return NS_ERROR_UNEXPECTED;                                                \
>+  }
Runtime abort *and* returning error code?

>+LoadContextParent::GetIsContent(bool *aIsContent)
bool* aIsContent



>+LoadContextParent::GetExtendedOrigin(nsIURI *aUri, nsACString &aResult)
nsIURI* aURI, nsACString& aResult

>+{
>+  ABORT_IF_NULL
>+
>+  nsCOMPtr<nsIScriptSecurityManager> ssmgr =
>+    do_GetService(NS_SCRIPTSECURITYMANAGER_CONTRACTID);
nsIScriptSecurityManager* ssmgr = nsContentUtils::GetSecurityManager();

>+  NS_ENSURE_TRUE(ssmgr, false);
Certainly you don't want to return false from a method which return nsresult.




>diff --git a/docshell/base/LoadContextParent.h b/docshell/base/LoadContextParent.h
>new file mode 100644
>--- /dev/null
>+++ b/docshell/base/LoadContextParent.h
>@@ -0,0 +1,35 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set sw=2 ts=8 et tw=80 : */
>+/* This Source Code Form is subject to the terms of the Mozilla Public
>+ * License, v. 2.0. If a copy of the MPL was not distributed with this
>+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>+
>+#ifndef LoadContextParent_h
>+#define LoadContextParent_h
>+
>+#include "IpcLoadContext.h"
>+
>+class LoadContextParent : public nsILoadContext
So this class has nothing to do with ipdl. *Parent is then somewhat odd naming.


>+{
>+public:
>+  NS_DECL_ISUPPORTS
>+  NS_DECL_NSILOADCONTEXT
>+
>+  LoadContextParent(const IPC::LoadContext &toCopy)
const IPC::LoadContext& aToCopy


Hmm, so you have
class LoadContext which doesn't inherit nsILoadContext, and
LoadContextParent which does inherit nsILoadContext.
This is very confusing.

Could you rename LoadContext to IpcLoadContext
(I know it is a bit silly since it is in IPC namespace, but at least the name is clear that it is about ipc)
Could you rename LoadContextParent to LoadContext and put it to mozilla namespace.

Comment 16

[Josh Matthews [:jdm]]

>Could you rename LoadContext to IpcLoadContext

Another possibility is SerializedLoadContext.

Comment 17

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

Yeah, that is perhaps better.

Comment 18

[Jason Duell]

Attachment: docshell changes v2

nits fixed, classes renamed.

Comment 19

[Jason Duell]

Attachment: necko changes v2

jdm's nits addressed and rebased to docshell patch.  Forwarding +r

Comment 20

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

Comment on attachment 649038 [details] [diff] [review]
docshell changes v2


>+// ParentChannels should never hand this class out during GetInterface if
>+// IsNotNull is false (i.e there was no LoadContext on Child channel)
>+#define ABORT_IF_NULL                                                          \
>+  if (!mIsNotNull) {                                                           \
>+    NS_RUNTIMEABORT("LoadContext handed out but !mIsNotNull!");                \
>+  }
Would it be ok to just use MOZ_ASSERT(mIsNotNull);


>diff --git a/netwerk/base/public/nsNetUtil.h b/netwerk/base/public/nsNetUtil.h
>--- a/netwerk/base/public/nsNetUtil.h
>+++ b/netwerk/base/public/nsNetUtil.h
I suppose these changes shouldn't be in this patch. At least I should not review them.
But for the other parts r=me

Comment 21

[Jason Duell]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5a6ac05db368

> Would it be ok to just use MOZ_ASSERT(mIsNotNull);

Yup--done.

>+++ b/netwerk/base/public/nsNetUtil.h

Sorry for the confusion: jdm already reviewed those bits. I moved them to the docshell patch only because they're needed to compile and I was debugging.

Comment 22

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/5a6ac05db368

Comment 23

[Landry Breuil (:gaston)]

Minor annoyance.. that changeset now yields :

netwerk/protocol/ftp/FTPChannelParent.cpp:47: error: extra ';'
netwerk/protocol/http/NullHttpTransaction.cpp:15: error: extra ';'
netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp:44: error: extra ';'
netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp:21: error: extra ';'

Patch fixing those extra ; in a few.

Comment 24

[Landry Breuil (:gaston)]

Attachment: Followup, remove extra semicolons after macros

Remove those 4 extra semicolons, fixes the build under netwerk/ for me.

Comment 25

[Jason Duell]

Comment on attachment 649782 [details] [diff] [review]
Followup, remove extra semicolons after macros

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

Thanks for noticing Landry!

  https://hg.mozilla.org/integration/mozilla-inbound/rev/8cb978beef29

Comment 26

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/8cb978beef29