813489 - crash in nsHttpTransaction::SetSecurityCallbacks

Status: RESOLVED FIXED

(Core :: Networking: HTTP, defect)

Description

[Scoobidiver (away)]

This bug tracks crashes not fixed by bug 812203.
It first showed up in 19.0a1/20121115. The regression range is:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=dd68409d7810&tochange=a761bfc192b5
It's likely a regression from bug 804655.

Signature 	nsHttpTransaction::SetSecurityCallbacks(nsIInterfaceRequestor*) More Reports Search
UUID	29f875ef-9dd2-41ad-bb10-53a262121120
Date Processed	2012-11-20 02:27:37
Uptime	171
Last Crash	3.4 minutes before submission
Install Age	1.9 hours since version was first installed.
Install Time	2012-11-20 00:33:11
Product	Firefox
Version	19.0a1
Build ID	20121116030725
Release Channel	nightly
OS	Windows NT
OS Version	6.1.7601 Service Pack 1
Build Architecture	x86
Build Architecture Info	GenuineIntel family 6 model 15 stepping 13
Crash Reason	EXCEPTION_ACCESS_VIOLATION_READ
Crash Address	0x0
App Notes 	
AdapterVendorID: 0x8086, AdapterDeviceID: 0x2a42, AdapterSubsysID: 30e8103c, AdapterDriverVersion: 8.15.10.1749
D3D10 Layers? D3D10 Layers- D3D9 Layers? D3D9 Layers- 
EMCheckCompatibility	True
Adapter Vendor ID	0x8086
Adapter Device ID	0x2a42
Total Virtual Memory	2147352576
Available Virtual Memory	1812946944
System Memory Use Percentage	60
Available Page File	2216480768
Available Physical Memory	820977664

Frame 	Module 	Signature 	Source
0 	xul.dll 	nsHttpTransaction::SetSecurityCallbacks 	netwerk/protocol/http/nsHttpTransaction.cpp:409
1 	xul.dll 	mozilla::net::nsHttpChannel::UpdateAggregateCallbacks 	netwerk/protocol/http/nsHttpChannel.cpp:5938
2 	xul.dll 	mozilla::net::nsHttpChannel::SetNotificationCallbacks 	netwerk/protocol/http/nsHttpChannel.cpp:5960
3 	xul.dll 	imgRequest::Init 	image/src/imgRequest.cpp:152
4 	xul.dll 	imgCacheValidator::OnStartRequest 	image/src/imgLoader.cpp:2182
5 	xul.dll 	mozilla::net::nsHttpChannel::CallOnStartRequest 	netwerk/protocol/http/nsHttpChannel.cpp:953
6 	xul.dll 	mozilla::net::nsHttpChannel::InitCacheEntry 	netwerk/protocol/http/nsHttpChannel.cpp:3566
7 	xul.dll 	xul.dll@0x2cbd4f 	
8 	xul.dll 	mozilla::net::nsHttpChannel::ProcessNormal 	netwerk/protocol/http/nsHttpChannel.cpp:1379
9 	xul.dll 	nsHttpChannelAuthProvider::Release 	netwerk/protocol/http/nsHttpChannelAuthProvider.cpp:1430
10 	xul.dll 	mozilla::net::nsHttpChannel::ProcessResponse 	netwerk/protocol/http/nsHttpChannel.cpp:1214
11 	xul.dll 	mozilla::net::nsHttpChannel::OnStartRequest 	netwerk/protocol/http/nsHttpChannel.cpp:4808
12 	xul.dll 	nsInputStreamPump::OnStateStart 	netwerk/base/src/nsInputStreamPump.cpp:417
13 	xul.dll 	nsInputStreamPump::OnInputStreamReady 	netwerk/base/src/nsInputStreamPump.cpp:368
14 	xul.dll 	nsInputStreamReadyEvent::Run 	xpcom/io/nsStreamUtils.cpp:82
15 	xul.dll 	nsThread::ProcessNextEvent 	xpcom/threads/nsThread.cpp:627
16 	xul.dll 	mozilla::ipc::MessagePump::Run 	ipc/glue/MessagePump.cpp:117
...

