611505 - ASSERTION: TabParent not thread-safe

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Josh Matthews [:jdm]]

###!!! 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.

Comment 1

[Josh Matthews [:jdm]]

I am now seeing this assertion spew in daily usage of desktop Fennec.  This should potentially block.

Comment 2

[Alon Zakai (:azakai)]

Attachment: patch

Make TabParent threadsafe w.r.t xpcom.

Comment 3

[Doug Turner (:dougt)]

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?

Comment 4

[Alon Zakai (:azakai)]

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?

Comment 5

[Doug Turner (:dougt)]

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.

Comment 6

[Josh Matthews [:jdm]]

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 7

[Honza Bambas (:mayhemer)]

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.

Comment 8

[Honza Bambas (:mayhemer)]

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.

Comment 9

[Honza Bambas (:mayhemer)]

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.

Comment 10

[Alon Zakai (:azakai)]

Honza, I am unsure of what should be done in this bug, and what should be left for later?

Comment 11

[Honza Bambas (:mayhemer)]

(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)

Comment 12

[Alon Zakai (:azakai)]

Attachment: patch v2

Ok, I hope I understood properly, is this correct?

With this patch, no longer getting that assertion, and nothing appears to be broken either.

Comment 13

[Honza Bambas (:mayhemer)]

Comment on attachment 491893 [details] [diff] [review]
patch v2

Excellent.

r=honzab.

Comment 14

[Alon Zakai (:azakai)]

http://hg.mozilla.org/mozilla-central/rev/5b540a5854b5