e10s HTTP: Improve how URIs are serialized

RESOLVED FIXED

Status

()

RESOLVED FIXED
9 years ago
8 years ago

People

(Reporter: jduell, Assigned: jdm)

Tracking

Other Branch
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

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

9 years ago
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?
> 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

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

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

9 years ago
(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?
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

9 years ago
Wonderful. Will patch soon.
Assignee: nobody → dwitte
Status: NEW → ASSIGNED

Comment 9

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

9 years ago
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.
HTTP is nsStandardURL, not nsSimpleURI.

Comment 12

9 years ago
Er, yeah, I meant nsStandardURL. :/
(Assignee)

Comment 13

9 years ago
Yoink.
Assignee: dwitte → josh
(Reporter)

Updated

9 years ago
Blocks: 558617
(Reporter)

Updated

9 years ago
No longer blocks: 516730
(Assignee)

Comment 14

9 years ago
Created attachment 439211 [details] [diff] [review]
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
Attachment #439211 - Flags: review?(dwitte)
(Assignee)

Comment 15

9 years ago
Created attachment 439213 [details] [diff] [review]
Part 2 of 2: Convert PHttpChannel to use URIs

Nothing too extraordinary here.
Attachment #439213 - Flags: review?(dwitte)

Comment 16

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

9 years ago
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
Attachment #439213 - Flags: review?(dwitte) → review+

Comment 18

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

Comment 19

9 years ago
(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.
> 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.
(Reporter)

Comment 21

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

Comment 22

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

8 years ago
(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

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

Comment 25

8 years ago
Created attachment 439913 [details] [diff] [review]
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.
Attachment #439211 - Attachment is obsolete: true
Attachment #439913 - Flags: review?(dwitte)
Attachment #439211 - Flags: review?(dwitte)
(Assignee)

Comment 26

8 years ago
Created attachment 439914 [details] [diff] [review]
Part 2 of 3: Convert PHttpChannel to use URIs

Nits scratched.
Attachment #439213 - Attachment is obsolete: true
(Assignee)

Comment 27

8 years ago
Created attachment 439915 [details] [diff] [review]
Part 3 of 3: Convert cookie service

Just because you asked so nicely.
Attachment #439915 - Flags: review?(dwitte)

Comment 28

8 years ago
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
Attachment #439913 - Flags: review?(dwitte) → review+

Comment 29

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

r=dwitte
Attachment #439914 - Flags: review+

Comment 30

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

r=dwitte
Attachment #439915 - Flags: review?(dwitte) → review+
(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

8 years ago
I stand corrected!
(Assignee)

Comment 33

8 years ago
Created attachment 441475 [details] [diff] [review]
Part 1 of 3: IPDL goop for serializing URIs

Comments addressed.  Let's push this sucker.
Attachment #439913 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
Whiteboard: [land in e10s]
Attachment #441475 - Flags: review+
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [land in e10s]
(Assignee)

Comment 35

8 years ago
Created attachment 441542 [details] [diff] [review]
Part 4 of 3: Build fix

mq let me down once more and allowed me to submit typos.
You need to log in before you can comment on or make changes to this bug.