541174 - e10s HTTP: Improve how URIs are serialized

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Jason Duell]

This is a good first bug for e10s necko.

Right now we're sending URIs across IPDL by sending a (spec, charset) tuple.  From talking with the beezees (bz and biesi :) it sounds like this is in fact the right data to be sending.  But it would be preferable if we could teach IPDL how to serialize nsURI's via ParamTraits, so we could simply pass the nsURI object.  See

   https://developer.mozilla.org/en/IPDL/Type_Serialization

for how to do this.  

The code that would need to be touched in is /netwerk/protocol/http/src; PHttpChannel.ipdl and HttpChannel{Parent/Child}.cpp

Comment 1

[Benjamin Smedberg]

ugh, do we *need* to send a (spec, charset) tuple, instead of just eagerly converting to UTF8?

Serialization of types must be essentially infallible. Unless we can guarantee that nsIURI serialization never fails, I don't think we should use ParamTraits serialization of nsIURI directly: perhaps just have helper functions to do the necessary string conversion?

Comment 2

[Boris Zbarsky [:bzbarsky]]

> Unless we can guarantee that nsIURI serialization never fails

GetSpec will never fail modulo OOM, unless someone implements some sort of bogus nsIURI impl in JS....

That said, the expensive part is _deserialization_.  Right now we're going through and reparsing the URIs, etc.  This isn't cheap.

Comment 3

[[e-mail]]

Could we implement a [noscript] method on nsIURI to return a UUID for the implementation it happens to be? It wouldn't be pretty, but it would allow us to fastpath things like nsStandardURL, and just directly construct one and write into its members on the deserialization end.

It doesn't sound pretty; can you think of a nicer alternative?

