Closed Bug 904571 Opened 6 years ago Closed 6 years ago

crash in mozilla::net::HttpChannelParentListener::OnRedirectResult

Categories

(Core :: Networking: HTTP, defect, critical)

25 Branch
defect
Not set
critical

Tracking

()

RESOLVED FIXED
mozilla30
Tracking Status
firefox24 --- unaffected
firefox25 --- affected
firefox26 --- affected

People

(Reporter: scoobidiver, Assigned: mayhemer)

References

Details

(Keywords: crash, regression)

Crash Data

Attachments

(2 files)

It first showed up in 25.0a1/20130725. The regression range might be (low volume):
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2983ca6d4d1a&tochange=a4c1961bf723

Signature 	mozilla::net::HttpChannelParentListener::OnRedirectResult(bool) More Reports Search
UUID 	ce7b3e6c-f56c-49d0-82a1-0ba792130811
Date Processed	2013-08-11 06:57:34.721808
Uptime	10
Install Age 	6035 since version was first installed.
Install Time 	2013-08-11 05:16:52
Product 	Firefox
Version 	26.0a1
Build ID 	20130810030206
Release Channel 	nightly
OS 	Windows NT
OS Version 	6.1.7600
Build Architecture 	x86
Build Architecture Info 	GenuineIntel family 6 model 58 stepping 9 | 4
Crash Reason 	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address 	0x0
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x0152, AdapterSubsysID: 00000000, AdapterDriverVersion: 8.15.10.2669
D2D? D2D+ DWrite? DWrite+ D3D10 Layers? D3D10 Layers+ 

Frame 	Module 	Signature 	Source
0 	xul.dll 	mozilla::net::HttpChannelParentListener::OnRedirectResult(bool) 	netwerk/protocol/http/HttpChannelParentListener.cpp
1 	xul.dll 	mozilla::net::AutoRedirectVetoNotifier::ReportRedirectResult(bool) 	netwerk/protocol/http/nsHttpChannel.cpp
2 	xul.dll 	mozilla::net::nsHttpChannel::ContinueProcessRedirection(tag_nsresult) 	netwerk/protocol/http/nsHttpChannel.cpp
3 	xul.dll 	mozilla::net::nsHttpChannel::OnRedirectVerifyCallback(tag_nsresult) 	netwerk/protocol/http/nsHttpChannel.cpp
4 	xul.dll 	nsAsyncVerifyRedirectCallbackEvent::Run() 	netwerk/base/src/nsAsyncRedirectVerifyHelper.cpp
5 	xul.dll 	nsThread::ProcessNextEvent(bool,bool *) 	xpcom/threads/nsThread.cpp
6 	xul.dll 	NS_ProcessNextEvent(nsIThread *,bool) 	obj-firefox/xpcom/build/nsThreadUtils.cpp
7 	xul.dll 	nsHttpConnectionMgr::Shutdown() 	netwerk/protocol/http/nsHttpConnectionMgr.cpp
8 	xul.dll 	nsHttpHandler::Observe(nsISupports *,char const *,wchar_t const *) 	netwerk/protocol/http/nsHttpHandler.cpp
9 	xul.dll 	nsObserverService::NotifyObservers(nsISupports *,char const *,wchar_t const *) 	xpcom/ds/nsObserverService.cpp
10 	xul.dll 	nsXREDirProvider::DoShutdown() 	toolkit/xre/nsXREDirProvider.cpp
11 	xul.dll 	ScopedXPCOMStartup::~ScopedXPCOMStartup() 	toolkit/xre/nsAppRunner.cpp
12 	xul.dll 	XREMain::XRE_main(int,char * * const,nsXREAppData const *) 	toolkit/xre/nsAppRunner.cpp
13 	xul.dll 	XRE_main 	toolkit/xre/nsAppRunner.cpp

More reports at:
https://crash-stats.mozilla.com/report/list?product=Firefox&signature=mozilla%3A%3Anet%3A%3AHttpChannelParentListener%3A%3AOnRedirectResult%28bool%29
Could be related to bug 904559.

I'll do a workaround but also check how this can even happen.
Assignee: nobody → honzab.moz
Status: NEW → ASSIGNED
When there is no activeRedirectingChannel QI'ed out of mActiveChannel, we should turn succeeded to false and bypass call on the null activeRedirectingChannel.

I think there is nothing more to do here.  We will just show the content of the 30X response page and not crash.
Attached patch v1Splinter Review
My recent re-experience with this code tells me there is nothing more to do.
Attachment #8365135 - Flags: review?(jduell.mcbugs)
See Also: → 904559
Comment on attachment 8365135 [details] [diff] [review]
v1

According https://bugzilla.mozilla.org/show_bug.cgi?id=904559#c4 we may want to remove the assert as well, what do you thing Jason?
Jason, ping on review here.
Comment on attachment 8365135 [details] [diff] [review]
v1

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

I can see that this patch probably removes the crash, but it still seems we have an underlying bug that we're not dealing with yet?

