561085 - Make wyciwyg channel work in e10s

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Michal Novotny [:michal]]

Wyciwyg channel needs to cache the content that is being created via document.write() etc.. We can either store the data directly in the channel or we can create a lightweight memory cache in child process that could be also used by FTP channel.

Comment 1

[Michal Novotny [:michal]]

Do we want to keep the same behavior (i.e. use regular cache) in non-e10s builds?

With the current code the wyciwyg documents are persistent across restart. E.g. we can navigate back to wyciwyg document after restart when session saving is turned on. We'll loose this feature if we use in-memory cache. Is this OK?

Comment 2

[Boris Zbarsky [:bzbarsky]]

> We'll loose this feature if we use in-memory cache. Is this OK?

It's not ideal (since it means dataloss on crash), but it might be survivable.  Is there any way to avoid this?

But we do need to be able to perform back/forward in new tabs across wyciwyg pages.

We also need to be able to reload (which means that having the cache in the channel is weird).

The other wyciwyg issue is that the data _could_ be quite big.  Right now we allow it do be usefully sent to disk instead of RAM, right?  At least for non-SSL cases?

Comment 3

[Michal Novotny [:michal]]

(In reply to comment #2)
> It's not ideal (since it means dataloss on crash), but it might be survivable. 
> Is there any way to avoid this?
> 
> But we do need to be able to perform back/forward in new tabs across wyciwyg
> pages.

Hmm, so we probably need to move wyciwyg channel to the parent process just like in case of HTTP or FTP. Or is there a better solution? We've decided to not expose the cache to child process...


> The other wyciwyg issue is that the data _could_ be quite big.  Right now we
> allow it do be usefully sent to disk instead of RAM, right?  At least for
> non-SSL cases?

Yes, right now the disk cache is used if it is enabled.

Comment 4

[Boris Zbarsky [:bzbarsky]]

> we probably need to move wyciwyg channel to the parent process just
> like in case of HTTP or FTP.

That might work, yes.

> We've decided to not expose the cache to child process...

You could poke through a special-purpose API... but I think fundamentally that's the same as making wyciwyg cannels happen in the parent...  Maybe not, though.

Comment 5

[Michal Novotny [:michal]]

Attachment: patch v1

This is a first version ready for the review.

What still doesn't work is:
- Suspend/Resume/Cancel
- Security info isn't serialized and sent to the parent

Comment 6

[Josh Matthews [:jdm]]

You need to be calling Send__delete__ somewhere in there, or the protocol will never die.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 450348 [details] [diff] [review]
patch v1

r=jst for this, but given that this is largely changing code in areas where I'm not very familiar I'd recommend someone else also have a look at the changes to nsSimpleURI, NeckoChild, NeckoParent, PWyciwygChannel, WyciwygChannelChild, and WyciwygChannelParent. Those changes are really not that big, the bulk of the patch is existing code moving around and I've looked over all that. Biesi, could you have a look at those few classes here?

Comment 8

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

Comment on attachment 450348 [details] [diff] [review]
patch v1

I take it that ReadParam<PRBool> can read something written with WriteParam<bool>?

Did you use hg mv to move the wyciwyg files around? This diff lists them as entirely new files :/

Please get rid of the public and src directory in protocol/wyciwyg, those aren't used anymore in necko protocols.

+++ b/netwerk/protocol/wyciwyg/src/PWyciwygChannel.ipdl
+  OnDataAvailable(nsCString data,
+                  PRUint32  offset,
+                  PRUint32  count);
+

You shouldn't need count - that's just data.Length(), right?

+++ b/netwerk/protocol/wyciwyg/src/WyciwygChannelParent.cpp
+// C++ file contents

That seems like a fairly useless comment

+  // XXX handle 64-bit stuff for real

Not really an issue for this file

+  data.SetLength(aCount);

Hmm... can that no longer fail?

+  data.EndWriting();

You don't need that; it's maybe a bit confusingly named but all it does is give you a writable iterator past the last character of the string.

+  if (!NS_SUCCEEDED(rv) || bytesRead != aCount) {

!NS_SUCCEEDED -> NS_FAILED; please change this in all places where you're doing that

+  // TODO security info must be serialized and sent to the parent
+  mSecurityInfo = aSecurityInfo;

Yes, that's actually somewhat important I think, would break SSL document.write otherwise.

+WyciwygChannelChild::GetStatus(nsresult *aStatus)
+{
+  NS_ENSURE_ARG_POINTER(aStatus);


Please don't nullcheck out parameters
+WyciwygChannelParent::GetInterface(const nsIID& uuid, void **result)
+{
+  DROP_DEAD();
+}

Why bother implementing nsIInterfaceRequestor?

+++ b/netwerk/protocol/wyciwyg/src/nsWyciwyg.h
+#define wyciwyg_TYPE "text/html"

Please name this in all uppercase, i.e. WYCIWYG_TYPE

Comment 9

[Steffen Imhof]

Attachment: patch v1 updated to latest m-c

When trying to collect some patches, I updated this one to the latest m-c.
Bigger changes were moving netwerk/protocol/wyciwyg/src and public into one dir and the registration stuff in nsNetModule.cpp
(this updated patch is also built on bug 565163)

Comment 10

[Michal Novotny [:michal]]

Attachment: patch v2

(In reply to comment #8)
> I take it that ReadParam<PRBool> can read something written with
> WriteParam<bool>?

fixed

> Did you use hg mv to move the wyciwyg files around? This diff lists them as
> entirely new files :/

This is fixed in the new patch.

> Please get rid of the public and src directory in protocol/wyciwyg, those
> aren't used anymore in necko protocols.

fixed

> +++ b/netwerk/protocol/wyciwyg/src/PWyciwygChannel.ipdl
> +  OnDataAvailable(nsCString data,
> +                  PRUint32  offset,
> +                  PRUint32  count);
> +
> 
> You shouldn't need count - that's just data.Length(), right?

count removed

> +  data.SetLength(aCount);
> 
> Hmm... can that no longer fail?

Not sure... The same code is in HttpChannelParent::OnDataAvailable()

> +  data.EndWriting();
> 
> You don't need that; it's maybe a bit confusingly named but all it does is give
> you a writable iterator past the last character of the string.

removed

> +  if (!NS_SUCCEEDED(rv) || bytesRead != aCount) {
> 
> !NS_SUCCEEDED -> NS_FAILED; please change this in all places where you're doing
> that

fixed
 
> +  // TODO security info must be serialized and sent to the parent
> +  mSecurityInfo = aSecurityInfo;
>
> Yes, that's actually somewhat important I think, would break SSL document.write
> otherwise.

I've added the serialization of security info that should IMO work. But there seems to be some bug somewhere because nsHTMLDocument has mSecurityInfo==null at time of call to nsWyciwygChannel::SetSecurityInfo().

> +WyciwygChannelChild::GetStatus(nsresult *aStatus)
> +{
> +  NS_ENSURE_ARG_POINTER(aStatus);
> 
> Please don't nullcheck out parameters

fixed

> +WyciwygChannelParent::GetInterface(const nsIID& uuid, void **result)
> +{
> +  DROP_DEAD();
> +}
> 
> Why bother implementing nsIInterfaceRequestor?

removed (the code was taken from FTP protocol)

> +++ b/netwerk/protocol/wyciwyg/src/nsWyciwyg.h
> +#define wyciwyg_TYPE "text/html"
> 
> Please name this in all uppercase, i.e. WYCIWYG_TYPE

fixed

Comment 11

[Michal Novotny [:michal]]

nsSimpleNestedURI and nsNestedAboutURI inherit from nsSimpleURI. They use nsIObjectInputStream::WriteCompoundObject and nsIObjectInputStream::ReadObject in their nsISerializable::Read/Write methods. How this should be handled in nsIIPCSerializable::Read/Write methods? For now I didn't implement them, but this needs to be fixed.

Comment 12

[Josh Matthews [:jdm]]

Please note the lack of Send__delete__ anywhere.  This protocol will never die.

Comment 13

[Jason Duell]

dwitte:  do you have an answer for comment 11?

Comment 14

[Doug Turner (:dougt)]

next steps here?  jason/dwitte

Comment 15

[Michal Novotny [:michal]]

As Josh suggested I need to add Send__delete__ somewhere. And IPC serialization of nsSimpleNestedURI and nsNestedAboutURI needs to be implemented, but maybe this can be made later.

Comment 16

[Doug Turner (:dougt)]

wouldn't we leak?

Comment 17

[Michal Novotny [:michal]]

I meant that just IPC serialization maybe could be done later. That shouldn't leak. I think that neither nsSimpleNestedURI nor nsNestedAboutURI are sent across processes but I'm not sure. They implement nsIIPCSerializable just because they inherit from nsSimpleURI.

Comment 18

[Michal Novotny [:michal]]

Attachment: patch v3

- fixed serialization of nsSimpleNestedURI and nsNestedAboutURI
- fixed destroying of the IPDL protocol

Although patch v1 has already r+ and sr+, could anybody have a look at this patch?

Comment 19

[Jason Duell]

Comment on attachment 465414 [details] [diff] [review]
patch v3

JST/biesi, 

Either of you have time to review Michal's fixes on this?  Seems very close if not ready to land.

Comment 20

[Josh Matthews [:jdm]]

nsIPCSerializable::Read should return bool, not PRBool.

>+	using RequestHeaderTuples;

Unused.

There are many places that IPDL handlers return false in this patch.  That is undesirable, as it will terminate the content process in the future.

Also, do we need to replicate the HTTP IPDL queuing strategy here?

Furthermore, the AddRef in NeckoChild::AllocPWyciwygChannel makes me suspect that we'll actually leak the child channel object.  We're addrefing the created actor, then addrefing again in nsWyciwygProtocolHandler::NewChannel.  Why don't we move the AddIPDLReference into AllocPWyciwygChannel and get some nice symmetry going between the Alloc and Dealloc functions?

Comment 21

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

Comment on attachment 465414 [details] [diff] [review]
patch v3

+++ b/netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp
+WyciwygChannelChild::RecvOnStartRequest(const PRInt32& contentLength,

You ought to pass status to RecvOnStartRequest as well, so that mStatus is correct here.

+    // TODO: Cancel request:

That's kinda important...

+  stringStream->Close();

No real need to close a string stream

+WyciwygChannelChild::Cancel(nsresult aStatus)
+{
+  DROP_DEAD();

Seems risky but sure...

+WyciwygChannelChild::SetNotificationCallbacks(nsIInterfaceRequestor * aCallbacks)
+{
+  mCallbacks = aCallbacks;
+  mProgressSink = do_GetInterface(mCallbacks);

Your semantics are not quite correct - if there are no notificationcallbacks or if the notification callbacks don't have a progress sink, you should query the loadgroup's callbacks for a sink.

In short, you should call NS_QueryNotificationCallbacks(mCallbacks, mLoadGroup, NS_GET_IID(nsIProgressSink), getter_AddRefs(mProgressSink)) in both SetLoadGroup and SetNotificationCallbacks.

+    if (serializable) {
+    nsCString secInfoStr;
+      NS_SerializeToString(serializable, secInfoStr);
+      SendSetSecurityInfo(secInfoStr);
+    }

please fix the indentation on the nsCString line

you should probably have an NS_ERROR or something for the case that the securityInfo isn't serializable

+++ b/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp
+  nsCOMPtr<nsIURI> uri(aURI);

why?

Comment 22

[[e-mail]]

I think we want this to block 2.0b2 -- fixing this means we can disable cache in the child entirely. (FTP is being remoted for b2, so it won't need cache in the child at all.)

I can review this pretty soon.

Comment 23

[[e-mail]]

Comment on attachment 465414 [details] [diff] [review]
patch v3

Yoink!

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

we'll take this if and when the patch is reviewed and it has tests, but we're not going to block on it.

Comment 25

[[e-mail]]

Comment on attachment 465414 [details] [diff] [review]
patch v3

>diff --git a/netwerk/base/src/nsSimpleNestedURI.h b/netwerk/base/src/nsSimpleNestedURI.h

>+    // nsIIPCSerializable overrides
>+    PRBool Read(const IPC::Message *aMsg, void **aIter);
>+    void Write(IPC::Message *aMsg);

Can't you just use NS_DECL_NSIIPCSERIALIZABLE here?

>diff --git a/netwerk/build/nsNetModule.cpp b/netwerk/build/nsNetModule.cpp

>+#ifdef NECKO_PROTOCOL_viewsource
>+NS_DEFINE_NAMED_CID(NS_WYCIWYGPROTOCOLHANDLER_CID);
>+#endif

You mean NECKO_PROTOCOL_wyciwyg right?

>diff --git a/netwerk/ipc/NeckoChild.cpp b/netwerk/ipc/NeckoChild.cpp

>+PWyciwygChannelChild*
>+NeckoChild::AllocPWyciwygChannel()
>+{
>+  WyciwygChannelChild *p = new WyciwygChannelChild();
>+  p->AddRef();
>+  return p;
>+}

Like jdm said: move the AddIPDLReference call in nsWyciwygProtocolHandler::NewChannel to here?

>diff --git a/netwerk/protocol/about/nsAboutProtocolHandler.h b/netwerk/protocol/about/nsAboutProtocolHandler.h

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

Can't you just use NS_DECL_NSIIPCSERIALIZABLE here?

>diff --git a/netwerk/protocol/wyciwyg/PWyciwygChannel.ipdl b/netwerk/protocol/wyciwyg/PWyciwygChannel.ipdl

>+parent:
>+  __delete__();
>+
>+  Init(URI uri);
>+  AsyncOpen(URI      originalURI,
>+            PRUint32 loadFlags);
>+  WriteToCacheEntry(nsString data);
>+  CloseCacheEntry(nsresult reason);
>+  SetCharsetAndSource(PRInt32 source, nsCString charset);
>+  SetSecurityInfo(nsCString securityInfo);
>+
>+child:
>+  OnStartRequest(PRInt32   contentLength,
>+                 PRInt32   source,
>+                 nsCString charset,
>+                 nsCString securityInfo);
>+
>+  OnDataAvailable(nsCString data,
>+                  PRUint32  offset);
>+
>+  OnStopRequest(nsresult statusCode);

Document what all these do, please. (If they're documented elsewhere, you can just refer to those; otherwise, write a brief comment.)

>diff --git a/netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp b/netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp

>+bool
>+WyciwygChannelChild::RecvOnStartRequest(const PRInt32& contentLength,
>+                                        const PRInt32& source,
>+                                        const nsCString& charset,
>+                                        const nsCString& securityInfo)
>+{
>+  LOG(("WyciwygChannelChild::RecvOnStartRequest [this=%x]\n", this));
>+
>+  mState = WCC_ONSTART;
>+
>+  mContentLength = contentLength;
>+  mCharsetSource = source;
>+  mCharset = charset;
>+
>+  if (!securityInfo.IsEmpty()) {
>+    NS_DeserializeObject(securityInfo, getter_AddRefs(mSecurityInfo));
>+  }
>+
>+  nsresult rv = mListener->OnStartRequest(this, mListenerContext);
>+  if (NS_FAILED(rv)) {
>+    // TODO: Cancel request:

File a followup for fixing this (and other TODOs, e.g. "send failure message to child") up?

>+bool
>+WyciwygChannelChild::RecvOnStopRequest(const nsresult& statusCode)
>+{

>+  // This calls NeckoChild::DeallocPWyciwygChannel(), which deletes |this| if
>+  // IPDL holds the last reference.  Don't rely on |this| existing after here.
>+  PWyciwygChannelChild::Send__delete__(this);

We have an mIPCOpen flag in HttpChannelChild for this purpose. Use the same thing here? See e.g. http://mxr.mozilla.org/mozilla-central/source/netwerk/protocol/http/HttpChannelChild.cpp#593

>+/* void cancel (in nsresult aStatus); */
>+NS_IMETHODIMP
>+WyciwygChannelChild::Cancel(nsresult aStatus)
>+{
>+  DROP_DEAD();

Are we really guaranteed that this will never be called? If not, we should just return failure instead. Same with Suspend() and Resume().

>+NS_IMETHODIMP
>+WyciwygChannelChild::SetOriginalURI(nsIURI * aOriginalURI)
>+{
>+  NS_ENSURE_TRUE(mState == WCC_INIT, NS_ERROR_UNEXPECTED);
>+
>+  // TODO mOriginalURI is sent to the parent only when reading from the channel.
>+  // Do we need to send it when writing to the cache too?

Don't really understand this comment. Does writing need to know the original URI?

>+NS_IMETHODIMP
>+WyciwygChannelChild::SetContentLength(PRInt32 aContentLength)
>+{
>+  // SetContentLength() doesn't seem to be used
>+  DROP_DEAD();

Hmm. Return failure instead? "Doesn't seem" isn't a very strong statement. ;)

>+/* void closeCacheEntry (in nsresult reason); */
>+NS_IMETHODIMP
>+WyciwygChannelChild::CloseCacheEntry(nsresult reason)
>+{
>+  NS_ENSURE_TRUE(mState == WCC_ONWRITE, NS_ERROR_UNEXPECTED);
>+
>+  SendCloseCacheEntry(reason);
>+  mState = WCC_ONCLOSED;
>+
>+  // This calls NeckoChild::DeallocPWyciwygChannel(), which deletes |this| if
>+  // IPDL holds the last reference.  Don't rely on |this| existing after here.
>+  PWyciwygChannelChild::Send__delete__(this);

Same thing about mIPCOpened.

>+/* void setSecurityInfo (in nsISupports aSecurityInfo); */
>+NS_IMETHODIMP
>+WyciwygChannelChild::SetSecurityInfo(nsISupports *aSecurityInfo)
>+{
>+  mSecurityInfo = aSecurityInfo;
>+
>+  if (mSecurityInfo) {
>+    nsCOMPtr<nsISerializable> serializable = do_QueryInterface(mSecurityInfo);
>+    if (serializable) {
>+    nsCString secInfoStr;
>+      NS_SerializeToString(serializable, secInfoStr);

Indenting is off.

>diff --git a/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp b/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp

>+NS_IMETHODIMP
>+WyciwygChannelParent::OnDataAvailable(nsIRequest *aRequest,
>+                                      nsISupports *aContext,
>+                                      nsIInputStream *aInputStream,
>+                                      PRUint32 aOffset,
>+                                      PRUint32 aCount)
>+{
>+  LOG(("WyciwygChannelParent::OnDataAvailable [this=%x]\n", this));
>+
>+  nsresult rv;
>+
>+  nsCString data;
>+  data.SetLength(aCount);
>+  char * p = data.BeginWriting();
>+  PRUint32 bytesRead;
>+  rv = aInputStream->Read(p, aCount, &bytesRead);
>+  if (NS_FAILED(rv) || bytesRead != aCount) {
>+    return rv;              // TODO: figure out error handling
>+  }

Use http://mxr.mozilla.org/mozilla-central/source/netwerk/base/public/nsNetUtil.h#1149 here.

>diff --git a/netwerk/protocol/wyciwyg/nsWyciwyg.cpp b/netwerk/protocol/wyciwyg/nsWyciwyg.cpp

>+#if defined(PR_LOGGING)
>+PRLogModuleInfo *gWyciwygLog = nsnull;
>+#endif
>+

>diff --git a/netwerk/protocol/wyciwyg/nsWyciwyg.h b/netwerk/protocol/wyciwyg/nsWyciwyg.h

>+#ifdef MOZ_IPC
>+// e10s mess: IPDL-generatd headers include chromium which both #includes
>+// prlog.h, and #defines LOG in conflict with this file.

Hmm. Do you still need all this? I think cjones fixed it up.

>diff --git a/content/html/document/src/nsWyciwygChannel.cpp b/netwerk/protocol/wyciwyg/nsWyciwygChannel.cpp

> NS_IMETHODIMP
> nsWyciwygChannel::GetContentLength(PRInt32 *aContentLength)
> {
>-  return NS_ERROR_NOT_IMPLEMENTED;
>+  *aContentLength = mContentLength;
>+  return NS_OK;

Why do you need this?

>diff --git a/content/html/document/src/nsWyciwygProtocolHandler.cpp b/netwerk/protocol/wyciwyg/nsWyciwygProtocolHandler.cpp

> nsWyciwygProtocolHandler::nsWyciwygProtocolHandler() 
> {
>+#if defined(PR_LOGGING)
>+  if (!gWyciwygLog)
>+    gWyciwygLog = PR_NewLogModule("nsWyciwygChannel");

Why do we need the 'if' check?

>+#endif
> 
>-#if defined(PR_LOGGING)
>-  gWyciwygLog = PR_NewLogModule("nsWyciwygChannel");
>-#endif
>+  LOG(("Creating nsHttpHandler [this=%x].\n", this));

You mean WyciwygProtocolHandler?

> nsWyciwygProtocolHandler::~nsWyciwygProtocolHandler() 
> {
>+  LOG(("Deleting nsHttpHandler [this=%x]\n", this));

You mean WyciwygProtocolHandler?

>+nsresult::Init()
>+{

I can't see anywhere this gets called. Can you remove it? Or if you need to check and instantiate the necko child, do it in NewChannel() below?

> NS_IMETHODIMP
> nsWyciwygProtocolHandler::NewChannel(nsIURI* url, nsIChannel* *result)
> {

>+  if (IsNeckoChild()) {
>+    NS_ENSURE_TRUE(gNeckoChild != nsnull, NS_ERROR_FAILURE);
>+
>+    WyciwygChannelChild *wcc = static_cast<WyciwygChannelChild *>(
>+                                 gNeckoChild->SendPWyciwygChannelConstructor());
>+    if (!wcc)
>+      return NS_ERROR_OUT_OF_MEMORY;
>+
>+    wcc->AddIPDLReference();

Per above, this should go into NeckoChild::AllocPWyciwygChannel.

>+
>+    channel = wcc;
>+    rv = wcc->Init(url);

I think you need to Send__delete__ if this fails, otherwise you'll leak the IPDL objects.

>+  } else
>+#endif
>+  {
>+    nsWyciwygChannel *wc = new nsWyciwygChannel();
>+    if (!wc)
>+      return NS_ERROR_OUT_OF_MEMORY;
>+    channel = wc;
>+    rv = wc->Init(url);
>+  }
>+
>   NS_ADDREF(channel);

Hmm. How does this compile? Manually addrefing a COMPtr should be illegal.

In any case, just do this:

  return channel.forget(result);

r=dwitte with fixes.

Comment 26

[Jason Duell]

>+#ifdef MOZ_IPC
>+// e10s mess: IPDL-generatd headers include chromium which both #includes
>+// prlog.h, and #defines LOG in conflict with this file.
>
> Hmm. Do you still need all this? I think cjones fixed it up.

AFAIK we still need it.  If that's changed, let's modify nsHttp.h, too.

Comment 27

[Michal Novotny [:michal]]

Attachment: patch v4

> +++ b/netwerk/protocol/wyciwyg/WyciwygChannelParent.cpp
> +  nsCOMPtr<nsIURI> uri(aURI);
> 
> why?

I don't understand the question. I need nsIURI, what else should I do?


> nsIPCSerializable::Read should return bool, not PRBool.

But it wouldn't compile. From nsIIPCSerializable.h :

#define NS_DECL_NSIIPCSERIALIZABLE \
  NS_IMETHOD_(PRBool ) Read(const IPC::Message *msg, void* *iter); \
  NS_IMETHOD_(void) Write(IPC::Message *msg);.


> Also, do we need to replicate the HTTP IPDL queuing strategy here?

Not sure. In which situation do we need it?


> > NS_IMETHODIMP
> > nsWyciwygChannel::GetContentLength(PRInt32 *aContentLength)
> > {
> >-  return NS_ERROR_NOT_IMPLEMENTED;
> >+  *aContentLength = mContentLength;
> >+  return NS_OK;
> 
> Why do you need this?

It is called from WyciwygChannelParent::OnStartRequest().


> >+nsresult::Init()
> >+{
> 
> I can't see anywhere this gets called. Can you remove it? Or if you need to
> check and instantiate the necko child, do it in NewChannel() below?

It should be nsWyciwygProtocolHandler::Init(), right? It is called here:
NS_GENERIC_FACTORY_CONSTRUCTOR_INIT(nsWyciwygProtocolHandler, Init)
I've removed it and moved the check into NewChannel() as suggested.


> > nsWyciwygProtocolHandler::nsWyciwygProtocolHandler() 
> > {
> >+#if defined(PR_LOGGING)
> >+  if (!gWyciwygLog)
> >+    gWyciwygLog = PR_NewLogModule("nsWyciwygChannel");
> 
> Why do we need the 'if' check?

gWyciwygLog can be initialized in nsWyciwygProtocolHandler (standard build) or on WyciwygChannelParent (e10s build).


> There are many places that IPDL handlers return false in this patch.  That is
> undesirable, as it will terminate the content process in the future.
> 
> >+  nsresult rv = mListener->OnStartRequest(this, mListenerContext);
> >+  if (NS_FAILED(rv)) {
> >+    // TODO: Cancel request:
> 
> File a followup for fixing this (and other TODOs, e.g. "send failure message to
> child") up?

I'll file a followup bug for fixing both: returning false and correct error handling.

Comment 28

[Josh Matthews [:jdm]]

>> Also, do we need to replicate the HTTP IPDL queuing strategy here?
>
>Not sure. In which situation do we need it?

I believe it would be necessary in situations where we're triggering listeners that could end up spinning the event loop.

Comment 29

[Jason Duell]

dougt: mark this as fennec-2.0b (blocks other 2.0b bugs).

Sounds like this needs queuing, and then may be ready.

Comment 30

[Josh Matthews [:jdm]]

Attachment: Add IDPL queueing to wygiwyg protocol.

Comment 31

[Josh Matthews [:jdm]]

The queuing patch depends on the IPDL generalization patch in the FTP implementation.

Comment 32

[[e-mail]]

Comment on attachment 482226 [details] [diff] [review]
patch v4

r=dwitte

Comment 33

[Josh Matthews [:jdm]]

If we're in a position to check this in, I recommend pushing Part 1.5 from bug 536289 and the two patches here together.

Comment 34

[[e-mail]]

Comment on attachment 483836 [details] [diff] [review]
Add IDPL queueing to wygiwyg protocol.

>diff --git a/netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp b/netwerk/protocol/wyciwyg/WyciwygChannelChild.cpp

>@@ -154,58 +218,83 @@ WyciwygChannelChild::RecvOnDataAvailable
>   // rest.
>   nsCOMPtr<nsIInputStream> stringStream;
>   nsresult rv = NS_NewByteInputStream(getter_AddRefs(stringStream),
>                                       data.get(),
>                                       data.Length(),
>                                       NS_ASSIGNMENT_DEPEND);
>   if (NS_FAILED(rv)) {
>     // TODO:  what to do here?  Cancel request?  Very unlikely to fail.
>-    return false;
>   }

So we silently continue? o_O

>diff --git a/netwerk/protocol/wyciwyg/WyciwygChannelChild.h b/netwerk/protocol/wyciwyg/WyciwygChannelChild.h

>+bool
>+WyciwygChannelChild::IsSuspended()
>+{
>+  return false;

Where's this used?

r=dwitte with comments addressed.

Comment 35

[Josh Matthews [:jdm]]

Michal indicated that the various error/cancellation TODOs would be cleaned up in a followup.  IsSuspended() is part of the new IPDL event queue interface: ChannelEventQueue derivations implement IsSuspended() so the logic in FlushEventQueue and ShouldEnqueue is correct.

Comment 36

[Michal Novotny [:michal]]

The follow-up bug is #605327.

Comment 37

[Josh Matthews [:jdm]]

http://hg.mozilla.org/mozilla-central/rev/c56bcfa6defb
http://hg.mozilla.org/mozilla-central/rev/45fa68335edf

Comment 38

[Jason Duell]

Checked in--I assume should be RESOLVED-FIXED.