Last Comment Bug 639830 - WyciwygChannelChild is not thorough about checking for cancellation
: WyciwygChannelChild is not thorough about checking for cancellation
Status: RESOLVED FIXED
[inbound]
: crash
Product: Core
Classification: Components
Component: Networking (show other bugs)
: unspecified
: x86 Linux
: -- normal (vote)
: ---
Assigned To: Sreenidhi Nair
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-03-08 07:51 PST by Josh Matthews [:jdm]
Modified: 2011-09-09 22:47 PDT (History)
6 users (show)
See Also:
Crash Signature:
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Proposed fix for bug 639830 - Added null checks for the required functions (3.18 KB, patch)
2011-09-02 09:46 PDT, Sreenidhi Nair
no flags Details | Diff | Splinter Review
Proposed fix for bug 639830 - Edited (2.28 KB, patch)
2011-09-02 11:33 PDT, Sreenidhi Nair
no flags Details | Diff | Splinter Review
Proposed fix for bug 639830 - Edited (2.18 KB, patch)
2011-09-02 12:34 PDT, Sreenidhi Nair
jduell.mcbugs: review+
Details | Diff | Splinter Review

Description Josh Matthews [:jdm] 2011-03-08 07:51:05 PST
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] 2011-08-03 12:12:33 PDT
The fix here should add null checks to WyciwygChannelParent::RecvAsyncOpen, RecvWriteToCacheEntry, RecvCloseCacheEntry, RecvSetCharsetAndSource, and RecvSetSecurityInfo.
Comment 2 Josh Matthews [:jdm] 2011-08-23 15:49:59 PDT
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 2011-09-01 22:24:25 PDT
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] 2011-09-02 06:25:01 PDT
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 2011-09-02 09:46:35 PDT
Created attachment 557866 [details] [diff] [review]
Proposed fix for bug 639830 - Added null checks for the required functions
Comment 6 Josh Matthews [:jdm] 2011-09-02 10:35:51 PDT
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 2011-09-02 11:33:53 PDT
Created attachment 557896 [details] [diff] [review]
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] 2011-09-02 11:49:30 PDT
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 2011-09-02 12:34:28 PDT
Created attachment 557915 [details] [diff] [review]
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] 2011-09-02 12:39:20 PDT
Comment on attachment 557915 [details] [diff] [review]
Proposed fix for bug 639830 - Edited

Great! This looks ready for review!
Comment 11 Jason Duell [:jduell] (needinfo me) 2011-09-09 15:54:41 PDT
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) 2011-09-09 22:47:47 PDT
(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)

Note You need to log in before you can comment on or make changes to this bug.