e10s: cleanup IPC nsILoadContext code

RESOLVED FIXED in mozilla17

Status

()

Core
Networking
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jduell, Assigned: jduell)

Tracking

unspecified
mozilla17
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking-basecamp:-)

Details

Attachments

(3 attachments, 2 obsolete attachments)

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.

Updated

5 years ago
Summary: e10s: cleanup IPC nsILoader code → e10s: cleanup IPC nsILoadContext code
(Assignee)

Updated

5 years ago
Duplicate of this bug: 777419
(Assignee)

Comment 2

5 years ago
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.
(Assignee)

Comment 3

5 years ago
Created attachment 646103 [details] [diff] [review]
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.
Assignee: nobody → jduell.mcbugs
Status: NEW → ASSIGNED
Attachment #646103 - Flags: review?(benjamin)
(Assignee)

Comment 4

5 years ago
Created attachment 646105 [details] [diff] [review]
necko changes v1
Attachment #646105 - Flags: review?(josh)
(Assignee)

Comment 5

5 years ago
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

5 years ago
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 7

5 years ago
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

5 years ago
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)
(Assignee)

Comment 9

5 years ago
> 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.
(Assignee)

Comment 10

5 years ago
> 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: ? → -
(Assignee)

Comment 12

5 years ago
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.
Sounds good.
(Assignee)

Comment 14

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

Another possibility is SerializedLoadContext.
Yeah, that is perhaps better.
(Assignee)

Comment 18

5 years ago
Created attachment 649038 [details] [diff] [review]
docshell changes v2

nits fixed, classes renamed.
Attachment #646103 - Attachment is obsolete: true
Attachment #649038 - Flags: review?(bugs)
(Assignee)

Comment 19

5 years ago
Created attachment 649039 [details] [diff] [review]
necko changes v2

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+
(Assignee)

Comment 21

5 years ago
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.
(Assignee)

Updated

5 years ago
Blocks: 780818
https://hg.mozilla.org/mozilla-central/rev/5a6ac05db368
Status: ASSIGNED → RESOLVED
Last Resolved: 5 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.
Created attachment 649782 [details] [diff] [review]
Followup, remove extra semicolons after macros

Remove those 4 extra semicolons, fixes the build under netwerk/ for me.
Attachment #649782 - Flags: review?(jduell.mcbugs)
(Assignee)

Comment 25

5 years ago
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
Attachment #649782 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/8cb978beef29
(Assignee)

Updated

5 years ago
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.