949667 - crash in mozilla::net::HttpChannelParent::OnStartRequest(nsIRequest*, nsISupports*)

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Honza Bambas (:mayhemer)]

Attachment: WIP1

+++ This bug was initially created as a clone of Bug #932046 +++

On the latest 1.3 mozRIL I got these two crashes when selecting a video within the Terra app.

https://crash-stats.mozilla.com/report/index/751ecba6-0a93-4ce8-9ba6-6beb52131206

https://crash-stats.mozilla.com/report/index/e6782082-a7e9-49c9-a48b-01de12131206

Environmental Variables:
Device: Buri 1.3 mozRIL
BuildID: 20131206040203
Gaia: 8fca2ca67e8a6022fe6ed8cb576e5d59dfb5237f
Gecko: 1401e4b394ad
Version: 28.0a1
Firmware Version: 20131115


The simplest and quickest solution right now is to check the channel in HttpChannelParent::OnStartRequest impls nsIHttpChannelInternal.  If not, fail.  

But RTSP channel has gaps here too.  I'll file one more bug.

Comment 1

[Jason Smith [:jsmith]]

Does this need to be nominated for 1.3? - sounds like this is going to affect a target partner app.

Comment 2

[Honza Bambas (:mayhemer)]

Attachment: v1

This is the simplest possible patch to fix this crash.  It was actually lying just in front of our eyes all the time.  When the child channel misses the nsIChildChannel when a redirect begins, just veto it right away.

https://tbpl.mozilla.org/?tree=Try&rev=cdbc5448dfd7

Comment 3

[Jason Duell]

Comment on attachment 8349578 [details] [diff] [review]
v1

Review of attachment 8349578 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/HttpChannelChild.cpp
@@ +747,5 @@
> +    rv = gHttpHandler->AsyncOnChannelRedirect(this,
> +                                              newChannel,
> +                                              redirectFlags);
> +  }
> +  else {

Don't use this horrible multiline "}\n else {" in new code.  I hope you and Michal aren't using it in the new cache code.  We're trying to make Mozilla code look more similar over time, new code doesn't use this.

::: netwerk/protocol/http/HttpChannelParentListener.cpp
@@ +176,5 @@
>  
>        if (NS_SUCCEEDED(rv))
>          newChannel->Cancel(NS_BINDING_ABORTED);
> +
> +      succeeded = false;

Looks like HttpChannelParentListener::OnRedirectResult could use some MOZ_ASSERT()s?

Ex: if mRedirectChannelId==0 and 'succeeded=true' for some reason, we'll wind up blindly setting mActiveChannel = redirectChannel, even though that probably couldn't work.

::: netwerk/test/unit/test_redirect_different-protocol.js
@@ +5,5 @@
> +});
> +
> +var httpServer = null;
> +// Need to randomize, because apparently no one clears our cache
> +var randomPath = "/redirect/" + Math.random();

is that still true?  I thought each xpcshell test run got its own profile, so a new cache each time.  I could be totally wrong though.

@@ +30,5 @@
> +function redirectHandler(metadata, response)
> +{
> +  response.setStatusLine(metadata.httpVersion, 301, "Moved");
> +  response.bodyOutputStream.write(redirectBody, redirectBody.length);
> +  response.setHeader("Location", "data:text/plain," + responseBody, false);

So 'redirectBody' is the content of the page that does the redirect, and 'responseBody' is the redirected-to content? The names confused me.  Maybe change to 'redirectingFromContent' and 'redirectedToContent'?

@@ +36,5 @@
> +
> +function finish_test(request, buffer)
> +{
> +  if (inChildProcess())
> +    do_check_eq(buffer, redirectBody); // until bug 590682 is fixed

make comment clearer:

   // redirects to protocols other than http/ftp will fail until bug 590682 is fixed.

@@ +50,5 @@
> +  httpServer.registerPathHandler(randomPath, redirectHandler);
> +  httpServer.start(-1);
> +
> +  var chan = make_channel(randomURI);
> +  // chan.notificationCallbacks = new ChannelEventSink(ES_ABORT_REDIRECT);

I assume you want to remove this commented line.

Comment 4

[Jason Duell]

Note that we'll be doing the complete fix (to properly support http->rstp redirects) in bug 949675.  This is just to fix the crash.

Comment 5

[Honza Bambas (:mayhemer)]

(In reply to Jason Duell (:jduell) from comment #3)
> ::: netwerk/test/unit/test_redirect_different-protocol.js
> @@ +5,5 @@
> > +});
> > +
> > +var httpServer = null;
> > +// Need to randomize, because apparently no one clears our cache
> > +var randomPath = "/redirect/" + Math.random();
> 
> is that still true?  I thought each xpcshell test run got its own profile,
> so a new cache each time.  I could be totally wrong though.

Each test has it's own profile, but to save me some work, I'll leave this as is for now.

Comment 6

[Honza Bambas (:mayhemer)]

Attachment: v1.1 [as landed]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc25308dfeb9

Comment 7

[Honza Bambas (:mayhemer)]

Jason, should this fix be uplifted to any branch of b2g?

Comment 8

[Honza Bambas (:mayhemer)]

Since there are some inbound oranges, I rather have repushed the slightly modified patch to try:

https://tbpl.mozilla.org/?tree=Try&rev=6da4b23464f8

Comment 9

[Jason Smith [:jsmith]]

(In reply to Honza Bambas (:mayhemer) from comment #7)
> Jason, should this fix be uplifted to any branch of b2g?

Probably - it was originally reproduced on Gecko 27. We should ask for aurora approval.

Comment 10

[Honza Bambas (:mayhemer)]

Comment on attachment 8359803 [details] [diff] [review]
v1.1 [as landed]

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 765934
User impact if declined: crash of an app on b2g
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): relatively low, the change is quite simple and straightforward, and has a test coverage (in the patch)
String or IDL/UUID changes made by this patch: none

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/fc25308dfeb9

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/17e6923abc9f