crash in nsHttpTransaction::SetSecurityCallbacks

RESOLVED FIXED in Firefox 19

Status

()

Core
Networking: HTTP
--
critical
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Scoobidiver (away), Assigned: jdm)

Tracking

({crash, regression, topcrash})

19 Branch
mozilla20
All
Windows 7
crash, regression, topcrash
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox19+ verified, firefox20 verified)

Details

(crash signature)

Attachments

(2 attachments)

(Reporter)

Description

5 years ago
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
We may have to protect mConnection as well by a lock.  Apparently it can be written (nullified) by a different thread.
(Assignee)

Comment 2

5 years ago
Created attachment 684490 [details] [diff] [review]
Protect HTTP transaction's connection member from being changed by other threads.
Attachment #684490 - Flags: review?(honzab.moz)
(Assignee)

Updated

5 years ago
Assignee: nobody → josh
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.
(Assignee)

Comment 4

5 years ago
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.
(Reporter)

Updated

5 years ago
Crash Signature: [@ nsHttpTransaction::SetSecurityCallbacks(nsIInterfaceRequestor*)] → [@ nsHttpTransaction::SetSecurityCallbacks(nsIInterfaceRequestor*)] [@ RtlEnterCriticalSection | PR_Lock | mozilla::BaseAutoLock<mozilla::Mutex>::BaseAutoLock<mozilla::Mutex>(mozilla::Mutex&)]
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.
Attachment #684490 - Flags: review?(honzab.moz)
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.
(Assignee)

Comment 7

5 years ago
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.
so, instead of adding locking, how about just posting a runnable to the socket thread to update the notification callbacks?
just suggesting it because that sounds a lot simpler/faster than this code.
(Assignee)

Comment 10

5 years ago
Created attachment 685362 [details] [diff] [review]
Update the HTTP connection's security callbacks from the socket thread.

Try push at https://tbpl.mozilla.org/?tree=Try&rev=d528ddf769f1
Attachment #685362 - Flags: review?(honzab.moz)
(Reporter)

Comment 11

5 years ago
It's #18 top browser crasher in 19.0a2.
tracking-firefox19: --- → ?
Keywords: topcrash
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?
Attachment #685362 - Flags: review?(honzab.moz) → review+

Updated

5 years ago
Depends on: 818203
tracking based on comment 11
tracking-firefox19: ? → +
(Assignee)

Comment 14

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/84f3936e91a7
https://hg.mozilla.org/mozilla-central/rev/84f3936e91a7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
(Reporter)

Updated

5 years ago
Crash Signature: [@ nsHttpTransaction::SetSecurityCallbacks(nsIInterfaceRequestor*)] [@ RtlEnterCriticalSection | PR_Lock | mozilla::BaseAutoLock<mozilla::Mutex>::BaseAutoLock<mozilla::Mutex>(mozilla::Mutex&)] → [@ nsHttpTransaction::SetSecurityCallbacks(nsIInterfaceRequestor*)] [@ RtlEnterCriticalSection | PR_Lock | mozilla::BaseAutoLock<mozilla::Mutex>::BaseAutoLock<mozilla::Mutex>(mozilla::Mutex&)] [@ RtlEnterCriticalSection | PR_Lock | mozilla::Monit&hellip;
Please request uplift approval for 19.
(Assignee)

Comment 17

5 years ago
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
Attachment #685362 - Flags: approval-mozilla-aurora?
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.
Attachment #685362 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/3e9449cd4df4
status-firefox19: affected → fixed
status-firefox20: --- → fixed

Comment 20

5 years ago
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
Scoobidiver, any thoughts on comment 20?
(Reporter)

Comment 22

5 years ago
(In reply to Anthony Hughes, Mozilla QA (:ashughes) from comment #21)
> Scoobidiver, any thoughts on comment 20?
See bug 812203 comment 10.
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.
status-firefox19: fixed → verified
status-firefox20: fixed → verified
You need to log in before you can comment on or make changes to this bug.