Closed Bug 471296 Opened 11 years ago Closed 11 years ago

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

Categories

(Core :: XPCOM, defect)

x86
Windows XP
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla1.9.2a1
Tracking Status
blocking1.9.1 --- -
status1.9.1 --- .4-fixed

People

(Reporter: timeless, Assigned: timeless)

References

Details

(Keywords: assertion, regression)

Attachments

(1 file, 1 obsolete file)

3.36 KB, patch
benjamin
: review+
samuel.sidler+old
: approval1.9.1.2-
Details | Diff | Splinter Review
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
fwiw: ASSERTION: trying to suspect from non-main thread: 'NS_IsMainThread()',
Keywords: assertion
Attached patch split CC class (obsolete) — Splinter Review
Assignee: peterv → timeless
Status: NEW → ASSIGNED
Attachment #354577 - Flags: review?(dbaron)
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.
Attachment #354577 - Flags: review?(dbaron) → review?(benjamin)
Blocks: 470240
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?
Attachment #354577 - Flags: review?(benjamin) → review-
Attached patch changesSplinter Review
Attachment #354577 - Attachment is obsolete: true
Attachment #356486 - Flags: review?(benjamin)
Attachment #356486 - Flags: review?(benjamin) → review+
http://hg.mozilla.org/mozilla-central/rev/57e38d817e3f
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.2a1
I think this should get in for 1.9.1; see bug 505041.
blocking1.9.1: --- → ?
blocking1.9.1: ? → -
Attachment #356486 - Flags: approval1.9.1.2? → approval1.9.1.2+
Comment on attachment 356486 [details] [diff] [review]
changes

a=beltzner for mozilla-1.9.1
Comment on attachment 356486 [details] [diff] [review]
changes

This didn't land in time for 1.9.1.2.
Attachment #356486 - Flags: approval1.9.1.3?
Attachment #356486 - Flags: approval1.9.1.2-
Attachment #356486 - Flags: approval1.9.1.2+
Comment on attachment 356486 [details] [diff] [review]
changes

Approved for 1.9.1.4, a=dveditz for release-drivers
Attachment #356486 - Flags: approval1.9.1.3? → approval1.9.1.4+
Let's try to get this one landed this time
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.
Keywords: checkin-needed
Whiteboard: checkin-needed: 1.9.1 branch
You need to log in before you can comment on or make changes to this bug.