251969 - FF10PR1 M17x crash [@ nsHttpChannel::OnStopRequest ]

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Adam Hauner]

I met this crash while submitting two forms in different tabs (1.7/W2K). One of
them was successfully finished, data was sent on server, second was just search...

TalkBack server has info about 120 crashes across M17, Trunk, Ff and Tb with
this signature:
http://talkback-public.mozilla.org/talkback/fastfind.jsp?search=1&searchby=stacksig&match=contains&searchfor=nsHttpChannel%3A%3AOnStopRequest

I can't find any open/fixed bug with this signature.

TB355155 (my crash):
0x65757274
nsHttpChannel::OnStopRequest
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/netwerk/protocol/http/src/nsHttpChannel.cpp,
line 3535]
nsInputStreamPump::OnStateStop
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/netwerk/base/src/nsInputStreamPump.cpp,
line 499]
nsInputStreamPump::OnInputStreamReady
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/netwerk/base/src/nsInputStreamPump.cpp,
line 340]
nsInputStreamReadyEvent::EventHandler
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/io/nsStreamUtils.cpp,
line 215]
PL_HandleEvent
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c,
line 674]
PL_ProcessPendingEvents
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c,
line 612]
_md_EventReceiverProc
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpcom/threads/plevent.c,
line 1415]
nsAppShellService::Run
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpfe/appshell/src/nsAppShellService.cpp,
line 524]
main1
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1313]
main
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1783]
WinMain
[d:/BUILDS/tinderbox/Mozilla1.7/WINNT_5.0_Clobber/mozilla/xpfe/bootstrap/nsAppRunner.cpp,
line 1809]
WinMainCRTStartup()
KERNEL32.DLL + 0x11af6 (0x7c581af6)

Comment 1

[Nicolas Mailhot]

See also http://bugzilla.gnome.org/show_bug.cgi?id=149395

Comment 2

[timeless]

we're hitting this in a deployed environment.

Comment 3

[Darin Fisher]

Attachment: v1 patch

This patch may fix the crash.

I believe the problem is caused by the NS_RELEASE(mConnection) that occurs
inside nsHttpTransaction::Close, which can run on the socket transport thread. 
As a result, the code in nsHttpChannel::OnStopRequest can get into trouble when
it tries to access mTransaction's mConnection variable. 
nsHttpTransaction::Close keeps mConnection around if the STICKY_CONNECTION flag
was set at initialization time.  So, this patch checks for that flag, and only
tries to access mConnection if that flag is set.

Comment 4

[timeless]

Comment on attachment 159643 [details] [diff] [review]
v1 patch

this works

Comment 5

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 159643 [details] [diff] [review]
v1 patch

sr=bzbarsky

Comment 6

[Darin Fisher]

fixed-on-trunk

Comment 7

[Darin Fisher]

Attachment: v2 followup patch

Thanks to dveditz for asking questions about this patch.  Together we found
another edge case that could still trigger the crash.  By not testing the value
of |status|, we could end up accessing the connection when it was not saved. 
nsHttpTransaction::Close closes the connection if |status| is a failure code.

This patch also adds more documentation.

Comment 8

[Darin Fisher]

I realized after checking in this patch on the trunk that I had done so on a
frozen trunk.  Oops! :(

I will ask drivers for 1.8a4 approval.

Comment 9

[Daniel Veditz [:dveditz]]

Comment on attachment 159883 [details] [diff] [review]
v2 followup patch

>-        if (mCaps & NS_HTTP_STICKY_CONNECTION)
>+        if (authRetry && (mCaps & NS_HTTP_STICKY_CONNECTION))

This is the only real difference between v1 and v2 (to catch a potential, but
we think improbable, problem if Close() was called with a failing status). The
rest is comment and a new var for code clarity. I think it's fair to carry over
r= and grant sr=dveditz

And since my original involvement was an a= request, a=dveditz for the 1.7.x
branch. Please add the fixed1.7.x keyword when this is checked in to the
branch.

Comment 10

[Darin Fisher]

Comment on attachment 159883 [details] [diff] [review]
v2 followup patch

need this on aviary too

Comment 11

[Darin Fisher]

v2 patch fixed-on-trunk

Comment 12

[Darin Fisher]

fixed1.7.x

Comment 13

[Jay Patel [:jay]]

Adding FF10PR1 and M17x so summary for tracking.

Comment 14

[Asa Dotzler [:asa]]

Comment on attachment 159883 [details] [diff] [review]
v2 followup patch

a=asa for aviary checkin.

Comment 15

[Darin Fisher]

fixed-aviary1.0

Comment 16

[Darin Fisher]

*** Bug 257639 has been marked as a duplicate of this bug. ***