(I'm not sure how much we can optimize regular NS_NewURI creation, but maybe we can get some wins there to make it not matter...)

Comment 4

[[e-mail]]

(In reply to comment #3)
> Could we implement a [noscript] method on nsIURI

(Obviously it'd have to be on a new interface, or in fact, just the success of QI'ing to that interface -- nsIStandardURLImpl? -- would suffice.)

Comment 5

[Boris Zbarsky [:bzbarsky]]

> and just directly construct one and write into its members on the
> deserialization end

You mean like using the existing nsISerializable implementation on URIs?

Comment 6

[[e-mail]]

(In reply to comment #5)
> You mean like using the existing nsISerializable implementation on URIs?

Oh. Yeah. :)

I'll roll a patch. Do we need a fallback to NS_NewURI, or can we require that all nsIURI implementations implement nsISerializable/nsIClassInfo? We could abort the child if this doesn't hold.

Any other things I should know before patching?

Comment 7

[Boris Zbarsky [:bzbarsky]]

You can require all URIs to implement this, I think.  All our internal URI impls certainly do so.

You don't need to worry about nsIClassInfo yourself.  As long as you have an object input stream or object output stream, using readObject/writeCompoundObject on them will do the right thing in terms of classinfo, createInstance, etc.  That's preferable to manual QI to nsISerializable and manual object creation, obviously.

Not sure there's anything else to know offhand.

Comment 8

[[e-mail]]

Wonderful. Will patch soon.

Comment 9

[Benjamin Smedberg]

nsISerializable is a lot of XPCOM goop. If we're going to do this kind of thing, can we use a purpose-built interfaces that works directly on Message* and skips all the XPCOM goop? Currently the IPC code assumes that parameters are always serializable and deserialization is a completely fatal error.

Comment 10

[[e-mail]]

Some URI implementations include nested URIs and nsIPrincipals. We're only really worried about remoting http URIs at the moment (nsSimpleURL); so the URI implementations that need something fancier can just runtime abort, for now.

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

HTTP is nsStandardURL, not nsSimpleURI.

Comment 12

[[e-mail]]

Er, yeah, I meant nsStandardURL. :/

Comment 13

[Josh Matthews [:jdm]]

Yoink.

Comment 14

[Josh Matthews [:jdm]]

Attachment: Part 1 of 2: IPDL goop for serializing URIs

Things to note:

* LOG in nsStandardURL.cpp has to be renamed because the chromium code defines its own LOG macro first.
* I suspect my nsURI is the least intrusive way to give IPDL an object to pass around, but I'm open to suggestions

Comment 15

[Josh Matthews [:jdm]]

Attachment: Part 2 of 2: Convert PHttpChannel to use URIs

Nothing too extraordinary here.

Comment 16

[[e-mail]]

Comment on attachment 439211 [details] [diff] [review]
Part 1 of 2: IPDL goop for serializing URIs

diff --git a/netwerk/base/public/nsIIPCSerializable.idl b/netwerk/base/public/nsIIPCSerializable.idl

>+[ptr] native ConstMessage(const IPC::Message);
>+[ptr] native Message(IPC::Message);
>+[ptr] native Iterator(void*);
>+
>+[uuid(1f605ac7-666b-471f-9864-1a21a95f11c4)]
>+interface nsIIPCSerializable : nsISupports

Want to mark this entire interface as [noscript].

>+  void read(in ConstMessage msg, in Iterator iter);
>+  void write(in Message msg);

Also mark these [notxpcom], and then you can have sensible prototypes:

  [notxpcom] bool read(in ConstMessage msg, in Iterator iter);
  [notxpcom] void write(in Message msg);

You may have to tweak the argument types to add *'s where appropriate.

diff --git a/netwerk/base/src/nsStandardURL.cpp b/netwerk/base/src/nsStandardURL.cpp

>-#define LOG(args)     PR_LOG(gStandardURLLog, PR_LOG_DEBUG, args)
>-#define LOG_ENABLED() PR_LOG_TEST(gStandardURLLog, PR_LOG_DEBUG)
>+#define LOG_(args)     PR_LOG(gStandardURLLog, PR_LOG_DEBUG, args)
>+#define LOG_ENABLED()  PR_LOG_TEST(gStandardURLLog, PR_LOG_DEBUG)

Gah, can you just #undef LOG and #define LOG ... ?

>+#ifdef MOZ_IPC
>+bool
>+nsStandardURL::ReadSegment(const IPC::Message *aMsg, void **aIter, URLSegment &seg)
>+{
>+    return (IPC::ReadParam(aMsg, aIter, &seg.mPos) &&
>+            IPC::ReadParam(aMsg, aIter, (PRUint32 *) &seg.mLen));
>+}
>+
>+void
>+nsStandardURL::WriteSegment(IPC::Message *aMsg, const URLSegment &seg)
>+{
>+    IPC::WriteParam(aMsg, seg.mPos);
>+    IPC::WriteParam(aMsg, PRUint32(seg.mLen));
>+}

PRInt32 seg.mLen?

>+NS_IMETHODIMP
>+nsStandardURL::Read(const IPC::Message *aMsg, void **aIter)
>+{

Signature can be less XPCOMish with the [notxpcom] IDL suggestion, and just 'return false' on failure.

>+    NS_PRECONDITION(!mHostA, "Shouldn't have cached ASCII host");
>+    NS_PRECONDITION(mSpecEncoding == eEncoding_Unknown,
>+                    "Shouldn't have spec encoding here");

Can also assert !mFile here.

>+    if (ReadParam(aMsg, aIter, &mPort) &&
>+        ReadParam(aMsg, aIter, &mDefaultPort) &&
>+        ReadParam(aMsg, aIter, &mSpec) &&
[...]

I'd rather see these in the form

  if (!ReadParam(...) ||
      !ReadParam(...) ||
    [...]
    return false;

>+        mHostEncoding = hostEncoding;
>+        mMutable = isMutable;
>+        mSupportsFileURL = supportsFileURL;
>+        return NS_OK;

Would like to see a comment here, a la nsStandardURL::Write(nsIObjectOutputStream *stream):

  // mFile, mSpecEncoding and mHostA are just caches that can be recovered as needed.

>diff --git a/netwerk/ipc/NeckoMessageUtils.h b/netwerk/ipc/NeckoMessageUtils.h

>+class nsURI {

Hmm. How about we call it IPC::URI or somesuch? nsURI sounds a bit antiquated.

>+  // The contained URI is already addrefed on creation. We don't want another
>+  // addref when passing it off to its actual owner.
>+  operator nsCOMPtr<nsIURI>() const { return already_AddRefed<nsIURI>(uri); }

Hmm. I think it'd be a bit cleaner to write this as:

  operator already_AddRefed<nsIURI>() const { return already_AddRefed<nsIURI>(uri); }

which the compiler should be able to implicitly convert through when calling the nsCOMPtr constructor or operator=. If not, what you have is fine.

>+  operator nsIURI*() const { return uri; }

I also don't think you want this -- it leaves us too open to trouble with the compiler implicitly converting URIs to nsCOMPtrs and such. Which will then leak.

In fact, I think we should lock down this class by not providing this method at all. Its sole purpose should be to allow creation of a URI from an nsIURI, and then get that back into an nsCOMPtr on the other side.

So, let's require that everyone go through the already_AddRefed operator, declare a private 'operator=', don't declare a default constructor, and make ParamTraits a friend class so that it can access mURI directly.

>+ private:
>+  nsIURI* uri;

mURI?

>+    static void Write(Message* aMsg, const paramType& aParam)
>+    {
>+      bool isNull = !aParam;

Then you'd have '!aParam.mURI' here.

>+      nsCID* cid;
>+      classInfo->GetClassID(&cid);

This leaks. I think you want:

  nsCID cid;
  classInfo->GetClassIDNoAlloc(&cid);

And since this is serialization code, you'd better check the return value of that call, just in case.

>+    static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>+    {
>+      bool isNull;
>+      if (!ReadParam(aMsg, aIter, &isNull))
>+        return false;
>+      if (isNull) {
>+        *aResult = nsnull;

And *aResult.mURI = nsnull here.

>+        return true;
>+      }
>+    
>+      nsCString cidStr;

nsCAutoString here.

>+      nsCID cid;
>+      if (!ReadParam(aMsg, aIter, &cidStr) ||
>+          !cid.Parse(cidStr.get()))
>+        return false;
>+
>+      nsresult rv;
>+      nsCOMPtr<nsIURI> uri = do_CreateInstance(cid, &rv);
>+      if (NS_FAILED(rv) || !uri)
>+        return false;

'if (!uri)' is sufficient here; no need for the 'rv' form of do_CI.

>+      *aResult = uri.forget().get();

uri.forget(&aResult->mURI);

>+      return true;
>+    }
>+
>+    static void Log(const paramType& aParam, std::wstring* aLog)
>+    {
>+      nsIURI* uri = aParam;
>+      if (uri) {
>+        nsCString spec, charset;

nsCAutoStrings here. Also I think it's OK to just log the spec.

Comment 17

[[e-mail]]

Comment on attachment 439213 [details] [diff] [review]
Part 2 of 2: Convert PHttpChannel to use URIs

>+HttpChannelParent::RecvAsyncOpen(const nsURI&               aURI,
[...]
>+  nsCString uriSpec, charset;
>+  uri->GetSpec(uriSpec);
>+  uri->GetOriginCharset(charset);
>   LOG(("HttpChannelParent RecvAsyncOpen [this=%x uri=%s (%s)]\n", 
>        this, uriSpec.get(), charset.get()));

nsCAutoStrings here, and no need for the charset I think.

>diff --git a/netwerk/protocol/http/src/PHttpChannel.ipdl b/netwerk/protocol/http/src/PHttpChannel.ipdl
>+  AsyncOpen(nsURI               uri,
>             // - TODO: unclear if any HTTP channel clients ever set
>             // originalURI != uri (about:credits?); also not clear if chrome
>             // channel would ever need to know.  Can we get rid of next two
>             // args?
>-            nsCString           originalUriSpec, 
>-            nsCString           originalCharset, 

'next arg', now.

Very nice! r=dwitte

Comment 18

[[e-mail]]

Also, PCookieService is dying for the same treatment. :)

Comment 19

[[e-mail]]

(In reply to comment #16)
> (From update of attachment 439211 [details] [diff] [review])
> >+bool
> >+nsStandardURL::ReadSegment(const IPC::Message *aMsg, void **aIter, URLSegment &seg)
> >+{
> >+    return (IPC::ReadParam(aMsg, aIter, &seg.mPos) &&
> >+            IPC::ReadParam(aMsg, aIter, (PRUint32 *) &seg.mLen));
> >+}
> >+
> >+void
> >+nsStandardURL::WriteSegment(IPC::Message *aMsg, const URLSegment &seg)
> >+{
> >+    IPC::WriteParam(aMsg, seg.mPos);
> >+    IPC::WriteParam(aMsg, PRUint32(seg.mLen));
> >+}
> 
> PRInt32 seg.mLen?

Whoops, what I meant here was: why the PRUint32 casts? ParamTraits should work fine with PRInt32's.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> nsCAutoStrings here, and no need for the charset I think.

Note that if you're always looking at nsStandardURL here then GetSpec will never use the auto buffer of an auto-string.  So might not make sense to use one here.

Comment 21

[Jason Duell]

You probably can #undef chromium's LOG, but carefully.  See bug 545995 and/or nsHttp.h (which has slightly more complex logic).

Comment 22

[Josh Matthews [:jdm]]

> And since this is serialization code, you'd better check the return value of
> that call, just in case.

The problem here is that Write() shouldn't fail.  Is there a CID representation guaranteed to be invalid that I could write out if the real one is inaccessible?  If not, I guess I'll just add another bool parameter to indicate whether there's actually a CID coming down the wire or not.

Comment 23

[[e-mail]]

(In reply to comment #22)
> The problem here is that Write() shouldn't fail.  Is there a CID representation
> guaranteed to be invalid that I could write out if the real one is
> inaccessible?  If not, I guess I'll just add another bool parameter to indicate
> whether there's actually a CID coming down the wire or not.

I think it's OK -- if you don't write those bits to the stream, then Read will fail on the other side. Which will result in process killage.

Comment 24

[[e-mail]]

(You could also explicitly ABORT_IF_FALSE(NS_SUCCEEDED(rv)), similar to what you do a few lines up. Which would be more explicit...)

Comment 25

[Josh Matthews [:jdm]]

Attachment: Part 1 of 3: IPDL goop for serializing URIs

All comments addressed.  The default constructor for IPC::URI stays, because IPDL requires it.  Something broke when trying to use |operator already_AddRefed| (I forget what) so I'm sticking with the nsCOMPtr conversion.  Everything else should be by the book.

Comment 26

[Josh Matthews [:jdm]]

Attachment: Part 2 of 3: Convert PHttpChannel to use URIs

Nits scratched.

Comment 27

[Josh Matthews [:jdm]]

Attachment: Part 3 of 3: Convert cookie service

Just because you asked so nicely.

Comment 28

[[e-mail]]

Comment on attachment 439913 [details] [diff] [review]
Part 1 of 3: IPDL goop for serializing URIs

>diff --git a/netwerk/base/src/nsStandardURL.cpp b/netwerk/base/src/nsStandardURL.cpp

>+PRBool
>+nsStandardURL::Read(const IPC::Message *aMsg, void **aIter)

>+    NS_PRECONDITION(!mFile, "Should have cached file");

"Shouldn't".

Also, I just realized all these are preconditions. I think we just want asserts here. Precondition will happen in release builds and is a bit strong.

>diff --git a/netwerk/ipc/NeckoMessageUtils.h b/netwerk/ipc/NeckoMessageUtils.h

We have netwerk/protocol/http/src/PHttpChannelParams.h for http-related serializers. We should probably just move that into your common file sometime, but cleanup can wait.

>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)

>+    nsCOMPtr<nsIURI> uri = do_CreateInstance(cid);
>+    if (!uri)
>+      return false;
>+    nsCOMPtr<nsIIPCSerializable> serializable = do_QueryInterface(uri);

You should check that 'serializable' is nonnull. If Read() returns false in the child, it's terminated with prejudice; in the parent, the child is terminated and the message is ignored. Whereas when you dereference it a couple lines down, you'll take down the parent. Prefer the former behavior :)

Nice work. r=dwitte

Comment 29

[[e-mail]]

Comment on attachment 439914 [details] [diff] [review]
Part 2 of 3: Convert PHttpChannel to use URIs

r=dwitte

Comment 30

[[e-mail]]

Comment on attachment 439915 [details] [diff] [review]
Part 3 of 3: Convert cookie service

r=dwitte

Comment 31

[Shawn Wilsher :sdwilsh]

(In reply to comment #28)
> Also, I just realized all these are preconditions. I think we just want asserts
> here. Precondition will happen in release builds and is a bit strong.
NS_PRECONDITION does nothing in release builds:
http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsDebug.h#175

Comment 32

[[e-mail]]

I stand corrected!

Comment 33

[Josh Matthews [:jdm]]

Attachment: Part 1 of 3: IPDL goop for serializing URIs

Comments addressed.  Let's push this sucker.

Comment 34

[Frederic Plourde (:fred23)]

Feels good to be pushing those guys!  Nice work Josh!

Pushed : 
http://hg.mozilla.org/projects/electrolysis/rev/7b2da3a79e35

Pushed :
http://hg.mozilla.org/projects/electrolysis/rev/cc46a878de23

Pushed :
http://hg.mozilla.org/projects/electrolysis/rev/a912b6efbb98

Comment 35

[Josh Matthews [:jdm]]

Attachment: Part 4 of 3: Build fix

mq let me down once more and allowed me to submit typos.

Comment 36

[Frederic Plourde (:fred23)]

Pushed http://hg.mozilla.org/projects/electrolysis/rev/6bf63a6ac301