983910 - crash in mozilla::net::HttpChannelParentListener::SuspendForDiversion()

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Rachel Pribble [:rpribble]]

This bug was filed from the Socorro interface and is 
report bp-db0efc4f-9bac-452f-a0bd-2e1c12140315.
=============================================================

Repro steps:
I believe I had just launched the browser for exploratory testing.

Comment 1

[Jason Smith [:jsmith]]

Any STR?

Comment 2

[Rachel Pribble [:rpribble]]

I believe I had just opened the browser and/or the Notes app. I was opening and flipping through windows for exploratory testing. Unfortunately I don't have anything more specific.

Comment 3

[Jason Smith [:jsmith]]

Okay - putting steps-wanted then to have someone investigate whether they can repro.

Putting needinfo on Sarah as this is in Core, so it won't show up in our queries to find an assignee for this QA request.

Comment 4

[Sarah Parsons]

Setting rpribble as QA Contact to run exploratory testing around this issue and narrow down STR.

Comment 5

[Sarah Parsons]

We were unable to repro this issue on the latest Buri 1.4 Build ID: 20140317040204

BuildID: 20140317040204
Gaia: 8f802237927c7d5e024fb7dca054dd5efef6b2e6
Gecko: 25cfa01ba054
Version: 30.0a1
Firmware Version: v1.2-device-cfg

Comment 6

[Jason Duell]

The only code that should be causing this codepath to run is when you kick off a download.  So if you're trying to repro (which would be great), please make sure to try that.

I don't trust the stack trace here--HttpChannelParentListener::SuspendForDiversion literally does nothig besides set a boolean member variable, so it shouldn't cause a crash unless we're calling into a deallocated object or something (and even then the page would have to have been unmapped).

Comment 7

[Rachel Pribble [:rpribble]]

I was able to reproduce this trying to access a marketplace app through a bit.ly link in the browser.

Environmental Variables:
Device: Buri v1.4 MOZ ril
BuildID: 20140319000200
Gaia: c03a6af9028c4b74a84b5a98085bbb0c07261175
Gecko: b07ecc057abf
Version: 30.0a2
Firmware Version: v1.2-device.cfg

Repro Steps:
1) Updated Buri to BuildID: 20140319000200
2) Tap the browser icon to launch
3) Enter http://mzl.la/1hYzDSQ into the address bar (bit.ly link to Loja de Serviços Vivo app in marketplace)
4) Observe the device crash

Repro rate: 100%

Comment 9

[Jason Smith [:jsmith]]

Looks like the dupe & comment 7 gives us something actionable to work off of.

Comment 10

[Preeti Raghunath(:Preeti)]

QA,

Is this reproducible in 1.3? I understand its a regression from 1.2 but wondering if 1.3 is impacted as well.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Preeti Raghunath(:Preeti) from comment #10)
> QA,
> 
> Is this reproducible in 1.3? I understand its a regression from 1.2 but
> wondering if 1.3 is impacted as well.

Will check 1.3, but I doubt it's reproducing there.

Comment 12

[bhavana bajaj [:bajaj]]

QA,Please renominate once we find if this is reproducible(or not) on 1.3

Comment 13

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

Looking at socorro this crash started on the 3/13/2013 and happens on 1.4/1.5 not on 1.3; it looks like a new crasher

Comment 14

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

https://crash-stats.mozilla.com/report/list?product=B2G&range_unit=days&range_value=14&signature=mozilla%3A%3Anet%3A%3AHttpChannelParentListener%3A%3ASuspendForDiversion%28%29#tab-reports

Comment 15

[Preeti Raghunath(:Preeti)]

1.4+ for reproducible crash on 1.4

Comment 16

[Jason Duell]

Steve, see if you can reproduce on e10s desktop and if not, let's get you a phone build.

It looks like mParentListener is null here--the only thing HttpChannelParentListener::SuspendForDiversion touches is mSuspendedForDiversion, and we're getting an exception at address 0x20, which is a plausible offset for the member variable.

