Jason Duell Reporter
	Description • 15 years ago

Another fairly easy, self-contained necko e10s project.

Right now HttpChannelChild::SetRequestHeader is unimplemented.  We need to implement it so that it both sets headers on the tab channel (in case later code looks them up) and sends them during AsyncOpen and sets them on the chrome channel.

I was initially thinking we'd try to copy over the entire mRequestHead during AsyncOpen, but there's actually a lot of stuff in it (URI, HTTP version, boilerplate headers) that will already be set in the chrome channel after Init.  So I think the way to go now is just to keep an array of any headers set with SetRequestHeader and send that along in AsyncOpen.

I strongly suspect that no necko clients call SetRequestHeader after AsyncOpen, but there's currently nothing prohibiting it.  I'd like to DROP_DEAD for now if this is tried--I don't want to try to sync the headers in the channels after AsyncOpen, and this is a pretty nonsensical thing to do.  If we determine this isn't happening, we can remove the DROP_DEAD and add ASSERTs (or some other non-fatal warning) to both HttpChannelChild and nsHttpChannel.

	Jason Duell Reporter
	Comment 1 • 15 years ago

Note: see HttpChannelChild::SetDocumentURI() for the type of state checking I'm talking about here.

	Jason Duell Reporter
	Comment 2 • 15 years ago

Another part of this bug is to uncomment (or move into a separate, shared function--see comments) the logic to set standard HTTP headers in HttpChannelChild::Init

	Jae-Seong Lee-Russo Assignee
	Comment 3 • 15 years ago

Jason, I appreciate any feedback on this incomplete patch.  Having a bit of hard time understanding the description of this bug as I have few experience in Necko, IPDL, etc.

	dwitte@gmail.com
	Comment 4 • 15 years ago

Comment on attachment 424995 [details] [diff] [review]
wip

I'll jump in and say that this looks like it's along the right lines. 

>diff --git a/netwerk/protocol/http/src/HttpChannelChild.cpp b/netwerk/protocol/http/src/HttpChannelChild.cpp

