Closed
Bug 776174
Opened 12 years ago
Closed 12 years ago
e10s: cleanup IPC nsILoadContext code
Categories
(Core :: Networking, defect)
Core
Networking
Tracking
()
People
(Reporter: jduell.mcbugs, Assigned: jduell.mcbugs)
References
Details
Attachments
(3 files, 2 obsolete files)
11.85 KB,
patch
|
smaug
:
review+
|
Details | Diff | Splinter Review |
47.78 KB,
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
3.49 KB,
patch
|
jduell.mcbugs
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
Summary: e10s: cleanup IPC nsILoader code → e10s: cleanup IPC nsILoadContext code
Assignee | ||
Comment 2•12 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•12 years ago
|
||
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•12 years ago
|
||
Attachment #646105 -
Flags: review?(josh)
Assignee | ||
Comment 5•12 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•12 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•12 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•12 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•12 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•12 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•12 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•12 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 15•12 years ago
|
||
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-
Comment 16•12 years ago
|
||
>Could you rename LoadContext to IpcLoadContext
Another possibility is SerializedLoadContext.
Comment 17•12 years ago
|
||
Yeah, that is perhaps better.
Assignee | ||
Comment 18•12 years ago
|
||
nits fixed, classes renamed.
Attachment #646103 -
Attachment is obsolete: true
Attachment #649038 -
Flags: review?(bugs)
Assignee | ||
Comment 19•12 years ago
|
||
jdm's nits addressed and rebased to docshell patch. Forwarding +r
Attachment #646105 -
Attachment is obsolete: true
Attachment #649039 -
Flags: review+
Comment 20•12 years ago
|
||
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•12 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.
Comment 22•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5a6ac05db368
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Comment 23•12 years ago
|
||
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•12 years ago
|
||
Remove those 4 extra semicolons, fixes the build under netwerk/ for me.
Attachment #649782 -
Flags: review?(jduell.mcbugs)
Assignee | ||
Comment 25•12 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+
Assignee | ||
Updated•12 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.
Description
•