471296 - nsArray (NS_ARRAY_CONTRACTID) can't be used on a single thread of its creator's choice

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[timeless]

while nsArray never claimed threadsafety, it didn't claim it was mainthread_only.

bug 413281 broke that.
sadly some consumers expected (reasonably imo) to be able to create arrays on other threads (this is not the same thing as passing arrays from thread to thread, merely creating and using them on a single non-main-thread).

xpcom_core!NS_DebugBreak_P+0x2a4
xpcom_core!nsCycleCollector::Suspect+0x6a
xpcom_core!NS_CycleCollectorSuspect_P+0x1b
xpcom_core!nsCycleCollectingAutoRefCnt::decr+0x7e
xpcom_core!nsArray::Release+0x89
xpcom_core!nsArrayConstructor+0x9b
xpcom_core!nsGenericFactory::CreateInstance+0x26
xpcom_core!nsComponentManagerImpl::CreateInstanceByContractID+0xc8
xpcom_core!CallCreateInstance+0x51
xpcom_core!nsCreateInstanceByContractID::operator()+0x27
pipnss!nsCOMPtr<nsIMutableArray>::assign_from_helper+0x1c
pipnss!nsCOMPtr<nsIMutableArray>::nsCOMPtr<nsIMutableArray>+0x34
pipnss!nsNSSCertificate::GetChain+0xfa [this class is threadsafe and expected a reasonable NS_ARRAY_CONTRACTID]
pipnss!AppendErrorTextUntrusted+0xcc [this frame is custom]
pipnss!getInvalidCertErrorMessage+0xdd
pipnss!nsHandleInvalidCertError+0x145
pipnss!nsNSSBadCertHandler+0xb57 [this frame can call out to an arbitrary implementation of nsIBadCertListener2, for the sake of discussion, assume that the frames past this point are one such impl]
ssl3!ssl3_HandleCertificate+0x40e
ssl3!ssl3_HandleHandshakeMessage+0x3df
ssl3!ssl3_HandleHandshake+0x1c8
ssl3!ssl3_HandleRecord+0x5f8
ssl3!ssl3_GatherCompleteHandshake+0xbb
ssl3!ssl_GatherRecord1stHandshake+0x7b
ssl3!ssl_Do1stHandshake+0x21d
ssl3!ssl_SecureSend+0x1c5
ssl3!ssl_SecureWrite+0x16
ssl3!ssl_Write+0xa3
pipnss!nsSSLThread::Run+0x264
pipnss!nsPSMBackgroundThread::nsThreadRunner+0x16
nspr4!_PR_NativeRunThread+0xdb
nspr4!pr_root+0x19
MSVCR80D!_beginthreadex+0x221
MSVCR80D!_beginthreadex+0x1c7
kernel32!BaseThreadStart+0x37

Comment 1

[timeless]

fwiw: ASSERTION: trying to suspect from non-main thread: 'NS_IsMainThread()',

Comment 2

[timeless]

Attachment: split CC class

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 354577 [details] [diff] [review]
split CC class

Seems ok to me, except I'd put this brace on its own line:

>+NS_METHOD nsArrayConstructor(nsISupports *aOuter, const nsIID& aIID, void **aResult) {

... but I think Benjamin should probably review this.

Comment 4

[Benjamin Smedberg]

Comment on attachment 354577 [details] [diff] [review]
split CC class

>+NS_METHOD nsArrayConstructor(nsISupports *aOuter, const nsIID& aIID, void **aResult) {
>+    nsresult rv;
>+    nsArray * inst;
>+    *aResult = nsnull;

Please do not set aResult in failure cases.

>+    inst = NS_IsMainThread() ? new nsArrayCC : new nsArray;
>+    if (!inst)
>+        return NS_ERROR_OUT_OF_MEMORY;

Please use a nsCOMPtr<nsIArray> or nsRefPtr<nsArray> to avoid the manual addref/release pair below.

>diff --git a/xpcom/ds/nsArray.h b/xpcom/ds/nsArray.h

>+    ~nsArrayCC();

Is there any reason you can't just make this an empty inline?

Comment 5

[timeless]

Attachment: changes

Comment 6

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/mozilla-central/rev/57e38d817e3f

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I think this should get in for 1.9.1; see bug 505041.

Comment 8

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 356486 [details] [diff] [review]
changes

a=beltzner for mozilla-1.9.1

Comment 9

[Samuel Sidler (old account; do not CC)]

Comment on attachment 356486 [details] [diff] [review]
changes

This didn't land in time for 1.9.1.2.

Comment 10

[Daniel Veditz [:dveditz]]

Comment on attachment 356486 [details] [diff] [review]
changes

Approved for 1.9.1.4, a=dveditz for release-drivers

Comment 11

[Daniel Veditz [:dveditz]]

Let's try to get this one landed this time

Comment 12

[Phil Ringnalda (:philor)]

It's assigned to timeless: that means that if you want it to land, you can land it yourself, or you can hope that I'm not on vacation and that the tree is green enough that I can land things after work, or you can slather it with checkin-neededs, but the one thing that you cannot do is treat it as though the assignee is someone who has been around forever and as a result will land his own patches. I know it's confusing, what with him having done the bulk of the checkin-needed for years, but it's also true.

Comment 13

[Phil Ringnalda (:philor)]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/c0e708a64d05