Closed Bug 611505 Opened 9 years ago Closed 9 years ago

ASSERTION: TabParent not thread-safe

Categories

(Core :: IPC, defect)

x86
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: jdm, Assigned: azakai)

Details

(Keywords: assertion)

Attachments

(1 file, 1 obsolete file)

###!!! ASSERTION: TabParent not thread-safe: '_mOwningThread.GetThread() == PR_GetCurrentThread()', file /home/t_mattjo/src/firefox/mobilebase/dom/ipc/TabParent.cpp, line 86
nsAutoRefCnt::operator--() (/home/t_mattjo/src/firefox/mobilebase/fennec-dbg/dom/ipc/../../dist/include/nsISupportsImpl.h:251)
~nsCOMPtr (/home/t_mattjo/src/firefox/mobilebase/fennec-dbg/security/manager/ssl/src/../../../../dist/include/nsCOMPtr.h:531)
nsNSSSocketInfo::GetPreviousCert(nsIX509Cert**) (/home/t_mattjo/src/firefox/mobilebase/security/manager/ssl/src/nsNSSIOLayer.cpp:849)
nsCOMPtr<nsIX509Cert>::get() const (/home/t_mattjo/src/firefox/mobilebase/fennec-dbg/security/manager/ssl/src/../../../../dist/include/nsCOMPtr.h:800)
ssl3_HandleFinished (/home/t_mattjo/src/firefox/mobilebase/security/nss/lib/ssl/ssl3con.c:8506)
ssl3_HandleHandshakeMessage (/home/t_mattjo/src/firefox/mobilebase/security/nss/lib/ssl/ssl3con.c:8659)
ssl3_HandleHandshake (/home/t_mattjo/src/firefox/mobilebase/security/nss/lib/ssl/ssl3con.c:8727)
ssl3_HandleRecord (/home/t_mattjo/src/firefox/mobilebase/security/nss/lib/ssl/ssl3con.c:9066)
ssl3_GatherCompleteHandshake (/home/t_mattjo/src/firefox/mobilebase/security/nss/lib/ssl/ssl3gthr.c:209)
ssl_GatherRecord1stHandshake (/home/t_mattjo/src/firefox/mobilebase/security/nss/lib/ssl/sslcon.c:1258)
ssl_Do1stHandshake (/home/t_mattjo/src/firefox/mobilebase/security/nss/lib/ssl/sslsecur.c:151)
ssl_SecureSend (/home/t_mattjo/src/firefox/mobilebase/security/nss/lib/ssl/sslsecur.c:1213)
ssl_SecureWrite (/home/t_mattjo/src/firefox/mobilebase/security/nss/lib/ssl/sslsecur.c:1259)
ssl_Write (/home/t_mattjo/src/firefox/mobilebase/security/nss/lib/ssl/sslsock.c:1652)
nsSSLThread::Run() (/home/t_mattjo/src/firefox/mobilebase/security/manager/ssl/src/nsSSLThread.cpp:1052)
nsPSMBackgroundThread::nsThreadRunner(void*) (/home/t_mattjo/src/firefox/mobilebase/security/manager/ssl/src/nsPSMBackgroundThread.cpp:45)
_pt_root (/home/t_mattjo/src/firefox/mobilebase/nsprpub/pr/src/pthreads/ptthread.c:190)
__free_tcb (/usr/src/debug/glibc-2.11.2/nptl/pthread_create.c:211)

Running localstorage mochitests in fennec, I saw this assertion crop up fairly frequently.
I am now seeing this assertion spew in daily usage of desktop Fennec.  This should potentially block.
tracking-fennec: --- → ?
Assignee: nobody → azakai
tracking-fennec: ? → 2.0+
Attached patch patch (obsolete) — Splinter Review
Make TabParent threadsafe w.r.t xpcom.
Attachment #491021 - Flags: review?(doug.turner)
Comment on attachment 491021 [details] [diff] [review]
patch

well, yes, this will fix the assertion.  Is there any data in the implementation that needs to be in a critical section?
From skimming through it I would guess no. But to be sure we would need to carefully read every single line of code that can be reached from TabParent methods or that calls into them, which seems infeasible.

There is little downside to this patch - it doesn't make us more exposed to concurrency problems than before. It just improves the situation by making the xpcom stuff threadsafe, I think?
you could just see what is causing these threadsafe assertions and inspect what those callers are doing.  I would imagine it is a very small set of things that happen.
The only non-main thread work done on TabParent that I've seen is from the SSL thread, and involves the nsISSLStatusProvider aspect of the object.  From the stack in comment 0, we can see this:

>    nsCOMPtr<nsISSLStatusProvider> statprov = do_QueryInterface(secureUI);
>    if (statprov) {
>      nsCOMPtr<nsISupports> isup_stat;
>      statprov->GetSSLStatus(getter_AddRefs(isup_stat));
>      if (isup_stat) {
>        nsCOMPtr<nsISSLStatus> sslstat = do_QueryInterface(isup_stat);
>        if (sslstat) {
>          sslstat->GetServerCert(getter_AddRefs(mPreviousCert));
>        }
>      }
>    }

Just above this we proxy a release of an nsISecureUI object (the TabParent, presumably) to the main thread, which is fine, but the quoted bit here looks suspicious to me.

Honza, I'm presuming this is new with your SSL work.  Got any thoughts?
Comment on attachment 491021 [details] [diff] [review]
patch

Sorry to r- on your behalf Doug.  This patch is not really the right way to choose.

This is an interesting issue, pointing a bit weird design out here (should not be solved in this bug, though).  We actually need just to reach a class that implements nsISecureBrowserUI to switch to a user-friendly and correct type of ssl error reporting (the certError.xul page).  We don't need the object returned to necessarily work...

The solution here is to simply remove nsISSLStatusProvider from the list of interfaces that TabParent implements because doing nsCOMPtr<nsISSLStatusProvider> statprov = do_QueryInterface(secureUI) calls AddRef on TabParent from a non-main thread, as it returns itself as nsISSLStatusProvider.

TabParent::GetSSLStatus always returns null object after bug 582840 has landed (fennec uses a different way to collect security state), so we'll never have mPreviousCert filled anyway.
Attachment #491021 - Flags: review?(doug.turner) → review-
Also, mSecurityStatusObject, mSecurityTooltipText and mSecurityState should be removed from TabParent.  nsISecureBrowserUI method implementation then should return just null and empty strings.  It does so anyway.
Kai, comment 7 might be interesting for you.  It is about our favorite code for switching to "external error reporting".  I think we should slowly move to a different way how to recognize when this flag should be set.
Honza, I am unsure of what should be done in this bug, and what should be left for later?
(In reply to comment #10)
> Honza, I am unsure of what should be done in this bug, and what should be left
> for later?

If you still want to work on this then please:
- remove nsISSLStatusProvider implementation from TabParent class (don't let TabParent derive from it)
- remove mSecurityStatusObject, mSecurityTooltipText and mSecurityState from TabParent class, they are always empty (null, "" and 0 respectively)
Attached patch patch v2Splinter Review
Ok, I hope I understood properly, is this correct?

With this patch, no longer getting that assertion, and nothing appears to be broken either.
Attachment #491021 - Attachment is obsolete: true
Attachment #491893 - Flags: review?(honzab.moz)
Comment on attachment 491893 [details] [diff] [review]
patch v2

Excellent.

r=honzab.
Attachment #491893 - Flags: review?(honzab.moz) → review+
http://hg.mozilla.org/mozilla-central/rev/5b540a5854b5
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.