408327 - Camino sometimes crashes when using the Trash button in the Downloads window

Status: RESOLVED FIXED

(Camino Graveyard :: General, defect)

Description

[davedit]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.11) Gecko/20071207 Camino/1.6a1 (like Firefox/2.0.0.11)
Build Identifier: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en; rv:1.8.1.11) Gecko/20071207 Camino/1.6a1 (like Firefox/2.0.0.11)

Occasionally when I use the Trash button in the Downloads window, Camino will crash. Started seeing this on 1.5.3, and have also seen it on 1.5.4 and 1.6a1. I'm currently running OS X 10.5.1.

Reproducible: Sometimes




1.5.3 crash log: http://mozilla.pastebin.com/f58a74be6
1.5.4 crash log: http://mozilla.pastebin.com/f2c42c4fe
1.6a1 crash log: http://mozilla.pastebin.com/f4eaed0b

Comment 1

[Stuart Morgan]

I suspect we have a race condition around releasing the directory path NSString when the download is removed, and having the kqueue trigger and try to use that string.

Comment 2

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

I've definitely seen something like this, but running trunk. Confirming...

Comment 3

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: additional branch crash log

I just saw this, too, using yesterday's branch build on 10.5.1, but I have a different stack than Dave has.  (That's one way to make it easier for me to update to today's build :p)

Comment 4

[Stuart Morgan]

Odd; these all seem to be view related, but I thought the forums had something from the kqueue code.

Comment 5

[Stuart Morgan]

Nevermind, wrong thread; those stacks are what I was thinking of. Smokey, your crash may well be a slightly different manifestation of the same early-released string.

Comment 6

[Stuart Morgan]

Attachment: fix

Comment 7

[Stuart Morgan]

Attachment: review version

This is a -w version for review, since a lot of indentation changed.

There were actually several thread safety problem here, much to my chagrin, but the basic problem was indeed that we were using a string on the background thread that had just been released on the main thread by unsubscribing that progress viewer (this crash could happen any time the last tracked download in a given folder was deleted). The changes:
- Add an array owning the paths that are passed in to kevent as context data, and only remove things from that array on the background thread that actually uses them (this also fixes the only-works-by-accident nature of my previous use of the presence of the values as keys in a dictionary to own them; if NSString copy weren't just retaining, this would always have crashed).
- Protect code around concurrent access to the watch info dictionary. I didn't try to make it granular because there's no need for tight performance here, and it makes the logic much easier.
- Make sure the ProgressViewControllers won't send themselves performOnMainThread messages just before they are going to dealloc themselves.

Comment 8

[Stuart Morgan]

If we do a 1.5.5, we'll want this there too.

Comment 9

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment: Probable 1_5 branch patch

Stuart (or someone), can you make sure that I backported hunk 3 of FileChangeWatcher.m (the hunk smack dab in the middle of the exception stuff) correctly?

All the other hunks of attachment 293462 [details] [diff] [review] ("fix") applied.

Comment 11

[Stuart Morgan]

Comment on attachment 299921 [details] [diff] [review]
Probable 1_5 branch patch

Yep, looks good

Comment 12

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Attachment 299921 [details] [diff] checked in on the CAMINO_1_5_BRANCH.