More reports at:
https://crash-stats.mozilla.com/report/list?signature=nsHttpTransaction%3A%3ASetSecurityCallbacks%28nsIInterfaceRequestor*%29

Comment 1

[Honza Bambas (:mayhemer)]

We may have to protect mConnection as well by a lock.  Apparently it can be written (nullified) by a different thread.

Comment 2

[Josh Matthews [:jdm]]

Attachment: Protect HTTP transaction's connection member from being changed by other threads.

Comment 3

[Honza Bambas (:mayhemer)]

When I'm looking at the stacktrace, it seems like imglib is doing something very wrong already.  Maybe before having bug 804655 fixed this could be more or less OK.  However, now I don't believe we should change callbacks to be imgRequest for the complete channel-transaction-connection-transport+ssl socket chain, where the lower level objects are shared.

This is example of why I believe security callbacks are simply a bad or incomplete design and why I'm not a big fan of having bug 804655 in the tree.

Comment 4

[Josh Matthews [:jdm]]

I can do some further testing with regards to backing out bug 804655; bug 804653 may have eliminated the visible effect that caused me to file the problem with the connection/transaction callbacks in the first place.

Comment 5

[Honza Bambas (:mayhemer)]

Comment on attachment 684490 [details] [diff] [review]
Protect HTTP transaction's connection member from being changed by other threads.

Based on comment 4 I'm dropping r for now.

Josh, please rerequest when there is no other way then having a patch like this.

Comment 6

[Honza Bambas (:mayhemer)]

Comment on attachment 684490 [details] [diff] [review]
Protect HTTP transaction's connection member from being changed by other threads.

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

Few comments:

Please never call method of mConnection under this new lock.

Have GetConnectionLocked() method (feel free to suggest a better name) that is copying mConnection to a local var under the lock and then work with the local var.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +393,1 @@
>      NS_IF_RELEASE(mConnection);

Do not do the actual release under the lock please.

Since this is on more places in the code, you may want to have a ReleaseConnectionLocked() method that does the actual release out of the lock and wipes mConnection.

ReleaseConnectionLocked() may however not be the best approach for this particular method (SetConnection).  Better to swap mConneciton to a local var and let it release out of the lock.

Comment 7

[Josh Matthews [:jdm]]

Honza, I have confirmed that backing out the changes from bug 804655 does not regress the original problem that caused me to file it. I'm totally fine with doing that, but Christian was concerned when I asked about the original problem and stated that the connection/transaction's callbacks not updating was a definite mistake.

Comment 8

[Christian :Biesinger (don't email me, ping me on IRC)]

so, instead of adding locking, how about just posting a runnable to the socket thread to update the notification callbacks?

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

just suggesting it because that sounds a lot simpler/faster than this code.

Comment 10

[Josh Matthews [:jdm]]

Attachment: Update the HTTP connection's security callbacks from the socket thread.

Try push at https://tbpl.mozilla.org/?tree=Try&rev=d528ddf769f1

Comment 11

[Scoobidiver (away)]

It's #18 top browser crasher in 19.0a2.

Comment 12

[Honza Bambas (:mayhemer)]

Comment on attachment 685362 [details] [diff] [review]
Update the HTTP connection's security callbacks from the socket thread.

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

Nice!  I checked that nsHttpTransaction::mConnection is being moved only on the socket thread.  There should be a comment added to the header if there not already one about that.

::: netwerk/protocol/http/nsHttpTransaction.cpp
@@ +424,5 @@
>          MutexAutoLock lock(mCallbacksLock);
>          mCallbacks = aCallbacks;
>      }
> +
> +    nsCOMPtr<nsIRunnable> event = new UpdateSecurityCallbacks(this, aCallbacks);

nsRefPtr<UpdateSecurityCallbacks> please to save QI.

@@ +425,5 @@
>          mCallbacks = aCallbacks;
>      }
> +
> +    nsCOMPtr<nsIRunnable> event = new UpdateSecurityCallbacks(this, aCallbacks);
> +    gSocketTransportService->Dispatch(event, nsIEventTarget::DISPATCH_NORMAL);