> HttpChannelChild::SetRequestHeader(const nsACString& aHeader, 
>                                    const nsACString& aValue, 
>                                    PRBool aMerge)
> {
>-  DROP_DEAD();
>+  // TODO: for now detect call after AsyncOpen by dying
>+  // -- remove before code enters production, obviously
>+  if (mIsPending)
>+    DROP_DEAD();
>+  if (mWasOpened)
>+    DROP_DEAD();
>+
>+  if (!mHeaders.Contains(aHeader))
>+    mHeaders.AppendElement(aHeader);

You should respect the 'aMerge' parameter here - have a look at nsHttpChannel.cpp to see what it does. You also need to store 'aValue'. I'd recommend doing

struct HeaderValuePair {
  nsCString mHeader;
  nsCString mValue;
};

nsTArray<HeaderValuePair> mHeaders;

You'll also need to write an IPDL serializer for the array. See https://developer.mozilla.org/en/IPDL/Type_Serialization for details.

>+/**
>+ * COPIED from nsHttpChannel.cpp
>+ */

Don't need any of this, since it'll be done in nsHttpChannel. You just need to write the logic in HttpChannelParent to call SetRequestHeader for each entry in the array.

Let me know if you'd like help with this!

	Jason Duell Reporter
	Comment 5 • 15 years ago

Yes, I agree with Dan's plan to save (HeaderName, value) pairs, send them during AsyncOpen, and have the HttpChannelParent call SetRequestHeader for each header during RecvAsyncOpen.  But I think we'll need to save (headerName, value, merge), in order to get the merge semantics right.

>+  // TODO: for now detect call after AsyncOpen by dying
>+  // -- remove before code enters production, obviously
>+  if (mIsPending)
>+    DROP_DEAD();
>+  if (mWasOpened)
>+    DROP_DEAD();

There's now ENSURE_CALLED_BEFORE_ASYNC_OPEN(), which you should call instead of copying these checks.  Call it at the start of HttpChannelChild::SetRequestHeader.

	Jae-Seong Lee-Russo Assignee
	Comment 6 • 15 years ago

How does this look?

	dwitte@gmail.com
	Comment 7 • 15 years ago

Comment on attachment 427713 [details] [diff] [review]
patch

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

>@@ -0,0 +1,50 @@
>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
>+/* vim: set sw=2 ts=8 et tw=80 ft=cpp : */

Need a license header here.

>diff --git a/netwerk/protocol/http/src/HttpChannelChild.cpp b/netwerk/protocol/http/src/HttpChannelChild.cpp

> NS_IMETHODIMP
> HttpChannelChild::SetRequestHeader(const nsACString& aHeader, 
>                                    const nsACString& aValue, 
>                                    PRBool aMerge)
> {
>-  DROP_DEAD();
>+  ENSURE_CALLED_BEFORE_ASYNC_OPEN();
>+
>+  const nsCString &flatHeader = PromiseFlatCString(aHeader);
>+  const nsCString &flatValue  = PromiseFlatCString(aValue);
>+
>+  struct HeaderValuePair pair = { flatHeader, flatValue, aMerge };
>+  mHeaders.AppendElement(pair);

You can avoid all the string copies here like so:

  HeaderValuePair* pair = mHeaders.AppendElement();
  if (!pair)
    return NS_ERROR_OUT_OF_MEMORY;

  pair->mHeader = aHeader;
  pair->mValue = aValue;
  pair->mMerge = aMerge;

>diff --git a/netwerk/protocol/http/src/HttpChannelParent.cpp b/netwerk/protocol/http/src/HttpChannelParent.cpp

>+  nsCOMPtr<nsIHttpChannel> httpChan(do_QueryInterface(chan));
>+  PRUint32 length = headers.Length();
>+  for (PRUint32 i = 0; i < length; i++)

Can just fold the 'headers.Length()' into the 'for' line here.

Very nice. The serializer bits will need review from 2 people; probably jduell and cjones. Split that out into a separate patch for ease of review?

We'll need a story for GetRequestHeader() in the child, but that's a different bug.

(Also, next time you request review, make sure the address field gets set - probably want :jduell.)

	Jason Duell Reporter
	Comment 9 • 15 years ago

Comment on attachment 428407 [details] [diff] [review]
serializer for 536279 & 536283

Jae:

This is good stuff.  We've quite close to something that can be checked in.

Regarding reviews:  I just checked with cjones and bsmedberg, and this can go in as one patch.  Also, it doesn't need superreview.  I'd like to have 2 reviews, though (me and dwitte, since he knows the patch well by now).  

So go ahead and make the fixes I suggest below, file a new patch, and flag it for review by me and dwitte.

>diff --git a/netwerk/ipc/NeckoMessageUtils.h b/netwerk/ipc/NeckoMessageUtils.h
>new file mode 100644

You've used DOS-style carriage returns here: please use Unix-style ones.  See

  https://developer.mozilla.org/en/C___Portability_Guide#Don%27t_put_carriage_returns_in_XP_code

The solution is ideally to configure your editor to use Unix-style linebreaks.  Or you can use "dos2unix file.cpp", a Linux utility to convert to unix linebreaks.  (If you're on a windows box you can probably get a version of this on cygwin.  But you can also probably tell your editor to use Unix line breaks).

>+struct HeaderValuePair {
>+  nsCString mHeader;
>+  nsCString mValue;
>+  PRBool    mMerge;
>+};

>+typedef nsTArray<HeaderValuePair> HeaderValuePairs;

We won't be using the same struct for the response (we'll be serializing the entire nsHttpResponseHead instead).  So let's call these "RequestHeaderTuple" and "RequestHeaderTuples".  It's not a great name, but it's better than calling something a "Pair" when it has 3 values :)  


>diff --git a/netwerk/protocol/http/src/HttpChannelChild.cpp b/netwerk/protocol/http/src/HttpChannelChild.cpp
> NS_IMETHODIMP
> HttpChannelChild::SetRequestHeader(const nsACString& aHeader, 
>                                    const nsACString& aValue, 
>                                    PRBool aMerge)
> {
>-  DROP_DEAD();
>+  ENSURE_CALLED_BEFORE_ASYNC_OPEN();

So, we need to have the security checks (for valid header name and no carriage returns in value) that nsHttpChannel::SetRequestHeader has.  We want those errors to be synchronous (i.e. immediate), rather than waiting until we try to call SetRequestHeader on the chrome nsHttpChannel and discover that the user has passed an invalid header name, etc.   

Also, as Dan mentioned,  we want to make sure that if some later code in the child calls GetRequestHeader() it sees the value we're setting.  

I think the best way to solve both of these issues is to move nsHttpChannel's SetRequestHeader() into our new base class (HttpBaseChannel--see bug 546581), and then call HttpBaseChannel::SetRequestHeader() right after ENSURE_CALLED_BEFORE_ASYNC_OPEN.  If it fails, you should return right away with the error. 

Since we haven't landed HttpBaseChannel yet, why don't you just copy-and-paste nsHttpChannel::SetRequestHeader() into HttpChannelChild with some name like "BaseClassSetRequestHeader_HACK", and call it from HttpChannelChild::SetRequestHeader, and file a new bug to remove it once bug 546581 lands (make the bug dependent on
546581, obviously).

TEST:

I'd like to see a test for this code as part of the patch.  You can add one to unit/test_simple.js.  I've actually coded one for you (am I a helpful reviewer, or what? :).  I'll attach it and you can roll it into your patch.

	Jason Duell Reporter
	Comment 10 • 15 years ago

Here's the test. Go ahead and roll this into your new patch.

Oh, and run the test before you submit your final patch :)  Right now the SetRequestHeader works with your previous patch (the httpd handler function sees "Foo" in the ServerHandler() function), but the call to GetRequestHeader() fails (since we've defined it to DROP_DEAD()).  We should also move nsHttpChannel::GetRequestHeader into the base class, and that will solve this problem (in the meantime you can copy the implementation into HttpChannelChild, and mark it to be removed in the same followup bug I mentioned in my last comment).

	Jason Duell Reporter
	Comment 11 • 15 years ago

Oops--here's the full version of the test.  The previous version didn't fail because it didn't call GetRequestHeader().

	Jae-Seong Lee-Russo Assignee
	Comment 12 • 15 years ago

I had to comment this line: http://mxr.mozilla.org/projects-central/source/electrolysis/toolkit/crashreporter/google-breakpad/src/client/windows/handler/exception_handler.cc#730 because of the assert error.  Without the assert, the test ran fine.

	dwitte@gmail.com
	Comment 13 • 15 years ago

Comment on attachment 428717 [details] [diff] [review]
patch for 536279 & 536283

>diff --git a/netwerk/base/src/Makefile.in b/netwerk/base/src/Makefile.in

>+ifdef MOZ_IPC
>+EXPORTS_NAMESPACES = mozilla/net
>+
>+EXPORTS_mozilla/net = \
>+  nsURLHelper.h \
>+  $(NULL)
>+endif

What's this bit for?

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

>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */

Nit: tab-width should be 2 here.

>+ * The Initial Developer of the Original Code is
>+ *  The Mozilla Foundation
>+ * Portions created by the Initial Developer are Copyright (C) 2009
>+ * the Initial Developer. All Rights Reserved.
>+ *
>+ * Contributor(s):
>+ *   Jason Duell <jduell.mcbugs@gmail.com>

Date should be 2010, and you should be the contributor. :)

>+template<>
>+struct ParamTraits<nsHttpResponseHead>
>+{
>+  typedef nsHttpResponseHead paramType;
>+
>+  static void Write(Message* aMsg, const paramType& aParam)
>+  {
>+    paramType p = const_cast<paramType&>(aParam);
>+    nsCString response;
>+    p.Flatten(response, PR_FALSE);

Hmm. I don't think we want to do this. Flatten() only serializes a portion of the nsHttpResponseHead, which could lead to unexpected behaviors depending on what the child does with it. And if someone adds code to the child that depends on an extra field being serialized, it will be totally non-obvious why the child behaves differently. And it's unclear, without reading the code very, very closely, whether Flatten() and Parse() are robust to OOM and cannot result in an array bounds read.

I think you'll need to serialize all the fields at http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttpResponseHead.h#142. Which means writing a serializer for nsHttpHeaderArray (trivial; IPDL already knows how to serialize an nsTArray), nsEntry (trivial), and nsHttpAtom. (The latter will require using nsHttp::ResolveAtom() in Read().)

Sorry for the extra work. :(

>diff --git a/netwerk/protocol/http/src/HttpChannelChild.cpp b/netwerk/protocol/http/src/HttpChannelChild.cpp

> HttpChannelChild::~HttpChannelChild()
> {
>   LOG(("Destroying HttpChannelChild @%x\n", this));
> 
>   // release our reference to the handler
>   NS_RELEASE(gHttpHandler);
>+
>+  mHeaders.Clear();
>+
>+  delete mResponseHead;

Make mResponseHead an nsAutoPtr<> and then you don't need to delete it. (Or init it to null either, which should've been done in the constructor.)

>+HttpChannelChild::RecvOnStartRequest(const nsHttpResponseHead& responseHead)
> {
>   LOG(("HttpChannelChild::RecvOnStartRequest [this=%x]\n", this));
> 
>   mState = HCC_ONSTART;
> 
>-  mContentLength_HACK = HACK_ContentLength;
>-  mContentType_HACK = HACK_ContentType;
>-  mResponseStatus_HACK = HACK_Status;
>-  mResponseStatusText_HACK = HACK_StatusText;
>+  mResponseHead = const_cast<nsHttpResponseHead*>(&responseHead);

Can't do this - IPDL instantiates an nsHttpResponseHead on the stack before calling RecvOnStartRequest(), and it'll be destroyed once this function ends.

You'll need to copy the entire nsHttpResponseHead structure. The compiler-generated copy constructor for that class (and all its members) will work fine, so you can just do

mResponseHead = new nsHttpResponseHead(responseHead);

This needs tests.

> NS_IMETHODIMP
> HttpChannelChild::GetContentType(nsACString& value)
> {
>-  if (mState < HCC_ONSTART) {
>+  /**
>+   * COPIED from nsHttpChannel.cpp: move to shared base class?
>+   */
>+  if (!mResponseHead) {
>+    // We got no data, we got no headers, we got nothing
>     value.Truncate();
>     return NS_ERROR_NOT_AVAILABLE;
>   }

Not sure if jduell wants to start using mState for these things, or use the traditional nsHttpChannel checks. I think it'd be clearer to eventually do the former, but we have to be careful not to change behavior.

(Of course, it also means we have to handle things like OOM failures when they occur, and DROP_DEAD() in the child. cjones informs me that we are designing for infallible malloc in e10s, so for things like 'new foo' we can remove those DROP_DEAD()'s later.)

> NS_IMETHODIMP
> HttpChannelChild::SetRequestHeader(const nsACString& aHeader, 
>                                    const nsACString& aValue, 
>                                    PRBool aMerge)
> {
>-  DROP_DEAD();
>+  ENSURE_CALLED_BEFORE_ASYNC_OPEN();
>+
>+  nsresult rv = BaseClassSetRequestHeader_HACK(aHeader, aValue, aMerge);
>+  if (NS_FAILED(rv))
>+    return rv;

Since you're now keeping mRequestHead and mHeaders consistent, you should set the boilerplate headers on mRequestHead in Init() also. You should remove the #if 0'd code in Init() and copy over the relevant code from nsHttpChannel::Init().

If you want, you can punt on it and I can fix this in bug 546581, since I'll be factoring Init() into the base class anyway.

	dwitte@gmail.com
	Comment 14 • 15 years ago

(In reply to comment #13)

> > HttpChannelChild::~HttpChannelChild()
> > {
> >   LOG(("Destroying HttpChannelChild @%x\n", this));
> > 
> >   // release our reference to the handler
> >   NS_RELEASE(gHttpHandler);
> >+
> >+  mHeaders.Clear();
> >+
> >+  delete mResponseHead;
> 
> Make mResponseHead an nsAutoPtr<> and then you don't need to delete it. (Or
> init it to null either, which should've been done in the constructor.)

Also, don't need the 'mHeaders.Clear()'; the nsTArray's dtor will do this for you.

	Jae-Seong Lee-Russo Assignee
	Comment 15 • 15 years ago

> >+ifdef MOZ_IPC
> >+EXPORTS_NAMESPACES = mozilla/net
> >+
> >+EXPORTS_mozilla/net = \
> >+  nsURLHelper.h \
> >+  $(NULL)
> >+endif
> 
> What's this bit for?
nsHttp.h includes nsURLHelper.h.

I will try Init() part maybe in the next patch attempt.

	dwitte@gmail.com
	Comment 16 • 15 years ago

Comment on attachment 428918 [details] [diff] [review]
patch for 536279 & 536283, 2

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

>+template<>
>+struct ParamTraits<const char*>
>+{

Guh. We don't want to do this. Instead:

>+template<>
>+struct ParamTraits<nsHttpAtom>
>+{
>+  typedef nsHttpAtom paramType;
>+
>+  static void Write(Message* aMsg, const paramType& aParam)
>+  {
>+    WriteParam(aMsg, aParam.get());

'aParam._val' can't be null; see:
http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/src/nsHttp.cpp#188

So, we should just serialize this as an nsCString, which IPDL knows about.

  // aParam.get() cannot be null.
  NS_ASSERTION(aParam.get(), "null nsHttpAtom value");
  nsDependentCString value(aParam.get());
  WriteParam(aMsg, value);

>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>+  {
>+    const char *val;
>+    if(!ReadParam(aMsg, aIter, &val))
>+      return false;
>+
>+    *aResult = nsHttp::ResolveAtom(val);

  nsCAutoString value;
  if (!ReadParam(aMsg, aIter, &value))
    return false;

  *aResult = nsHttp::ResolveAtom(value.get());

>+template<>
>+struct ParamTraits<nsHttpHeaderArray>
>+{
>+  typedef nsHttpHeaderArray paramType;
>+
>+  static void Write(Message* aMsg, const paramType& aParam)
>+  {
>+    paramType p = const_cast<paramType&>(aParam);

Want 'paramType& p' here.

>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>+  {
>+    paramType *p = const_cast<paramType*>(aResult);

Don't need the cast.

>+template<>
>+struct ParamTraits<nsHttpResponseHead>
>+{
>+  typedef nsHttpResponseHead paramType;
>+
>+  static void Write(Message* aMsg, const paramType& aParam)
>+  {
>+    paramType p = const_cast<paramType&>(aParam);

Want 'paramType& p' here.

>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>+  {
>+    paramType *p = const_cast<paramType*>(aResult);

Don't need the cast.

>+    nsHttpVersion version;
>+    PRUint16 status;
>+    nsAFlatCString statusText, contentType, contentCharset;
>+    PRInt64 contentLength;
>+    PRBool noStore, cacheControlNoCache, pragmaNoCache;

Hmm. This would all be easier if you made the getters (e.g. 'p->Version()') return references rather than values or const strings. This may result in compile errors elsewhere in the code, in places where the nsHttpResponseHead is const. For those cases, you can add const versions of the getters as well. (Don't forget to change the 'const nsAFlatCString &' return types to 'nsCString &', and 'PRBool' to 'PRPackedBool &'!)

>diff --git a/netwerk/protocol/http/src/HttpChannelChild.cpp b/netwerk/protocol/http/src/HttpChannelChild.cpp

>+nsresult
>+HttpChannelChild::GetRequestHeader_HACK(const nsACString &header, nsACString &value)
>+{

Need 2-space indenting in this function.

>+nsresult
>+HttpChannelChild::BaseClassSetRequestHeader_HACK(const nsACString &header,
>+                                                 const nsACString &value,
>+                                                 PRBool merge)
>+{

Same here.

>+nsresult
>+HttpChannelChild::GetResponseHeader_HACK(const nsACString &header, nsACString &value)
>+{

And here.

>diff --git a/netwerk/protocol/http/src/nsHttpHeaderArray.h b/netwerk/protocol/http/src/nsHttpHeaderArray.h

>+struct nsEntry
>+{

You can still leave the declaration of nsEntry inside nsHttpHeaderArray - you just need to make it public, and refer to it as 'nsHttpHeaderArray::nsEntry'.

This is looking really good; I think we're done after you fix the above. :)

Thanks for the fast work!

	Jason Duell Reporter
	Comment 17 • 15 years ago

On using 

>-  if (mState < HCC_ONSTART) {

versus

>+  if (!mResponseHead) {

In general, I think checking !mResponseHead is the way to go, simply because it will allow us to share code in the base class.  There are a lot of places in the current HttpChannelChild code where you've added the access logic, but kept the check for mState.  Please switch those to use !mResponseHead if that's what's in nsHttpChannel.cpp. Basically, I'd like to see as many methods in HttpChannelChild be identical to those in nsHttpChannel, where possible, so they can be merged into the base class.   So go take a look at the original nsHttpChannel methods and see if that's possible for each.

(We also have places in HttpChannelChild where I've added ENSURE_CALLED_BEFORE_ASYNC_OPEN() calls as the only difference from the nsHttpChannel code.  I'd like to try adding those to the base class, and if they blow up, we can inspect to see how things are getting used, and we may wind up removing the check.)

------------------------------------------------------------ 
The part of your patch that modifies nsHttpResponseHead to add CacheControlNoCache() and PragmaNoCache() methods failed for me, because of the CacheControlPublic() method that was added in November 2007 (changeset
8464:5fffcdd09553).  Any idea what's up with that?  

------------------------------------------------------------ 
Testing:  we should have tests for all the Get/Set methods we're adding (i.e.  GetContentType, SetContentLength, etc.).  For Get/SetRequestHeader, I realize that the simple test I wrote for you doesn't actually test whether 'merge' works--please add an example that sets the same header name twice with two different values using 'merge=1' and see if it looks correct when the httpd server sees it (i.e. in the handler function).

This will be a fair number of tests, so let's not actually add them to test_simple.js after all (I want to keep that as a simple example).  Why don't you create 'test_heads.js' (and 'unit_ipc/test_heads_wrap.js'), and stick tests for both the request/response logic in there.   If it's not too much trouble, it would be nice to create test_heads.js in the requesthead patch (which looks likely to land first) and then add response tests in your response patch.

>+HttpChannelChild::GetRequestHeader_HACK(const nsACString &header, nsACString &value)
>+{
>
> Need 2-space indenting in this function.

I don't actually care about correct indenting for this function, since it's just a copy-and-paste from nsHttpChannel that we're going to blow away once the base class patch lands.

>+/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*-
>*/
>
> Nit: tab-width should be 2 here.

Wrong! :).  The Mozilla Style guide says to use ts=8:

  https://developer.mozilla.org/En/Developer_Guide/Coding_Style

I believe Jae's patch has the correct modelines verbatim:

/* -*- Mode: C++; tab-width: 8; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
/* vim: set ts=8 et sw=2 tw=80: */

I haven't had time yet to look over the ParamTraits logic, or the rest of the patch--will do soon.

Thanks for both of you for moving this along so quickly--we could use this landing soon.

	Jason Duell Reporter
	Comment 18 • 15 years ago

Two quick clarifications:

The patch hunk that failed was against nsHttpResponseHead.h.

To create test_head.js, just copy test_simple.js, then add your tests.

	Jae-Seong Lee-Russo Assignee
	Comment 19 • 15 years ago

> The part of your patch that modifies nsHttpResponseHead to add
> CacheControlNoCache() and PragmaNoCache() methods failed for me, because of the
> CacheControlPublic() method that was added in November 2007 (changeset
> 8464:5fffcdd09553).  Any idea what's up with that?  

CacheControlPublic() was removed in changeset 51f9d293c939 2010-02-12 - Bug 531801.

	dwitte@gmail.com
	Comment 21 • 15 years ago

Comment on attachment 429137 [details] [diff] [review]
patch for 536279 & 536283, 3

>diff --git a/ipc/glue/IPCMessageUtils.h b/ipc/glue/IPCMessageUtils.h

>+template<>
>+struct ParamTraits<nsDependentCString> : ParamTraits<nsCString>
>+{
>+  typedef nsDependentCString paramType;
>+};

On second thought, wrapping a user-provided buffer and treating it like an nsCString isn't all that great an idea. nsDependentCString asserts on null buffers, but that doesn't mean much in a release build. And we have to worry about other users of this serializer. :/

So let's remove this and use an nsCAutoString instead.

>+template<>
>+struct ParamTraits<nsCAutoString> : ParamTraits<nsCString>
>+{
>+  typedef nsCAutoString paramType;
>+};

This needs to be wrapped in an '#ifdef MOZILLA_INTERNAL_API', otherwise the build breaks when linking xpcshell (and any other external code that #includes this file). :/

Can you add this serializer for nsString as well, for consistency?

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

>+template<>
>+struct ParamTraits<nsHttpResponseHead>
>+{

>+  static bool Read(const Message* aMsg, void** aIter, paramType* aResult)
>+  {
>+    PRInt64 contentLength;
>+
>+    if (!ReadParam(aMsg, aIter, &(aResult->Headers()))             ||
>+        !ReadParam(aMsg, aIter, &(aResult->Version()))             ||
>+        !ReadParam(aMsg, aIter, &(aResult->Status()))              ||
>+        !ReadParam(aMsg, aIter, &(aResult->StatusText()))          ||
>+        !ReadParam(aMsg, aIter, &contentLength)                    ||
>+        !ReadParam(aMsg, aIter, &(aResult->ContentType()))         ||
>+        !ReadParam(aMsg, aIter, &(aResult->ContentCharset()))      ||
>+        !ReadParam(aMsg, aIter, &(aResult->NoStore()))             ||
>+        !ReadParam(aMsg, aIter, &(aResult->CacheControlNoCache())) ||
>+        !ReadParam(aMsg, aIter, &(aResult->PragmaNoCache())))
>+      return false;
>+
>+    aResult->SetContentLength(contentLength);

We shouldn't use the existing SetContentLength() here - it's not the serializer's job to enforce the internal invariants of nsHttpResponseHead. We just want to write the data directly into the member variable. So let's have ContentLength() return a reference, like the others.

>diff --git a/netwerk/protocol/http/src/nsHttpChannel.h b/netwerk/protocol/http/src/nsHttpChannel.h

> public: /* internal; workaround lame compilers */ 
>     typedef void (nsHttpChannel:: *nsAsyncCallback)(void);
>-
>+    nsHttpResponseHead * GetResponseHead() { return mResponseHead; }

Micro-nit: would like to see 'GetResponseHead() const ...' here.

Most excellent. r=dwitte with the above tweaks.

	dwitte@gmail.com
	Comment 22 • 15 years ago

So, I'm getting failures with this patch applied. See bug 549444 for the details, but the gist of it is that a Content-Type hint needs to be provided to the channel, or things go awry. And the Content-Type hint isn't being provided correctly. (There's also an extraneous test for a "text/html" contentType, which fails.)

Attached patch fixes it; test_head.js and test_head_wrapped.js now pass locally for me.

	Jason Duell Reporter
	Comment 23 • 15 years ago

Comment on attachment 429137 [details] [diff] [review]
patch for 536279 & 536283, 3

The patch doesn't compile for me right now--I get

IPCMessageUtils.h:218: error: redefinition of ‘struct IPC::ParamTraits<nsCString_external>’
IPCMessageUtils.h:207: error: previous definition of ‘struct IPC::ParamTraits<nsCString_external>’

Anyone else see this?  I'm not at all sure why it's happening--the error doesn't make much sense to me. 

(This time at least I don't think it's because my repo is stale :)


>diff --git a/netwerk/base/src/Makefile.in b/netwerk/base/src/Makefile.in
>
>+ifdef MOZ_IPC
>+EXPORTS_NAMESPACES = mozilla/net
>+
>+EXPORTS_mozilla/net = \
>+  nsURLHelper.h \
>+  $(NULL)
>+endif

You should use plain "EXPORTS", not EXPORTS_mozilla/net here, as none of nsURLHelper.h's code is in the mozilla/net namespace, and all our code uses "#include <nsURLHelper.h", not <mozilla/net/nsURLHelper.h>.

>diff --git a/netwerk/protocol/http/src/Makefile.in b/netwerk/protocol/http/src/Makefile.in
> 
> ifdef MOZ_IPC
> EXPORTS_NAMESPACES = mozilla/net
> 
> EXPORTS_mozilla/net = \
>   HttpChannelParent.h \
>   HttpChannelChild.h  \
>+  nsHttpResponseHead.h \
>+  nsHttpHeaderArray.h \
>+  nsHttp.h \
>+  nsHttpAtomList.h \
>   $(NULL)

Same for the files that you've added here--use EXPORTS instead (keep HttpChannelChild/Parent in _mozilla/net).

More comments coming--feel free to post a new patch with Dan's and my fixes in the meantime.

	dwitte@gmail.com
	Comment 24 • 15 years ago

(In reply to comment #23)
> (From update of attachment 429137 [details] [diff] [review])
> The patch doesn't compile for me right now--I get
> 
> IPCMessageUtils.h:218: error: redefinition of ‘struct
> IPC::ParamTraits<nsCString_external>’
> IPCMessageUtils.h:207: error: previous definition of ‘struct
> IPC::ParamTraits<nsCString_external>’
> 
> Anyone else see this?  I'm not at all sure why it's happening--the error
> doesn't make much sense to me. 

See comment 21. :)

	Jae-Seong Lee-Russo Assignee
	Comment 25 • 15 years ago

Added changes per comment 21 & 23.
Updated after http://hg.mozilla.org/projects/electrolysis/rev/898ce8caca5a

	Jason Duell Reporter
	Comment 26 • 15 years ago

Comment on attachment 431361 [details] [diff] [review]
 patch for 536279 & 536283, 4

A few more changes, and we'll be ready to land this:

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

So on further thought, since this file is only used by PHttpChannel.ipdl, I think it makes more sense for it to live in netwerk/protocol/http/src, and we should rename it "PHttpChannelParams.h".

> This would all be easier if you made the getters (e.g. 'p->Version()')
> return references rather than values or const strings.

After talking a bit with Dan, we agree that it's a better idea to *not* change the getter function of nsHttpResponseHead to return references.  We'd rather make the ParamTraits class a friend of nsHttpResponseHead, so it can get directly access the variables of nsHttpResponseHead it needs.  I'll attach a patch from Dan that implements the friend declaration.  Then you can just remove the changes to nsHttpResponseHead.h from the patch, and change the paramtraits logic to access the member variables directly.

> >diff --git a/netwerk/protocol/http/src/HttpChannelChild.cpp b/netwerk/protocol/http/src/HttpChannelChild.cpp
> > bool
> >+HttpChannelChild::RecvOnStartRequest(const nsHttpResponseHead& responseHead)
> > {
> >   LOG(("HttpChannelChild::RecvOnStartRequest [this=%x]\n", this));
> >
> >   mState = HCC_ONSTART;
> >
> >+  mResponseHead = new nsHttpResponseHead(responseHead);

This is a bit gross--we're deserializing the entire responseHead into a local variable, then copying the whole thing, which is fairly expensive for such a complex data structure.  It would be better to avoid the copy.  I've talked a little to cjones about this, and have some ideas.  We'll check in this version for now, but we should open up a new bug to optimize this.

	dwitte@gmail.com
	Comment 28 • 15 years ago

Comment on attachment 433896 [details] [diff] [review]
patched (untested) that makes ParamTraits a friend of nsHttpReponseHeader

>diff --git a/netwerk/protocol/http/src/nsHttpResponseHead.h b/netwerk/protocol/http/src/nsHttpResponseHead.h

>+#ifdef MOZ_IPC
>+    friend class ParamTraits;
>+#endif

Nit: 'struct' here please, for consistency.

r=dwitte

	Jason Duell Reporter
	Comment 30 • 15 years ago

Comment on attachment 434029 [details] [diff] [review]
patch for 536279 & 536283, 5

Looks good!  Thanks for all your hard work on this, Jae-Seong!

	Jason Duell Reporter
	Comment 31 • 15 years ago

http://hg.mozilla.org/projects/electrolysis/rev/d56ce00d30e8

I fixed up some of the variable names in test_head.js, so the names are more informative than "foo" and "bar" :)

	dwitte@gmail.com
	Comment 32 • 15 years ago

Yay!! Nice job. :)

	Jason Duell Reporter
	Comment 33 • 15 years ago

Comment on attachment 429598 [details] [diff] [review]
fix test_head.js

already checked in as part of bug fix.