+++ 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.

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

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 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.

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.

(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.

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

Since there are some inbound oranges, I rather have repushed the slightly modified patch to try: https://tbpl.mozilla.org/?tree=Try&rev=6da4b23464f8

(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 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