Null check on gSocketTransportService please.

::: netwerk/protocol/http/nsHttpTransaction.h
@@ +135,5 @@
>  
>      bool TimingEnabled() const { return mCaps & NS_HTTP_TIMING_ENABLED; }
>  
>  private:
> +    friend class UpdateSecurityCallbacks;

Could it be a nested class?

Comment 13

[Lukas Blakk [:lsblakk] use ?needinfo]

tracking based on comment 11

Comment 14

[Josh Matthews [:jdm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84f3936e91a7

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/84f3936e91a7

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

Please request uplift approval for 19.

Comment 17

[Josh Matthews [:jdm]]

Comment on attachment 685362 [details] [diff] [review]
Update the HTTP connection's security callbacks from the socket thread.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 804655
User impact if declined: bug 819843
Testing completed (on m-c, etc.): Crashes have disappeared on socorro for nightly
Risk to taking this patch (and alternatives if risky): Low, and easy to backout if catastrophe occurs. 
String or UUID changes made by this patch: None

Comment 18

[bhavana bajaj [:bajaj]]

Comment on attachment 685362 [details] [diff] [review]
Update the HTTP connection's security callbacks from the socket thread.

low risk patch for a top crasher.Approving on aurora.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/3e9449cd4df4

Comment 20

[Ioana (away)]

Apparently this bug hasn't been completely fixed:
[@ nsHttpTransaction::SetSecurityCallbacks(nsIInterfaceRequestor*)] - No recent crashes https://crash-stats.mozilla.com/report/list?query_search=signature&query_type=contains&reason_type=contains&range_value=4&range_unit=weeks&hang_type=any&process_type=any&signature=nsHttpTransaction%3A%3ASetSecurityCallbacks%28nsIInterfaceRequestor%2A%29

[@ RtlEnterCriticalSection | PR_Lock | mozilla::BaseAutoLock<mozilla::Mutex>::BaseAutoLock<mozilla::Mutex>(mozilla::Mutex&)] - One recent crash on the 01/21 Nightly https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=RtlEnterCriticalSection%20%7C%20PR_Lock%20%7C%20mozilla%3A%3ABaseAutoLock%26lt%3Bmozilla%3A%3AMutex%26gt%3B%3A%3ABaseAutoLock%26lt%3Bmozilla%3A%3AMutex%26gt%3B%28mozilla%3A%3AMutex%26amp%3B%29&reason_type=contains&date=01%2F31%2F2013%2013%3A09%3A04&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=RtlEnterCriticalSection%20%7C%20PR_Lock%20%7C%20mozilla%3A%3ABaseAutoLock%3Cmozilla%3A%3AMutex%3E%3A%3ABaseAutoLock%3Cmozilla%3A%3AMutex%3E%28mozilla%3A%3AMutex%26%29

[@ RtlEnterCriticalSection | PR_Lock | mozilla::MonitorAutoLock::MonitorAutoLock(mozilla::Monitor&)] - 6 recent crashes on Firefox 19 beta (2 & 3) https://crash-stats.mozilla.com/report/list?product=Firefox&query_search=signature&query_type=contains&query=RtlEnterCriticalSection%20%7C%20PR_Lock%20%7C%20mozilla%3A%3AMonitorAutoLock%3A%3AMonitorAutoLock%28mozilla%3A%3AMonitor%26amp%3B%29&reason_type=contains&date=01%2F31%2F2013%2013%3A09%3A27&range_value=4&range_unit=weeks&hang_type=any&process_type=any&do_query=1&signature=RtlEnterCriticalSection%20%7C%20PR_Lock%20%7C%20mozilla%3A%3AMonitorAutoLock%3A%3AMonitorAutoLock%28mozilla%3A%3AMonitor%26%29

Comment 21

[u279076]

Scoobidiver, any thoughts on comment 20?

Comment 22

[Scoobidiver (away)]

(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #21)
> Scoobidiver, any thoughts on comment 20?
See bug 812203 comment 10.

Comment 23

[u279076]

Thanks Scoobidiver. Given the reduction in volume I'm marking this verified. Please add the verifyme keyword if there's something more you'd like tested.