442066 - Calling a JS callback from a worker thread can cause a crash

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bruno Calvignac]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008062515 Firefox/3.0 Flock/2.0b2pre
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9) Gecko/2008062515 Firefox/3.0 Flock/2.0b2pre

Our Flock browser has an indexing (C++) thread that calls back to a JS component to signal that the operation is complete so that another document(bookmark) can be indexed.
I have discovered some threading issues that result in crashes. See attached stacks.

Reproducible: Sometimes

Steps to Reproduce:
1.Start Flock (Firefox based) with no profile
2.Do not import anything
3.Let the default bookmarks be indexed
4.Crash after 10-30 seconds (most reproducible on my Vista machine but seen once on Mac)

Comment 1

[Bruno Calvignac]

Attachment: Stack crash #1 (on Vista)

Comment 2

[Bruno Calvignac]

Attachment: Crash stack #2 (on Mac and Vista)

Comment 3

[Bruno Calvignac]

Flock is based on the Firefox 3 General release.

Comment 4

[Bruno Calvignac]

This is a scenario of the crash:

    Main thread                          Worker thread
                                     calls Release on a nsXPCWrappedJS object
which has a mRefCnt of 2
acquires map lock in GetNewOrUsed
finds a valid root in WrappedJSMap
releases the maplock

finds a valid IID

                                     mRefCnt is decremented to 1 then 0 and
calls NS_DELETEXPCOM

adds reference to it (mRefCnt = 1)
                                     the destructor is called and the object
removed from the WrappedJSMap
tries to access that nsXPCWrappedJS
BOOM

Comment 5

[Bruno Calvignac]

The final Release (when refcnt ==2) usually happens in the main thread, but some race condition can make it happen in the Worker thread

Comment 6

[Bruno Calvignac]

The crash says "Access violation reading location 0xddddddf1" which indicates
deleted memory.

Comment 7

[Bruno Calvignac]

Attachment: fix by adding synchronization locks

The lock on Release is needed to match the other lock on AddRef on line 336.
What happens is that the Worker thread is destroying an nsXPCWrappedJS object
while the main thread is deciding if it can reuse it or it should create a new
one.

Comment 8

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 326938 [details] [diff] [review]
fix by adding synchronization locks 

jst should review this.

Comment 9

[Johnny Stenback (:jst)]

Comment on attachment 326938 [details] [diff] [review]
fix by adding synchronization locks 

Nice catch! Looks good, but this needs comments in Release() and GetNewOrUsed() explaining the need for the map lock to prevent two (or more) threads racing in those two calls.

r- only because I want to see the comments, then I can r+sr, even.

Comment 10

[Manish Singh]

Attachment: Add requested comments

Bruno's on vacation, so I'm handling this now

Comment 11

[Johnny Stenback (:jst)]

Comment on attachment 328787 [details] [diff] [review]
Add requested comments

Awesome. r+sr=jst, thanks for fixing!

Comment 12

[Johnny Stenback (:jst)]

We should land this on the 1.9.0 branch too assuming it tests out good etc.

Comment 13

[Johnny Stenback (:jst)]

Fix landed, thanks a ton for the patch!

Comment 14

[Bruno Calvignac]

You're welcome. Only took me 3 weeks to hunt the bug.

Comment 15

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Could this be the cause of bug 444859?

Comment 16

[Manish Singh]

Possibly, but as I commented in bug 444859, we'd need a more useful stack trace to investigate.

Comment 17

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

Not approving until we find out if this indeed caused bug 444859. Also, can we get some tests for this?

Comment 18

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

An update on tests for this or a fix for bug 444859. I'm inclined to minus this for 1.9.0.2 right now...

Comment 19

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

Comment on attachment 328787 [details] [diff] [review]
Add requested comments

We'll revisit approval for 1.9.0.3 given the possible regression bug 444859.

Comment 20

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

Comment on attachment 328787 [details] [diff] [review]
Add requested comments

Clearing approval request. Please re-request when bug 444859 has been addressed and tests have been created for both bugs.