639830 - WyciwygChannelChild is not thorough about checking for cancellation

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Josh Matthews [:jdm]]

Example crash: https://crash-stats.mozilla.com/report/index/c6e18033-fa65-409c-b824-2c6c52110307

Since we've got a null mChannel in the parent, presumably we failed to create it in RecvInit and sent CancelEarly to the child.  The listener presumably did a SetCharsetAndSource in a callback, and these are the only checks present:

>642 // mState == WCC_ONSTART when reading from the channel
>643 // mState == WCC_INIT when writing to the cache
>644 NS_ENSURE_TRUE((mState == WCC_ONSTART) ||
>645                (mState == WCC_INIT), NS_ERROR_UNEXPECTED); 

mState isn't updated by cancellation, so the child merrily sent the message to the parent.  We can either fix this by being thorough about cancellation checks, or add null checks to the parent.

Comment 1

[Josh Matthews [:jdm]]

The fix here should add null checks to WyciwygChannelParent::RecvAsyncOpen, RecvWriteToCacheEntry, RecvCloseCacheEntry, RecvSetCharsetAndSource, and RecvSetSecurityInfo.

Comment 2

[Josh Matthews [:jdm]]

Krangam, are you planning to do this work? If not, would you mind de-assigning yourself so someone else can take a crack at it?

Comment 3

[Sreenidhi Nair]

I'd like to take this one up. I'm assuming we need to do something like
if(mChannel) { /*do stuff*/ } . I'm a newbie though - do correct me if I'm wrong.

Comment 4

[Josh Matthews [:jdm]]

For RecvAsyncOpen, we can just do an early return after the LOG statement if there's no channel, for the rest you can use what you wrote.

Comment 5

[Sreenidhi Nair]

Attachment: Proposed fix for bug 639830 - Added null checks for the required functions

Comment 6

[Josh Matthews [:jdm]]

Thanks for the patch Sreenidhi! I'm going to request a couple changes before we get the official review from a peer of this code.

>@@ -88,16 +88,19 @@ WyciwygChannelParent::RecvInit(const IPC
>   LOG(("WyciwygChannelParent RecvInit [this=%x uri=%s]\n",
>        this, uriSpec.get()));
> 
>+  if(!mChannel)
>+	return SendCancelEarly(rv);

While null checks are good, there can be too much of a good thing. Specifically, we don't actually assign anything to mChannel until later in this function :P You can just revert this bit to the way it was.

> WyciwygChannelParent::RecvAsyncOpen(const IPC::URI& aOriginal,
>                                     const PRUint32& aLoadFlags)
> {
>-  nsCOMPtr<nsIURI> original(aOriginal);
>+  if(mChannel)
>+  {
>+  	nsCOMPtr<nsIURI> original(aOriginal);

Instead of having a big indented block here, you can just do
if (!mChannel)
  return true;
and leave the rest as it was.

>+  if(mChannel)
>+  	mChannel->WriteToCacheEntry(data);

You seem to be using tabs almost everywhere in this patch, and we prefer spaces for indenting. Can you fix your editor to do this and make sure they're all replaced in your changes?

Again, thanks for the patch. This should fix some crash reports that have been coming in for Firefox Mobile, so I'm thrilled you've taken this on :)

Comment 7

[Sreenidhi Nair]

Attachment: Proposed fix for bug 639830 - Edited

Oops, sorry about the tabs, I forgot it was supposed to be 2 spaces for indentation. Other changes have been added.

Comment 8

[Josh Matthews [:jdm]]

Comment on attachment 557896 [details] [diff] [review]
Proposed fix for bug 639830 - Edited

One more revision and this should be good! We'd like to see the log message for RecvAsyncOpen, so move the null check after it, please. Furthermore, the style we use is |if (condition)| (note the space after if), so if you could fix all of those, that would be great. Lastly, you added some spaces after the |return true;| in RecvAsyncOpen.

Comment 9

[Sreenidhi Nair]

Attachment: Proposed fix for bug 639830 - Edited

Added the spaces between if, removed the trailing spaces, and null check is now after LOG.

Comment 10

[Josh Matthews [:jdm]]

Comment on attachment 557915 [details] [diff] [review]
Proposed fix for bug 639830 - Edited

Great! This looks ready for review!

Comment 11

[Jason Duell]

Looks good.  I landed the patch on inbound.

  http://hg.mozilla.org/integration/mozilla-inbound/rev/f06272a4447f

Sreenidhi:  if all goes well (which I'm 99% sure it will), this will wind up getting semi-automagically landed on the main mozilla-central tree and marked FIXED.

Thanks for your help!

Comment 12

[Justin Wood (:Callek)]

(In reply to Jason Duell (:jduell) from comment #11)
> Looks good.  I landed the patch on inbound.
> 
>   http://hg.mozilla.org/integration/mozilla-inbound/rev/f06272a4447f
> 
> Sreenidhi:  if all goes well (which I'm 99% sure it will), this will wind up
> getting semi-automagically landed on the main mozilla-central tree and
> marked FIXED.

Done (landed in m-c)