Closed Bug 776174 Opened 7 years ago Closed 7 years ago

e10s: cleanup IPC nsILoadContext code


(Core :: Networking, defect)

Not set



blocking-basecamp -


(Reporter: jduell.mcbugs, Assigned: jduell.mcbugs)




(3 files, 2 obsolete files)

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.
Summary: e10s: cleanup IPC nsILoader code → e10s: cleanup IPC nsILoadContext code
Duplicate of this bug: 777419
Now more than a cleanup bug: we also need to call GetExtendedOrigin on parent rather than trusting value child sends us.
Blocks: 776652
blocking-basecamp: --- → ?
blocking-kilimanjaro: --- → ?
OS: Linux → All
Hardware: x86_64 → All
Summary: e10s: cleanup IPC nsILoadContext code → e10s: make sure GetExtendedOrigin for necko channels is gotten from parent, not child.
Attached patch docshell changes v1 (obsolete) — Splinter Review
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.
Assignee: nobody → jduell.mcbugs
Attachment #646103 - Flags: review?(benjamin)
Attached patch necko changes v1 (obsolete) — Splinter Review
Attachment #646105 - Flags: review?(josh)
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 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.
Attachment #646105 - Flags: review?(josh) → review+
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 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.
Attachment #646103 - Flags: review?(benjamin) → review?(bugs)
> 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.
> 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...
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.
blocking-basecamp: ? → -
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.
Now that extendedOrigin will go away (in followup per current plan), this isn't a blocker.  Would still be nice to land...
blocking-kilimanjaro: ? → ---
Summary: e10s: make sure GetExtendedOrigin for necko channels is gotten from parent, not child. → e10s: cleanup IPC nsILoader code
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) {

>+// 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

>+  nsCOMPtr<nsIScriptSecurityManager> ssmgr =
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 */
>+#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.

>+  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.
Attachment #646103 - Flags: review?(bugs) → review-
>Could you rename LoadContext to IpcLoadContext

Another possibility is SerializedLoadContext.
Yeah, that is perhaps better.
nits fixed, classes renamed.
Attachment #646103 - Attachment is obsolete: true
Attachment #649038 - Flags: review?(bugs)
Attached patch necko changes v2Splinter Review
jdm's nits addressed and rebased to docshell patch.  Forwarding +r
Attachment #646105 - Attachment is obsolete: true
Attachment #649039 - Flags: review+
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
Attachment #649038 - Flags: review?(bugs) → review+

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


>+++ 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.
Blocks: 780818
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
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.
Remove those 4 extra semicolons, fixes the build under netwerk/ for me.
Attachment #649782 - Flags: review?(jduell.mcbugs)
Comment on attachment 649782 [details] [diff] [review]
Followup, remove extra semicolons after macros

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

Thanks for noticing Landry!
Attachment #649782 - Flags: review?(jduell.mcbugs) → review+
Summary: e10s: cleanup IPC nsILoader code → e10s: cleanup IPC nsILoadContext code
You need to log in before you can comment on or make changes to this bug.