I see we set mParentListener to null in both HttpChannelParent::RecvDivertComplete and HttpChannelParent::NotifyDiversionFailed.  Maybe something is calling Divert from the child twice?

Comment 17

[Jason Smith [:jsmith]]

FWIW - this should be possible to reproduce on Desktop under e10s, as there's a few comments in some of the crash reports that seem to imply that:

Comments

* OS X 10.8.5 - downloading file in e10s crashes
* OS X 10.8.5 - Downloading file from GMail crashes on e10s.
* OS X 10.8.5 - crashed when I try to download attachment file from GMail on e10s tab.

Comment 18

[Steve Workman [:sworkman] (INACTIVE)]

Reproducible on desktop e10s; debugging now :)

Comment 20

[Steve Workman [:sworkman] (INACTIVE)]

Jason, this is to do with redirects. HttpChannelParent::mParentListener is normally init'd in DoAsyncOpen, but that isn't called in the redirect case. I'm looking now to see where I can inject a line or two to initialize it.

Comment 21

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.0 Set mParentListener for HttpChannelParent objects created after HTTP redirects

Sets HttpChannelParent::mParentListener in OnStartRequest if it is not already set.
For HTTP redirects, OnStartRequest is the first function called in HttpChannelParent after ConnectChannel (at least in normal functioning). By calling GetListener here and using downcast<T>(), I'm trying to avoid specific divert-after-redirect changes to the class/IDL interfaces. I think this patch keeps things nicely localized, even if there is a cast involved.

Note: nsHttpChannel.h seems to have several code styles. I've tried to match up with functions defined inline like SetReferrerInternal. (Yup, pre-emptive nit-defense :) ).

Comment 22

[Jason Duell]

Comment on attachment 8396038 [details] [diff] [review]
v1.0 Set mParentListener for HttpChannelParent objects created after HTTP redirects

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