The issue here is that mActiveChannel is somehow not a nsIParentRedirectingChannel (in other words, it's not an HttpChannelParent--nothing else implements it AFAICT).  HttpChannelParentListener is *always* created with an initial mActiveListener == HttpChannelParent, and the only way that can change is in OnRedirectResult, if we're redirected to FTP (that's the only case so far, right).

That can happen, of course--but then how exactly does an HttpChannelParentListener with a FTPChannelParent mActiveListener ever run across AsyncOnChannelRedirect and/or OnRedirectResult again?  FTP doesn't do redirects.  

So it seems to me that we're getting an "extra" call to OnRedirectResult here.  The mostly likely cause could be that we accidentally have 2 AutoRedirectVetoNotifier's on the stack somehow?  The nsHttpChannel code here is not easy to follow--seems like we could maybe try to do something stupid like add a nsHttpChannel.mHasHadVetoNotifier member variable, set it when we use an AutoRedirectVetoNotifier, and ASSERT that it's false?  Then maybe we can catch the stack trace if we somehow call OnRedirectResult twice.

Maybe I'm totally wrong here--but I can't think of how else OnRedirectResult is getting called when mActiveChannel isn't an HTTP channel.

If we somehow can't figure this out, or there's a valid reason why mActiveChannel could be something else here, I'm ok with this patch.

And it looks like bug 904559 is the same thing, except we're seeing AsyncOnChannelRedirect() get called, which seems even stranger--how can it get called again after we've switched mActiveChannel to an FTP channel?

BTW there are two asserts that you'd need to remove for this patch--one in AsyncOnChannelRedirect and one in OnRedirectResult.
Attachment #8365135 - Flags: review?(jduell.mcbugs) → feedback+
(In reply to Jason Duell (:jduell) from comment #7)
> Comment on attachment 8365135 [details] [diff] [review]
> v1
> 
> Review of attachment 8365135 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I can see that this patch probably removes the crash, but it still seems we
> have an underlying bug that we're not dealing with yet?
> 
> The issue here is that mActiveChannel is somehow not a
> nsIParentRedirectingChannel (in other words, it's not an
> HttpChannelParent--nothing else implements it AFAICT). 
> HttpChannelParentListener is *always* created with an initial
> mActiveListener == HttpChannelParent, and the only way that can change is in
> OnRedirectResult, if we're redirected to FTP (that's the only case so far,
> right).

I think extensions may override http channel implementation and hence the active channel is not our nsHttpChannel.

> 
> That can happen, of course--but then how exactly does an
> HttpChannelParentListener with a FTPChannelParent mActiveListener ever run
> across AsyncOnChannelRedirect and/or OnRedirectResult again?  FTP doesn't do
> redirects.  

As mentioned above, that is not an FTP.

> 
> So it seems to me that we're getting an "extra" call to OnRedirectResult
> here.  The mostly likely cause could be that we accidentally have 2
> AutoRedirectVetoNotifier's on the stack somehow?  

Hmm.. I think not, but I'll check this, good idea.

> The nsHttpChannel code
> here is not easy to follow--seems like we could maybe try to do something
> stupid like add a nsHttpChannel.mHasHadVetoNotifier member variable, set it
> when we use an AutoRedirectVetoNotifier, and ASSERT that it's false?  Then
> maybe we can catch the stack trace if we somehow call OnRedirectResult twice.
> 
> Maybe I'm totally wrong here--but I can't think of how else OnRedirectResult
> is getting called when mActiveChannel isn't an HTTP channel.
> 
> If we somehow can't figure this out, or there's a valid reason why
> mActiveChannel could be something else here, I'm ok with this patch.
> 
> And it looks like bug 904559 is the same thing, except we're seeing
> AsyncOnChannelRedirect() get called, which seems even stranger--how can it
> get called again after we've switched mActiveChannel to an FTP channel?

Again, not an FTP channel :)

> 
> BTW there are two asserts that you'd need to remove for this patch--one in
> AsyncOnChannelRedirect and one in OnRedirectResult.

Yes, that is what I've mentioned (or something like that) in https://bugzilla.mozilla.org/show_bug.cgi?id=904559#c4
(In reply to Honza Bambas (:mayhemer) from comment #8)
> > So it seems to me that we're getting an "extra" call to OnRedirectResult
> > here.  The mostly likely cause could be that we accidentally have 2
> > AutoRedirectVetoNotifier's on the stack somehow?  
> 
> Hmm.. I think not, but I'll check this, good idea.

There is a potential for foreign (unknown) code be called while AutoRedirectVetoNotifier is on stack.  So it's actually not possible to say whether it can happen to be multiple times on the stack or not.  I think we can easily use mRedirectChannel of the channel to indicate whether we have called OnRedirectResult or not.  ~AutoRedirectVetoNotifier drops it from the channel (I've added an assert to check it's always non-null there and xpcshell tests that cover all codepaths for this class).  But a flag might be more reliable and also we'd have more control over the behavior...  

Question is, what happens when the nested notifier's dtor is called with FAILED and the other is called with SUCCEEDED.  Which one is right?  But I think this is a broader problem, since in this case the code would be seriously broken.  More arg for a flag...
Attached patch notifier flagSplinter Review
xpcshells locally passing.

https://tbpl.mozilla.org/?tree=Try&rev=4487989ea3ae
Attachment #8373536 - Flags: review?(jduell.mcbugs)
Attachment #8373536 - Flags: review?(jduell.mcbugs) → review+
Attachment #8365135 - Flags: feedback+ → review+
Covered by a test landed in bug 949667.
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/615c92a67089
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.