WyciwygChannelChild is not thorough about checking for cancellation

RESOLVED FIXED

Status

()

Core
Networking
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: jdm, Assigned: Sreenidhi Nair)

Tracking

({crash})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound], crash signature)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

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

Updated

6 years ago
Component: Networking → Plug-ins
QA Contact: networking → plugins
Summary: WyciwygChannelChild is not thorough about checking for cancellation → WyciwygChannelChild is not thorough about checking for cancellation (crash [@ mozilla::net::WyciwygChannelParent::RecvSetCharsetAndSource ])
(Reporter)

Updated

6 years ago
Whiteboard: [good first bug]
(Reporter)

Updated

6 years ago
Component: Plug-ins → Networking
QA Contact: plugins → networking
(Reporter)

Updated

6 years ago
Whiteboard: [good first bug] → [good first bug][mentor=jdm]
Crash Signature: [@ mozilla::net::WyciwygChannelParent::RecvSetCharsetAndSource ]
(Reporter)

Comment 1

6 years ago
The fix here should add null checks to WyciwygChannelParent::RecvAsyncOpen, RecvWriteToCacheEntry, RecvCloseCacheEntry, RecvSetCharsetAndSource, and RecvSetSecurityInfo.
Crash Signature: [@ mozilla::net::WyciwygChannelParent::RecvSetCharsetAndSource ] → [@ mozilla::net::WyciwygChannelParent::RecvSetCharsetAndSource ] [@ mozilla::net::WyciwygChannelParent::RecvSetSecurityInfo ]
Summary: WyciwygChannelChild is not thorough about checking for cancellation (crash [@ mozilla::net::WyciwygChannelParent::RecvSetCharsetAndSource ]) → WyciwygChannelChild is not thorough about checking for cancellation

Updated

6 years ago
Assignee: nobody → krangam
(Reporter)

Comment 2

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

Updated

6 years ago
Assignee: krangam → nobody
(Assignee)

Comment 3

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

Comment 4

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

Comment 5

6 years ago
Created attachment 557866 [details] [diff] [review]
Proposed fix for bug 639830 - Added null checks for the required functions
(Reporter)

Comment 6

6 years ago
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 :)
Assignee: nobody → sreenidimn
(Assignee)

Comment 7

6 years ago
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.
Attachment #557866 - Attachment is obsolete: true
(Reporter)

Comment 8

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

Comment 9

6 years ago
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.
Attachment #557896 - Attachment is obsolete: true
(Reporter)

Comment 10

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

Great! This looks ready for review!
Attachment #557915 - Flags: review?(jduell.mcbugs)
Attachment #557915 - Flags: review?(jduell.mcbugs) → review+
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!
Whiteboard: [good first bug][mentor=jdm] → [inbound]
(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)
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.