::: netwerk/protocol/http/nsHttpChannel.h
@@ +185,5 @@
>  
>      void ForcePending(bool aForcePending);
>  
> +    already_AddRefed<nsIStreamListener> GetListener() {
> +        return nsCOMPtr<nsIStreamListener>(mListener).forget();

I don't understand why you're using forget().  That will null out the nsHttpChannel's pointer to the ParentListener, and break OnData/Stop, no?  It's fine to keep separate references to the ParentListener in both nsHttpChannel and HttpChannelParent.

Comment 23

[Steve Workman [:sworkman] (INACTIVE)]

(In reply to Jason Duell (:jduell) from comment #22)
> > +    already_AddRefed<nsIStreamListener> GetListener() {
> > +        return nsCOMPtr<nsIStreamListener>(mListener).forget();
> 
> I don't understand why you're using forget().  That will null out the
> nsHttpChannel's pointer to the ParentListener, and break OnData/Stop, no? 
> It's fine to keep separate references to the ParentListener in both
> nsHttpChannel and HttpChannelParent.

The forget is not for the mListener, but the nsCOMPtr<nsIStreamListener> that's created in this statement. mListener should be untouched, but we should get an AddRef here and then just return the pointer in the already_AddRefed. No?

Comment 24

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v1.1 Set mParentListener for HttpChannelParent objects created after HTTP redirects

Just to expedite things, I've added a temporary stack variable in nsHttpChannel::GetListener(). Should be clearer now.

Comment 25

[Jason Duell]

Comment on attachment 8396038 [details] [diff] [review]
v1.0 Set mParentListener for HttpChannelParent objects created after HTTP redirects

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

::: netwerk/protocol/http/nsHttpChannel.h
@@ +185,5 @@
>  
>      void ForcePending(bool aForcePending);
>  
> +    already_AddRefed<nsIStreamListener> GetListener() {
> +        return nsCOMPtr<nsIStreamListener>(mListener).forget();

OK, so I misread this.

Comment 26

[Steve Workman [:sworkman] (INACTIVE)]

Thanks Jason!

https://hg.mozilla.org/integration/mozilla-inbound/rev/636fa3174368

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Backed out for xpcshell timeouts.
https://hg.mozilla.org/integration/mozilla-inbound/rev/3f5081e0dd76

https://tbpl.mozilla.org/php/getParsedLog.php?id=36751940&tree=Mozilla-Inbound

Comment 28

[Ryan VanderMeulen [:RyanVM]]

And B2G bustage. Please run this through Try before pushing again.

Comment 29

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.0 Set mParentListener for HttpChannelParent objects created after HTTP redirects

Previous patch (wrongly) assumed that nsHttpChannel::mListener would always be an HttpChannelParentListener in the case of a e10s HTTP redirect. In test_XHR_redirects.js the listener is an nsStreamListenerTee; could be other things, if I understand how addons/extensions can insert themselves into the nsIStreamListener chain.

New patch adds SetParentListener to nsIParentChannel, with a comment saying it is optional:
-- HttpChannelParent implements this by doing some checks and then setting mParentListener.
-- FTPChannelParent will do nothing since it doesn't need to know.
-- Function is called in HttpChannelParentListener::OnRedirectComplete when it has access to both the new and old HttpChannelParents.

If need be, I can code this up in a new interface; nsIParentChannel seems like the most suitable place to add the new method without doing that.

Note: test_XHR_redirects.js passes locally (Ubuntu 64) for e10s and non-e10s. And I also see a pass following the steps in comment 7.

try: https://tbpl.mozilla.org/?tree=Try&rev=60562546ccc4

Comment 30

[Jason Duell]

Comment on attachment 8397482 [details] [diff] [review]
v2.0 Set mParentListener for HttpChannelParent objects created after HTTP redirects

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

Looks good.  A couple nits.

::: netwerk/base/public/nsIParentChannel.idl
@@ +6,5 @@
>  
>  interface nsITabParent;
>  
> +%{C++
> +//#include "mozilla/net/ChannelDiverterChild.h"

remove commented out #include

::: netwerk/base/public/nsIParentRedirectingChannel.idl
@@ +11,5 @@
>  /**
>   * Implemented by chrome side of IPC protocols that support redirect responses.
>   */
>  
> +[scriptable, uuid(3ed1d288-5324-46ee-8a98-33ac37d1080b)]

no need to change uuid if you don't change IDL :)

Comment 31

[Steve Workman [:sworkman] (INACTIVE)]

Attachment: v2.1 Set mParentListener for HttpChannelParent objects created after HTTP redirects

-- Removed comment (oops - removed it from nsIDivertableChannel too).
-- Noticed nsIParentChannel is scriptable, so made the new method [noscript].
-- Re: IDL update: I just ran ./mach update-uuids which updated nsIParentRedirectingChannel because it inherits from nsIParentChannel. So what's the deal there with uuid updates?

Comment 32

[Jason Duell]

IRC can be your friend :)

<jduell> dbaron: if IDL "nsIFoo" inherits from a base IDL, and the base IDL changes, does the uuid of nsIFoo need to change too (since the interface at some level has changed?).  I'm guessing no 
<khuey> jduell: yes
<khuey> it does
<jduell> khuey: aha--thanks, good to know.
* jduell wonders if I've ever broken things in the past by not knowing that
<khuey> jduell: I think the mach update-uuids thing is supposed to do that for you
<khuey> although I don't think I've actually used it
<jduell> khuey: ooh, our tooling has gotten so sexy.
<jduell> khuey: does it not run automagically?
<khuey> no
<jduell> I take back the sexy comment then :)

Comment 33

[Steve Workman [:sworkman] (INACTIVE)]

Cool, thanks.

Try looks good.

Pushed to inbound again: https://hg.mozilla.org/integration/mozilla-inbound/rev/c8434e3d0563

Comment 34

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

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

Comment 35

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/260eebb49708

Comment 36

[Honza Bambas (:mayhemer)]

The SetParentListener method had to be on a new interface specific to HTTP channels...  Now every channel that wants to be redirectable to has to implement it as an empty method...