318001 - Fatal Hang on file downloads to desktop with Quicksilver running kqueue

Status: RESOLVED FIXED

(Camino Graveyard :: Downloading, defect)

Description

[starfirex]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.8) Gecko/20051111 Firefox/1.5

Since about two nightly builds ago, Camino now has a fatal hang whenever you try to download a file. In addition to hanging itself, it also takes over all of the system resources, making it extremely difficult to do ANYTHING, including force quit Camino itself.

Reproducible: Always

Steps to Reproduce:
1.download a file, zip files seem to produce this very predictably.

Actual Results:  
Camino hangs, much spinning beachballness, processor resources go away, sometimes it will produce multiple 'parts' to a zip file which should not have had more than one, sometimes it will open the wrong helper application (stuffit instead of the archive helper) and cause that application to crash as well with the messed up file it downloaded.

Expected Results:  
N/A

Fix this quick plz. The places I need to download files from are school involved and neither Safari nor Explorer can access that particular area of the site.

Comment 1

[Wevah (Nate Weaver)]

Do you have a public URL that you can reproduce this on?

Comment 2

[Chris Lawson (gone)]

I'm entirely unable to reproduce this with any sort of file using the latest trunk nightly.

Anyone on branch having this problem?

Reporter, are you using trunk or branch?

cl

Comment 3

[starfirex]

I'm not sure of the specific difference.. However it's not a version compiled from the CVS, just a nightly.

Also, if you want to see this yourself, just go to macupdate.com and download something random. I tried sudokout and produced this the first time.

Comment 4

[Chris Lawson (gone)]

Reporter: go to Camino -> About Camino and copy-n-paste the Version line here, like this:

Version 2005112617 (1.0+)

(BTW, the above is from a custom trunk build I did two days ago, rather than the trunk nightly I tried to reproduce this with.)

cl

Comment 5

[starfirex]

Version 2005112704 (1.0b1+)

Comment 6

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

Reporter (does thou have a name?), can you try renaming (or removing) your ~/Library/Application Support/Camino folder? It contains your Camino profile. Something may be corrupt causing this to happen (though it would be weird).

Comment 7

[starfirex]

You can call me Haplo.
And.. jozyxqk.
Removing the Camino profile does fix this, so either it's another CamiTools related problem (which is begging to anger me) or else Camino is corrupting the profile somehow.

Comment 8

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

(In reply to comment #7)
> You can call me Haplo.
> And.. jozyxqk.
> Removing the Camino profile does fix this, so either it's another CamiTools
> related problem (which is begging to anger me) or else Camino is corrupting the
> profile somehow.
> 

Right, so before we close this, let's see if we can figure out what's wrong.

Try adding the files back in from the old profile (I'm assuming you kept it) one at a time (more or less) and see which one breaks it. If it is something from CamiTools, it'd be interesting to know what setting broke.

Comment 9

[starfirex]

It doesn't appear that CamiTools had anything to do with this. After screwing around with it, it seems that downloads.plist was the culprit. Adding/removing stuff from CamiTools had no noticable effect, but removing downloads.plist fixed it, and putting the old one back had it coming back again.

If there's some way to test this to figure out what went wrong with the file itself, I'd be happy to send any/all data your way.

Comment 10

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

Yes! Please attach the downloads.plist file to this bug so Nick can have a look at it. :)

Comment 11

[starfirex]

Attachment: The culprit to this bug.

Here we go, have fun screwing around with it.

Comment 12

[Simon Fraser [no longer active]]

Can anyone else repro the freezes with this downloads.plist?
Haplo: does doing a "Clean up" in your Downloads window make the problem go away?
Also, if you can, please try to get a sample of Camino when it hangs. Run Activity Monitor, select Camino, Get Info, and click the "Sample" button. Attach the result to this bug.

Comment 13

[starfirex]

I didn't use clean up. I set it to autoremove downloaded files on quit, but removing that downloads.plist file and letting Camino create a new one fixed it.
I'll post an activity monitor sampling sometime tonight. Can't do it right now tho.

Comment 14

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

Using that plist causes no problems for me. I can download files just fine.

Comment 15

[starfirex]

Attachment: Activity Monitor Sample while Camino is hanging.

This was taken while Camino was hanging, produced by downloading the Apple Broadband Tuner from macupdate.com.

Comment 16

[Simon Fraser [no longer active]]

It's hanging in FNSubscribeByPath. I wonder if there is a max number of subscriptions per directory that we're running into?

Comment 17

[Nick Kreeger]

could be a max number of subscriptions per directory, a quick google search doesn't reveal anything. Maybe we should try to keep a list of the directories we have subscribed to prevent mulitple registrations.

Comment 18

[Nick Kreeger]

Haplo:
Could you please answer a few questions?

1. What version OS are you running? (I.E. 10.4.2)

2. Are all the downloads that are in your download manager all exist in that location, meaning that none of those downloads have moved from their original download directory?

3. Does this happen on every download that you try to download, or only on certain downloads, or even any download in a certain directory?

Thanks !

Comment 19

[starfirex]

I'm running 10.4.3 14" iBook/1.42GHz/1.5GB/80Gb.
I don't think this involves a partially completed download,
but as for details about my downloading habits, most of the
things I download are either .dmg or .zip with a dmg contained,
which are mounted, used, then I unmount and delete all of the
related files left over.

As far as types, I don't think it mattered what filetype I downloaded, it would hang on all of them. Some of the things I was attempting to download from my college's website were .doc, others were .zip, and I'm not sure if I had tried to download any .dmg files. One big thing I did notice that may help, camino kept trying to create additional files for the single file I attempted to download. The additional files were always suffixed with 'part', and the original file would have the correct icon, but would be corrupted and fail to extract.

By the way, it took me like 10 minutes to get that sample since Camino was eating my system and it was a huge pain to get the menu to open to actually take the sample.

I still have the offending file, so any data you need me to collect I'll be happy to, but right now it's 3 in the morning, so I'm not so inclined to do any testing right this instant. I have about 18 billion classes tomorrow, so I won't be able to do anything productive on it till late night. I am interested in seeing what caused this though.

Comment 20

[Chris Lawson (gone)]

(In reply to comment #19)

> to download any .dmg files. One big thing I did notice that may help, camino
> kept trying to create additional files for the single file I attempted to
> download. The additional files were always suffixed with 'part', and the
> original file would have the correct icon, but would be corrupted and fail to
> extract.

That sounds a lot like bug 317729, and it makes me wonder if fixing that will fix this.

cl

Comment 21

[Nick Kreeger]

Hmm this could be a core problem if it relates to jacked up downloads and their corresponding parts

Comment 22

[starfirex]

Hmm, I find it odd that he didn't mention anything about Camino eating his system. Perhaps it is in fact two different expressions of the same bug, just mine is messed up more. The real question, however, is how exactly did my downloads.plist get corrupted, and why does it cause major problems when I use it but doesn't cause anyone else any trouble?

Comment 23

[Simon Fraser [no longer active]]

Haplo: when it freezes, is Camino using CPU resources, or something else? Can you see from activity monitor which process is using the CPU cycles? Your sample makes it look like Camino is just hung (not using CPU), so maybe it's the Finder that's eating cycles.

Comment 24

[starfirex]

Good idea, I'll probably have that verified tonight.

Comment 25

[starfirex]

Ok, so I tested with activity monitor. Interestingly enough, neither Camino nor any other program is taking any more CPU usage than normal, so apparently Camino is simply filling up the user input queue, causing my actions to be extremely delayed, accompanied by tons of beach balling to give the illusion that it's taking up all of the system.

Comment 26

[ender.mb]

FWIW, I've been seeing this since about the same timeframe the reporter described; a clean profile also fixed it for me. I tested with a few .dmg and .zip downloads with the Dec. 2 trunk and branch builds. The app would freeze up for up to several minutes and then process the download as usual (only once did I end up with an unusable download). While Camino was hanging, both the .part and the .dmg or .zip showed a size of 0KB in the Finder. This is on 10.4.3.

Comment 27

[ender.mb]

I've started seeing this again since my download manager list grew to about 10 items.

Comment 28

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

I'm up to 22 downloads listed in my manager, and I've yet to experience anything like a hang while downloading on 10.3.9 with the latest branch nightlies.

Comment 29

[Simon Fraser [no longer active]]

How many items to people have in their download folders (on disk) when they see this?

Comment 30

[Simon Fraser [no longer active]]

Does the Finder also hang when this occurs? If so, a sample of it might be interesting.

Comment 31

[Simon Fraser [no longer active]]

We need to figure this out.

Comment 32

[ender.mb]

My download folder (my desktop) has 37 items right now, which total about 2.7GB. That's about normal for my desktop though. 

I've noticed that we switched over to question mark icons for files that are no longer in the location they were downloaded to. Since I get rid of the original files (i.e. zip files, DMGs) almost immediately, is it possible that the DL manager is searching for all those files every time it gets opened, or is that handled some other way?

I haven't noticed any Finder hangs in particular, just general system slowness, though I'll try some more sampling next time I see this.

BTW, my system is a 1GHz PowerBook with 512MB RAM.

Comment 33

[David VanScott]

After clearing my Downloads window, I had no hang when I started a Download.  Perhaps something is happening in the window before the download starts when there's a lot there.

Comment 34

[Simon Fraser [no longer active]]

We need more info if we're going to keep this in 1.0.

Comment 35

[David VanScott]

In my experience with this bug, if there are a lot of files (specifically files that don't exist in the location Camino downloaded them to), the application will hang, presumably while the Downloads window searches for where those missing files are.  Once I cleared my Downloads window, I haven't had an issue with this any more.

Comment 36

[ender.mb]

I've had a similar experience here.. once the download manager gets cleared, I don't get hangs until it fills back up again. Qualitatively, Camino seems to hang worse with, say, a large zip file than a tiny .ram file. I did have a particularly nasty hang while downloading this: http://thinkmac.co.uk/newsmacpro/downloads/NewsMacPro.dmg.zip 
However when I tried to re-download it I got no hang at all. I did manage to get samples for Camino and the Finder if anybody would like to see them. Also interesting is the fact that all other apps are sluggish while Camino hangs, and if I try to launch an app it will just bounce in the dock until Camino comes back to life.

Comment 37

[Simon Fraser [no longer active]]

(In reply to comment #36)
> However when I tried to re-download it I got no hang at all. I did manage to
> get samples for Camino and the Finder if anybody would like to see them.

Please attach them to this bug, thanks!

Comment 38

[ender.mb]

Attachment: Finder sample

Comment 39

[ender.mb]

Attachment: Camino sample

Comment 40

[Simon Fraser [no longer active]]

In both Camino samples in this bug, it looks like the downloads window is loading for the first time (we're loading nibs etc), so we might be setting up lots of file systen notifications at the same time.

Comment 41

[Dag Ågren]

Here's some of my observations on this bug, copied from the forum thread about it. I've since had to downgrade to 1.0b1 because 1.0b2 is pretty much useless due to this bug. It only appears in 1.0b2 for me, although I haven't tried CVS.

Further information:
* Creating a new profile in CamiTools does NOT help.
* The length of time of the lockups seems to vary somewhat.
* Not only Camino, but the entire OS becomes unresponsive when downloading.
* Input events seem to get through in batches a couple of times before the system unlocks. So if I start typing text, I get a spinning beach ball, and after a while, all the text will appear, the mouse cursor turns normal, and if I try to do anything more, the beach ball comes back for a while.
* I noticed this happening before a download completed. The download window opened, the bar moved a bit, and then the system locked up.

Further information (or more likely, lack of information): The lockup starts just after the download window opens (although it does also happen when saving things out of the cache, such as images). The length of time varies, and it does not happen all the time, just some. I haven't found any pattern in what triggers it. The same file can lock up the browser once, but not after restarting. Sometimes the lockups happen soon after starting the browser, sometimes not.

The number of downloads in the download list don't seem to influence it very much - I've gotten lockups with as few as six items in the list.

I really hope this can be tracked down and fixed, because it's making Camino completely useless.

Comment 42

[ender.mb]

> * Input events seem to get through in batches a couple of times before the
> system unlocks. So if I start typing text, I get a spinning beach ball, and
> after a while, all the text will appear, the mouse cursor turns normal, and if
> I try to do anything more, the beach ball comes back for a while.
> * I noticed this happening before a download completed. The download window
> opened, the bar moved a bit, and then the system locked up.

I've only noticed these two recently.. usually the lockup occurs before the download manage ever opens, but on RC1 for example I've gotten subsequent hangs even after it opens.

Also seems to be an increased chance of download failure if I get a hang. I just got a particularly nasty hang on a 10MB disk image; Camino gave me 164KB and said it was done.

Comment 43

[Nick Kreeger]

Dag,

What is the URL of the file you are trying to download when you get this problem?

Comment 44

[Nick Kreeger]

Dag,

Also what OS version are you using?

Thanks

Comment 45

[Dag Ågren]

The bug does not appear consistently for any specific URL. It seems to appear after the browser has been running for a while, and will affect most if not all downloads, from any site. Restarting the browser helps sometimes, but the hang can come back fairly quickly.

Also, I am running 10.4.4.

Comment 46

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

1.0 has shipped, requesting blocking for 1.0.1.

Comment 47

[Mark Nightingale]

Attachment: culprit downloads.plist (brainache)

I got so fed up with this problem i was gonna report it as a bug.  Saw this discussion, moved my downloads.plist to the desktop and sudenly the downloads work straight away.  If you want my downloads.plist (old one) to look at for troubleshooting, i've attached it (im brainache).

Anyway, hope it helps you find a solution!

Comment 48

[Stuart Morgan]

390 loadNib
  390 -[NSIBObjectData nibInstantiateWithOwner:topLevelObjects:]
    390 CHBrowserService::RegisterAppComponents(nsModuleComponentInfo const*, int)
      390 DNS_GetDomainName
        390 DNS_GetDomainName
          390 DNS_GetDomainName
            390 FNSubscribeByPath

RegisterAppComponents doesn't appear to call DNS_GetDomainName, nor does DNS_GetDomainName call itself or FNSubscribeByPath. Am I missing something, or is this part of the stack bogus?

Comment 49

[Simon Fraser [no longer active]]

It's bogus; you're seeing sampler find public symbols which are the closest to the symbols that have been stripped out. You should always read stacks from optimized builds with an eye of suspicion.

Comment 50

[dave]

I have been experincing similar problems.  And although I have no particluarly new insight, just wanted to add my observations. I am running 10.4.4 and using Camino Tools. 
-I only have this problem when there are several items in the download manager.
-I can avoid this problem by constantly cleaning up.
-And it seems to be related to the number of items downloaded plus the number of items downloaded no longer on my desktop. (I download all files onto the desktop and than usually do something with them from there-if I have downloaded a lot and since moved them or put them in the trash the problem seems to get worse)

Comment 51

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

*** Bug 328049 has been marked as a duplicate of this bug. ***

Comment 52

[Dan]

On 10.4.5, iBook G4.

I'm not sure if this is relevant, but I've recently tried logging the output of top during one of these camino-inspired system hangs, and noticed that distnoted sits between 1-5% CPU during the entire hang (>10sec), then goes back to 0% afterwards.  If camino's downloads list is clean (ie no system hang), then distnoted is only active very briefly (<1sec).

I do realise that this may only be a symptom of the problem, but thought I'd throw it in anyway...

Comment 53

[Nick Kreeger]

People that are having this issue, where are you downloading your items to? For instance, I download to some seperate folder in my documents folder (and I have not been able to reproduce), and by default they download to the Desktop. Maybe there is something related to that.

Comment 54

[starfirex]

Desktop here. (for easy processing of files and deleting afterwards)

Comment 55

[Dan]

I can confirm that it seems to be an issue with the Desktop
I was previously downloading to my Desktop and having the "hanging" issue after only a few files.  I just changed the default location to a new folder on the Desktop and downloaded about 12 files without any problems at all.
I then changed the default download dir back to the Desktop and saw the same "hanging/freezing" behaviour on the third download to the Desktop.

Comment 56

[Nick Kreeger]

As I was thinking, it seems that the desktop folder may have some sort of subscription problems with launch services. I'll take and look into.

Comment 57

[Nick Kreeger]

(In reply to comment #55)
> I can confirm that it seems to be an issue with the Desktop
> I was previously downloading to my Desktop and having the "hanging" issue after
> only a few files.  I just changed the default location to a new folder on the
> Desktop and downloaded about 12 files without any problems at all.
> I then changed the default download dir back to the Desktop and saw the same
> "hanging/freezing" behaviour on the third download to the Desktop.
> 

Dan,

Is there any chance you have some third party app that is running that may be interested in the Desktop?

Comment 58

[Dan]

I think it may have something to do with Quicksilver.
OSX 10.4.5 PPC, Camino Version 2006030904 (1.0.0+), Quicksilver B48
The Camino Downloads folder is set to my Desktop.

1. Download about 4 files (I am dlding from www.apple.com/downloads/macosx )
2. Move these files to the trash
3. Download another file.
-- Try moving the mouse over links on the page - it appears that Camino is frozen - and soon you'll see a spinning beachball.  Finally the Downloads window pops up and the download completes.
4. Quit Quicksilver.
-- Now you can download to your heart's content without any hangs or freezes.
5. Start Quicksilver.
6. Follow steps 1-3 again, and see the system "hang".

I changed the Downloads folder to ~/Desktop/downloads and all is OK.  I even added that folder to the Quicksilver catalog and I haven't had any more hangs.  It seems to have something to do with Quicksilver, the Desktop, and Camino, as I haven't seen this behaviour with any other browsers (Firefox, Safari).

Comment 59

[Nick Kreeger]

Do you have a link for Quicksilver?

Comment 60

[starfirex]

www.blacktree.com
It's under active development so you can get info for it. QS does index files, but unlike spotlight it's typically only interested in applications, so I can't really explain how it would cause this.

Comment 61

[ender.mb]

That's very interesting. I send my downloads to the desktop, and I have quicksilver installed as well. I'll see if I can verify the behavior Dan reports in comment 58..

Comment 62

[Dan]

More notes...
Either "Cleaning Up" the Downloads window or restarting Quicksilver temporarily solves the issue (at least until you download more files and delete them off the desktop). Once the hangs reoccur just "Clean Up" the Downloads window or restart Quicksilver again...

Could it have something to do with Notifications to both Camino and Quicksilver?

Comment 63

[Simon Fraser [no longer active]]

(In reply to comment #62)
> Could it have something to do with Notifications to both Camino and
> Quicksilver?

That sounds quite possible. Smells like the FNNotifcation stuff has some O(N^2) behavior with the number of notification registered on a directory.

Comment 64

[starfirex]

Why would camino receive notifications for files which have already been moved or deleted?

Comment 65

[ender.mb]

(In reply to comment #60)
> www.blacktree.com
> It's under active development so you can get info for it. QS does index files,
> but unlike spotlight it's typically only interested in applications, so I can't
> really explain how it would cause this.
> 

Quicksilver looks for all files, not just applications.

I can reproduce everything in comment 58 here (Camino 1.0, QS 3763). Additionally, I tried setting my downloads directory to ~/Documents (which, like ~/Desktop, has a large number of files in it) but that didn't trigger a hang. Could be because I had just relaunched Quicksilver though. 

Comment 66

[Simon Fraser [no longer active]]

(In reply to comment #64)
> Why would camino receive notifications for files which have already been moved
> or deleted?

Because we register file system notifications on the parent folder of a downloaded item, rather than the item itself (this is just the way that FNNotify works).

Comment 67

[Chris Lawson (gone)]

FWIW, I've never once seen this, and I have Quicksilver.

My downloads directory is not the desktop, however. So it would appear you MUST be downloading to ~/Desktop for this to be a problem. Quicksilver itself may be a red herring, not totally sure.

cl

Comment 68

[riki85]

*** Bug 330248 has been marked as a duplicate of this bug. ***

Comment 69

[Brad West]

I just wanted to chime in and say I'm seeing the same behavior: I download to the Desktop, periodically clean the itmes off the Desktop, use Quicksilver, and am seeing hanging on pretty much any download. Cleaning up my download history seems to remedy the situation.

Comment 70

[Chris Lawson (gone)]

Temporary workaround for now:

You can stop the indexing of the Desktop by opening the Catalog preferences in Quicksilver and either unchecking or removing Desktop (inside "User") from the catalog.

cl

Comment 71

[Dan]

(In reply to comment #70)
> Temporary workaround for now:
> 
> You can stop the indexing of the Desktop by opening the Catalog preferences in
> Quicksilver and either unchecking or removing Desktop (inside "User") from the
> catalog.
> 
> cl
> 

Weird. Turning off indexing of the Desktop in QS doesn't fix the problem for me - I'm still seeing the hangs.

Comment 72

[Dan]

Sorry - scrap my last comment.  Turning off indexing of the Desktop in Quicksilver *does* fix the problem.

Comment 73

[Parker McGee]

Attachment: New download.plist

Comment 74

[Mark Mentovai]

No progress, not gonna make 1.0.1, but we can do it for 1.0.2.

Comment 75

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

Per previous comments, this bug still has a shot at making 1.0.x if we get a tested patch by then.

Comment 76

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

Minusing because we don't have a fix here yet (but the issue has been relnoted); we should try to get this for 1.0.x since it is one of the regressions we shipped in 1.0.

Comment 77

[alcor]

QS only looks for notifications in beta mode. if you like, try switching it to stable mode with indexing every 10 minutes, if it doesn't cause a problem, it is the kqueue stuff. 

Comment 78

[Chris Lawson (gone)]

Updating summary.

cl

Comment 79

[Nick Kreeger]

Refining the summary, the problem comes from a conflict between kqueue and FNSubscribe.

Comment 80

[ender.mb]

(In reply to comment #77)
> QS only looks for notifications in beta mode. if you like, try switching it to
> stable mode with indexing every 10 minutes, if it doesn't cause a problem, it
> is the kqueue stuff. 
> 

Is it only in beta mode that it looks for notification, or does advanced mode do it as well? I've gotten a couple of hangs so far in advanced mode..

Comment 81

[Nick Kreeger]

*** Bug 342049 has been marked as a duplicate of this bug. ***

Comment 82

[Nick Kreeger]

If i remember correctly, queue was not an option for us during the initial creation of this patch because it requires 10.3. 

Maybe the option to fix this is to implement kqueue and dump NSSubscribe.

Comment 83

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

Missed 1.0.3, but we can try again for 1.0.4 if there's a patch.

Comment 84

[Nick Kreeger]

(In reply to comment #82)
> If i remember correctly, queue was not an option for us during the initial
> creation of this patch because it requires 10.3. 
> 
> Maybe the option to fix this is to implement kqueue and dump NSSubscribe.
> 

Another (inelegant) solution would be to sniff out Quicksliver and disable file subscriptions to the desktop.

Comment 85

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

We decided the way to fix this was to switch to kqueue, which is only 10.3+, so minusing for 1.0.x.  

kreeger, can you have this ready for a1? It would be good to eliminate our most-hated bug in the first 1.1 milestone we ship.

Comment 86

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

*** Bug 354909 has been marked as a duplicate of this bug. ***

Comment 87

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

*** Bug 355539 has been marked as a duplicate of this bug. ***

Comment 88

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

Not blocking 1.1a1, but we'd definitely take a patch.

Comment 89

[Nick Kreeger]

Attachment: Early WIP

Here is a WIP of the kqueue implementation. I'm posting this to give an idea of the architecture that I'm imposing here.

Comment 90

[Brian Donovan]

Unchecking the Desktop in the QS prefs does fix this. FWIW, I always get the lag when downloading a file, but rarely do I actually have to force-quit. Normally it straightens itself out after 10-40s. (on a MacBook)

Comment 91

[Nick Kreeger]

Attachment: Patch v1

Here is the first go round of this patch. It creates a thread when the download manager is created, then only polls the directory every 5 seconds while the download manager is visible, and every 60 when the manager is hidden.

Comment 92

[Stuart Morgan]

Comment on attachment 246250 [details] [diff] [review]
Patch v1

>+ *   M. Uli Kusterer (kqueue)

Is this really the right way to attribute code that you've used as a starting point for some part of the patch?  I would think this should only list people who've actually worked on this file, with any code you want to call out as being from someone else should be done as a comment on that code.

>+  DirectoryChangeWatcher* mDirChangeWatcher;        // Member here so we can stop the polling when we aren't showing.

No need to justify the rationale for it being a member. Do mention that it's a strong ref though.

>+@interface DirectoryChangeWatcher : NSObject
>+{
>+  NSMutableArray* mWatchedProgViewControllers;
>+  NSMutableArray* mWatchedFileDescriptors;

strong refs

>+  BOOL   mShouldRunThead;

This serves no purpose in the current design.

>+  int    mQueueFD;           // queue ID (Unix file descriptor).

Don't use abbreviations like FD. Ideally the name should be clear enough that there's no need for the comment.

>+  struct timespec mTimeout;  // polling timespec (slower when hidden).

Why not store this as something simple, like an unsigned int, and create the  struct locally when you need it?  Call it something more descriptive like mUpdateInterval.

>+-(void)windowIsVisible;
>+-(void)windowIsHidden;

The watcher doesn't have a window, and it shouldn't care what the criteria for changing the polling interval is. Provide a setter for the time interval, and let the download controller manage changes to it.

>+-(void)pollWatchedDirectories:(id)sender;

This should be private. And given that it's argument is unused, why call it sender?

>+-(void)addProgressViewToWatchQueue:(ProgressViewController*)aProgViewController;
>+-(void)removeProgressViewFromWatchQueue:(ProgressViewController*)aProgViewController;

It would probably better to use something more general like a simple protocol, rather than binding the implementation for the watcher to PVCs.

>+static const int gTimeoutVisibleSecondSpec = 5;
>+static const int gTimeoutHiddenSecondSpec = 60;

k, not g. They need to be more descriptive of what they are for, as well; something like kFileUpdateInterval{Visible,Hidden}?

>+-(void)pollWatchedDirectories;

Remove (there's no such method).

>+    [NSThread detachNewThreadSelector:@selector(pollWatchedDirectories:) toTarget:self withObject:nil];
>+    mQueueFD = kqueue();
>+    mTimeout.tv_sec = gTimeoutHiddenSecondSpec;
>+    mTimeout.tv_nsec = 0;

Values that you use in pollWatchedDirectories: need to be initialized before you spawn the thread.

You've also created a leak here in that the thread never ends, and it retains this object, so this object will never be deallocated. At the moment we only have one and its going to be around for the lifetime of the app anyway, but the implementation shouldn't rely on/enforce that. The thread should be started when the number of observed files changes from 0 to 1, and end itself when there is nothing left to observe.

>+    EV_SET(&ev, fileDesc, EVFILT_VNODE, EV_ADD | EV_ENABLE | EV_CLEAR, fflags, 0, (void*)aProgViewController);

You need to clearly document that adding an object to the watch queue creates a weak ref to it, and that it is therefore required to remove itself from the queue on dealloc to prevent crashes.

>+-(void)removeProgressViewFromWatchQueue:(ProgressViewController*)aProgViewController
>+{
>+  int index = 0;
>+  int fileDesc = -1;
...
>+-(void)pollWatchedDirectories:(id)sender
>+{
>+  int n;
>+  struct kevent event;

Declare variables as needed, not up front

>+  int mainFileDesc = mQueueFD;

Why assign this to a local?

>+-(void)closeOpenFileDescriptors

I think this will end up going away. If the thread only ends when there is nothing being watched, and the object can only be dealloced if the thread has stopped, then there would never be any file descriptors to close.

> -(void)setupFileSystemNotification
...
> -(void)unsubscribeFileSystemNotification

The old versions of these functions ensured that there was always at most one subscription; that doesn't appear to be true of the new versions, and having multiple entries for one PVC wouldn't be good, as they probably wouldn't ever get cleaned up correctly.

Comment 93

[Nick Kreeger]

Attachment: Patch V2

Round deuce, instead of binding the file watcher to PVC objects, I created a protocol for objects that want to be listened to by the file watcher, called: |WatchedFileDelegate|.

This patch is constructed on the 1.8 branch (for Camino 1.1a2)

Comment 94

[Stuart Morgan]

Comment on attachment 247876 [details] [diff] [review]
Patch V2

Getting close. General comments:
- FileChangeWatcher should probably go in a new file.
- Whatever file it ends up in needs to be compiled with -fobjc-exceptions, since it uses @synchronized.

> +(ProgressDlgController *)existingSharedDownloadController;   // doesn't create

This is never used, and should probably be removed.

>+ * This class provides polling of files represented
>+ * in PVC's that are known to exist. When a file is
>+ * renamed, moved, or deleted, this class tells the
>+ * progress view to update its view to contain the
>+ * '?' icon.

This comment needs to be updated. It's not PVC specific, and it knows nothing about what the delegate would choose to do with the notification that the file changed.

>+// Portions of this method was originally written by M. Uli Kusterer.

s/was/were/

>+-(void)pollWatchedDirectories
>+{
>+  struct timespec timeInterval = { mQueueFileDesc, 0 };

I assume this was supposed to be mPollingInterval, not mQueueFileDesc?

>+        [(id<WatchedFileDelegate>)event.udata fileDidChange];

The fact that this callback will be received on a background thread needs to be very clearly called out in header comments.

>+// Any object that needs a file to be polled within the life scope of |ProgressDlgController|
>+// can implement this protocol and register with the watch queue.

This is overly specific; WatchedFileDelegate is generic, not PDC-specific

>+// Aadding an object to the watched queue creates a weak ref to the object, and the object needs
>+// to remove itself from the watch queue on dealloc.

Typo ('Aadding'). This comment really belongs in the header comments for addFileDelegateToWatchQueue:, since that's what it's about.

>+@interface ProgressViewController : NSObject<CHDownloadProgressDisplay, WatchedFileDelegate>

Shouldn't there be header entries for the methods associated with conforming to the protocol?

>+-(id)initWithDictionary:(NSDictionary*)aDict 
>+    andWindowController:(ProgressDlgController*)aWindowController;

Why do we need this when there is already a setWindowController:? And if we do need this, do we still need setWindowController:?

> -(void)setupFileSystemNotification
> {
...
>+  if (mFileExists)
>+    [mProgressWindowController addFileDelegateToWatchQueue:self];
> }
> 
> -(void)unsubscribeFileSystemNotification
> {
...
>+  [mProgressWindowController removeFileDelegateFromWatchQueue:self];
> }

It's still not clear to me that this is guaranteed not to add itself to the queue multiple times, which would be really bad.

>+-(void)fileDidChange
>+{
>+  [self checkFileExists];
>+}

Document that this method is called on a background thread. Doesn't the call to checkFileExists need to be wrapped in a performSelectorOnMainThread?

Comment 95

[Nick Kreeger]

Attachment: Patch V3

> +(ProgressDlgController *)existingSharedDownloadController;	// doesn't
> create

> This is never used, and should probably be removed.

Actually, it is used in |MainController| twice and |SessionManager|.

> Shouldn't there be header entries for the methods associated with conforming to
> the protocol?

No?

> +-(id)initWithDictionary:(NSDictionary*)aDict 
> +    andWindowController:(ProgressDlgController*)aWindowController;

> Why do we need this when there is already a setWindowController:? And if we do
> need this, do we still need setWindowController:?

The reason we need to set the windowcontroller and init time is because PVC needs to be able to add itself to the watch queue when loading from a dictionary. So, I removed |setProgressWindowController:| and created a |initWithWindowController:| method instead. The only call to |setProgressWindowController:| was:
  ProgressViewController *newController = [[[ProgressViewController alloc] init] autorelease];
  [newController setProgressWindowController:self];

> It's still not clear to me that this is guaranteed not to add itself to the
> queue multiple times, which would be really bad.

Here is how |setupFileSystemNotification| can get called in PVC.

1. A download ends via the callback to |onEndDownload:|, a in-param to that method tells us if the download was successful. I added an additional check to only make the file system notification call |if (completedOK)|.

2. The other call to this method happens when we initialize a PVC from a dictionary via |setProgressViewFromDictionary:|, or a download starts from |nsDownloadListener::InitDialog()|. The call to attempt the file subscription setup is made only when |mDownloadDone == YES|. (Note that |mDownloadDone == NO| when |nsDownloadListener| makes the call to start the download.

There are also two checks that are made within |FileChangeWatcher| and kqueue itself. 

First, |FileChangeWatcher| checks to make sure that the |<WatchedFileDelegate>| instance isn't already in the array:

  if ([mWatchedFileDelegates containsObject:aWatchedFileDelegate])
    return;

Also from the kqueue man page:
"An (ident, filter) pair can only appear once is a given kqueue.  Subsequent attempts to register the same pair for a given kqueue will result in the replacement of the conditions being watched, not an addition."

Comment 96

[Stuart Morgan]

Comment on attachment 249697 [details] [diff] [review]
Patch V3

>+-(const char*)representedFilePath;
>+-(void)fileDidChange;  // This method will be called on a background thread.
...
>+-(void)setPollingInterval:(unsigned int)aUpdateInterval;
>+-(void)addFileDelegateToQueue:(id<WatchedFileDelegate>)aWatchedFileDelegate;
>+-(void)removeFileDelegateFromQueue:(id<WatchedFileDelegate>)aWatchedFileDelegate;

Add a comment before each method saying what it should/will do (and call out the weak ref thing for addFileDelegateToQueue). The idea is that someone could figure out how to use FileChangeWatcher just by reading the header.

>+const int kFileUpdateVisibleInterval = 5;
>+const int kFileUpdateHiddenInterval = 60;

These should be in ProgressDlgController.mm, not here.

>+  if ((self = [super init]))
>+  {

Same line.

>+    mPollingInterval = kFileUpdateHiddenInterval;

Just pick a default value here (either a raw value, or a new kDefaultUpdateInterval constant).

>+    mShouldRunThead = NO;

There's no need for this; It's set to no for you, and you set it before you use it anyway. Also, typo (Thead -> Thread).


>+-(void)setPollingInterval:(unsigned int)aUpdateInterval
>+{
>+  mPollingInterval = aUpdateInterval;
>+}

This should probably be @synchronized since it's being read from the background thread.

>+-(void)addFileDelegateToQueue:(id<WatchedFileDelegate>)aWatchedFileDelegate
>+{  
>+  if ([mWatchedFileDelegates containsObject:aWatchedFileDelegate])
>+    return;
...
>+    @synchronized(self) {
>+      [mWatchedFileDelegates addObject:aWatchedFileDelegate];
>+      [mWatchedFileDescriptors addObject:[NSNumber numberWithInt:fileDesc]];
>+      kevent(mQueueFileDesc, &ev, 1, NULL, 0, &nullts);
>+      if (!mShouldRunThead && [mWatchedFileDescriptors count] > 0)
>+        [self startPolling];
>+    }
>+  }
>+}

This whole method would need to be @synchronized; right now there is a race condition where two different methods could get through the check at the beginning. However, it seems like unless you are intending this class to be threadsafe to its clients, which we don't really need, it looks like you could make things easier and remove all the synchronization from this and removeFileDelegateFromQueue:.


>+-(void)startPolling
>+{
>+  mShouldRunThead = YES;
>+  [NSThread detachNewThreadSelector:@selector(pollWatchedDirectories) toTarget:self withObject:nil];
>+}

This needs synchronization as well or you could end up with two or more background threads. In fact you need more than that, since the background thread doesn't exit immediately; you need a marker value that the background thread controls indicating whether it is already running, and to synchronize that and mShouldRunThread carefully. This is true even if the class isn't intended to be threadsafe for clients since the issue is the following (which could easily happen):
1) The last file is removed from the watch list; mShouldRunThread is now NO
2) A new file is added to the watch list before the background thread wakes up and sees the NO; mShouldRunThread is now YES
3) The background thread will now stay running, and a second background thread will be started.

>+-(void)pollWatchedDirectories
>+{
>+  struct timespec timeInterval = { mPollingInterval, 0 };
>+  
>+  while (mShouldRunThead) {
>+    NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
>+    NS_DURING
>+      timeInterval.tv_sec = mQueueFileDesc;
>+      struct kevent event;
>+      int n = kevent(mQueueFileDesc, NULL, 0, &event, 1, &timeInterval);

You only set the timeInterval once, outside the loop, so mPollingInterval is never actually doing anything useful. I looked over the kqueue docs again though, and if I'm understanding correctly kevent will return immediately if anything changes; the timeout is only to allow waiting for an event for some bounded time. If that's true, then the timeout value has nothing to do with responsiveness to change events here, it just controls the upper bound on how long it will take the background thread to notice that it shouldn't run any more, in which case there's no need to have mPollingInterval and ways to adjust it; you can just pick a reasonable value (60 seems good) and use that.

>+      if (n > 0 && event.filter == EVFILT_VNODE && event.fflags)
>+        [(id<WatchedFileDelegate>)event.udata fileDidChange];
>+      NS_HANDLER
>+        NSLog(@"Error in watcherThread: %@", localException);
>+      NS_ENDHANDLER
>+      
>+      [pool release];
>+  }
>+    
>+    close(mQueueFileDesc);
>+}

The indentation of NS_HANDLER and everything after it is off

>\ No newline at end of file

Add a newline please.

>   BOOL                  mAwaitingTermination;       // true when we are waiting for users termination modal sheet
>+  
>+  FileChangeWatcher*    mFileChangeWatcher;        // strong ref.

Align the comment with the other comments

>+  [self performSelectorOnMainThread:@selector(checkFileExists)
>+                         withObject:nil waitUntilDone:NO];

Move waitUntilDone: to its own line; mixing newlines and spaces makes it easy to miss an argument when reading.


Sorry I missed those bigger issues in the last review.

Comment 97

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

This isn't blocking a2 (per the meeting a couple weeks ago), especially since the patch doesn't have r+ yet.

Comment 98

[Nick Kreeger]

Attachment: Patch V4

Another go-round.

Comment 99

[Stuart Morgan]

Comment on attachment 250429 [details] [diff] [review]
Patch V4

>+-(void)startPolling
>+{
>+  @synchronized(self) {
>+    if (!mThreadIsRunning) {
>+      mShouldRunThread = YES;
>+      mThreadIsRunning = YES;
>+      [NSThread detachNewThreadSelector:@selector(pollWatchedDirectories) 
>+                               toTarget:self 
>+                             withObject:nil];
>+    }
>+  }
>+}
...
>+  while (mShouldRunThread) {
>+    NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init];
>+    NS_DURING
>+      struct kevent event;
>+      int n = kevent(mQueueFileDesc, NULL, 0, &event, 1, &timeInterval);
>+      if (n > 0 && event.filter == EVFILT_VNODE && event.fflags)
>+        [(id<WatchedFileDelegate>)event.udata fileDidChange];
>+    NS_HANDLER
>+      NSLog(@"Error in watcherThread: %@", localException);
>+    NS_ENDHANDLER
>+      
>+    [pool release];
>+  }
>+    
>+  close(mQueueFileDesc);
>+  mThreadIsRunning = NO;
>+}

Having the startPolling stuff synchronized without doing synchronization in the background thread doesn't help; all it does is prevent two different threads from running startPolling at the same time, which we are assuming isn't a valid situation since it's not threadsafe for clients.

There's still a case now where we can completely lose the backrgound thread:
a) main thread removes the last watch; mShouldRunThread is now NO
b) watcher thread sees the no, exits the loop, and reaches the |close|
c) main thread adds a new watcher; startPolling sees that mThreadIsRunning is true, and does nothing
d) watcher thread exits
We need the case where the thread is going to exit and the updating of mThreadIsRunning to be atomic from other threads' perspectives.

I think this should work (leaving startPolling as you have it):
  while (1) {
    @synchronized(self) {
      if (!mShouldRunThread) {
        mThreadIsRunning = NO;
        break;
      }
    }
    // main body of the loop
  }
(Assuming break from within a @synchronized block breaks from the |while| as I would expect; otherwise just set a local bool and break just after if it's set)


You also need to revisit the logic around mQueueFileDesc. Right now it's opened exactly once in |init|, but closed at the end of the background thread, so the descriptor won't work after the first time there are no items to watch. The easiest approach is probably to just close it in dealloc (you don't have to worry about how that interacts with the background thread since dealloc can't be called while the background thread is running).

Comment 100

[Nick Kreeger]

Attachment: Patch V5

Here is the next go-round of the patch. I did some extra testing to make sure PVC's where getting released and removed from the watch queue when they were removed from the downloads list view. I found that having a reference to a PVC in the member array of |FileChangeWatcher| prevents the instance of a PVC from releasing (when removed from the member array in PDC). Therefore I added a member BOOL to validate if a PVC instance is in the watch queue to PVC, and added |displayWillBeRemoved:| to the |CHDownloadProgressDisplay| protocol. PDC will call |displayWillBeRemoved| when a PVC is getting removed from the array (actually right before the removeFromArray call). This will ensure that a PVC gets deallocated when removed from the member array in PDC that holds all the PVCS.  

phew.

Comment 101

[Stuart Morgan]

Comment on attachment 252258 [details] [diff] [review]
Patch V5

>+ * watch queue. When an object is added to the queue, a weak ref
>+ * is created to the object, therefore the object needs to remove
>+ * itself from the queue on |dealloc|.

So this part of the comment needs to be removed.  Sorry, I got hung up on that in-method comment about weak-ref'ing and missed the fact that this class is, as you discovered, keeping a strong ref to the delegate.

>+-(void)addFileDelegateToQueue:(id<WatchedFileDelegate>)aWatchedFileDelegate;
>+-(void)removeFileDelegateFromQueue:(id<WatchedFileDelegate>)aWatchedFileDelegate;

Could we not use the word Queue in the interface? It's not actually queuing anything, so it feels like it's just named this because of the underlying kqueue, which is an implementation detail. Maybe add/removeWatchedFileDelegate:?

>+-(void)stopPolling
>+{
>+  mShouldRunThread = NO;
>+}

This should be @synchronized, since it's read on another thread.

>+  mThreadIsRunning = NO;
>+}

Remove this.

>+ * Adding an object to the watch queue creates a weak ref to the object,
>+ * therefore that object needs to remove itself from the queue on |dealloc|.

And this

>+-(void)addFileDelegateToWatchQueue:(id<WatchedFileDelegate>)aWatchedFileDelegate;
>+-(void)removeFileDelegateFromWatchQueue:(id<WatchedFileDelegate>)aWatchedFileDelegate;

s/Queue/List/ ?

>+- (void)displayWillBeRemoved;

I hate to see the burden of dealing with the retain cycle put on the users of this class. Could this be done in |viewWillMoveToSuperview:| (when called with nil) or something of a similar nature so that it's all taken care of automatically?

Either way, please call out the retain loop in setupFileSystemNotification so that it's clear to someone maintaining this class why things need to be cleaned up somewhere other than dealloc.


I think we're finally in the home stretch here; sorry this has dragged out.

Comment 102

[Nick Kreeger]

Attachment: Patch V6

Go round 6, I removed the view will be removed stuff from ProgressDlgController, and made them the responsibility of ProgressView/ProgressViewController.

Comment 103

[Stuart Morgan]

Comment on attachment 253405 [details] [diff] [review]
Patch V6

Neither PVC nor PDC will build without including FileChangeWatcher.h (since they refer to the protocol).

There's also a warning stemming from the fact that didStartDownloa: takes an id<CHBownloadProgressDisplay>, then passes it to scrollIntoView:, which takes a PVC. Since PVCs are supposed to conform to two protocols, but you only know you have one of them, it is unhappy. I haven't chased it enough to know if scrollIntoView: could be changed to take an id<CHBPD>, but something needs to be done there since it's code that purports to work with just the protocol, but doesn't really. (If it would become a big mess to fix that, I'm okay with landing without that and filing a bug to fix it, but I'd rather not if we can avoid it.)

Behaviorally, I actually tested this version (yay!) and the issue I found is that the viewWillBeRemovedFromSuperview thing won't fly; that gets called all the freaking time. Every time that stack view decides to lay itself out again, it rips all the views out and starts over. I guess for now, in the interests of getting this in, lets go back to your model of having the PDC tell the PVC it's all done, and we can revisit it in the Great Download Manager View rewrite that we want to see happen down the road.

Nits:

>+  if (mFileIsWatched)
>+    [self unsubscribeFileSystemNotification];

This should be removed from the dealloc method as it's misleading; it's impossible for dealloc to be called if it's currently on the watch list.

>+-(void)viewWillBeRemovedFromSuperview
>+{
>+  // The file can only be watched if the download compeleted sucessfully
>+  if (mFileIsWatched) {
>+    [self unsubscribeFileSystemNotification];

That comment doesn't make sense here; was it supposed to be in onEndDownload:statusCode: below?

This does need a comment about why it need to be here (namely that it's owned by the file watcher, and will never be dealloc'd otherwise). Probably there should be a similar comment in setupFileSystemNotification, since that's where the extra retain starts.

Comment 104

[Stuart Morgan]

(In reply to comment #103)
> This does need a comment about why it need to be here (namely that it's owned
> by the file watcher, and will never be dealloc'd otherwise). Probably there
> should be a similar comment in setupFileSystemNotification, since that's where
> the extra retain starts.

Sorry, I wrote this before I found the behavioral problem. But please do put a comment to that effect in the displayWillBeRemoved that you put back in.

Comment 105

[Nick Kreeger]

Attachment: Patch V7

This patch includes the latest changes, but uses the |viewWillBeRemoved:| setup.

Comment 106

[Stuart Morgan]

Comment on attachment 254979 [details] [diff] [review]
Patch V7

>+/**
>+ * Any object that needs a file to be polled can implement this protocol and 
>+ * register itself with the watch queue.
>+ */

Change all these comments to c-style (//), since that's our prevailing style.

>Index: src/download/FileChangeWatcher.mm

Unless there's something I missed that this pulls in, this should be a .m file.

>+-(void)startPolling
>+{
>+  @synchronized(self) {
>+    if (!mThreadIsRunning) {
>+      mShouldRunThread = YES;
>+      mThreadIsRunning = YES;

This should be:

  @synchronized(self) {
    mShouldRunThread = YES;
    if (!mThreadIsRunning) {
      mThreadIsRunning = YES;

To prevent there being a 0-60 second window where clearing the downloads and starting a new one will result in the thread incorrectly dying.

>   NSTimer               *mDownloadTimer;            // used for updating the status, STRONG ref
>   NSMutableArray        *mProgressViewControllers;  // the downloads we manage, STRONG ref
...
>+  FileChangeWatcher*    mFileChangeWatcher;         // strong ref.

The * location needs to be consistent here (I don't really care which you change)

With those changes... r=me! Yay!

Comment 107

[Nick Kreeger]

Attachment: Patch V8

SR ready patch, Mento we had one warning that we would like your opinion on:

> int n = kevent(mQueueFileDesc, NULL, 0, &event, 1, &timeInterval);

complains about param 6 (timeInterval) of kevent discarding the qualifiers from pointer target type. The poll works just fine with the warning, but we weren't sure if we need to be using the heap for our |struct timespec timeInterval = { 60, 0 };| local.

Comment 108

[Nick Kreeger]

> complains about param 6 (timeInterval) of kevent discarding the qualifiers from

to clarify, from the man page of kevent:
kevent(int kq, const struct kevent *changelist, int nchanges,
         struct kevent *eventlist, int nevents,
         const struct timespec *timeout);

Comment 109

[Simon Fraser [no longer active]]

Mento we had one warning that we would like your opinion on:
> 
> > int n = kevent(mQueueFileDesc, NULL, 0, &event, 1, &timeInterval);
> 
> complains about param 6 (timeInterval) of kevent discarding the qualifiers from
> pointer target type. The poll works just fine with the warning, but we weren't
> sure if we need to be using the heap for our |struct timespec timeInterval = {
> 60, 0 };| local.

You shouldn't have to use the heap. Did you try declaring it to be const?
const struct timespec timeInterval = { 60, 0 };

Comment 110

[Nick Kreeger]

> You shouldn't have to use the heap. Did you try declaring it to be const?
> const struct timespec timeInterval = { 60, 0 };
> 

Yeah I tried that before posting the patch.

Comment 111

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

Attachment: Trunk project patch

Comment 112

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

Attachment: Branch project patch

Comment 113

[Mark Mentovai]

Comment on attachment 255061 [details] [diff] [review]
Patch V8

>Index: src/download/FileChangeWatcher.m

>+const int kPollingInterval = 60;

You didn't use this anywhere, but you should.  You should indicate the units used (seconds) either in the constant name itself or in a comment at its declaration.

As for your warning, this line:

>+  struct timespec timeInterval = { 60, 0 };

should be:

  const struct timespec timeInterval = { kPollingInterval, 0 };

as Simon said.  I can't imagine that wouldn't work, but if it really doesn't, then we'll talk about it some more.

Comment 114

[Nick Kreeger]

Attachment: Final Patch

This patch addresses Mark's comments.

In regards to our mystery compiler warning from |kevent()|, even with the |const| modifier we see the warning with gcc 3.3. However, we do not see the warning with gcc 4.0.

Comment 115

[Nick Kreeger]

Checked into the MOZILLA_1_8_BRANCH, trunk patch coming soon...

Comment 116

[froodian (Ian Leue)]

Reopening, since this isn't checked in on trunk yet.  The fixed1.8.1.3 documents its having been checked in on the branch.

Comment 117

[Nick Kreeger]

Landed on HEAD, now closing..... yay!

Comment 118

[Dachaz]

I'm not sure whither does my problem fit here or as an new ticket, however, I never got the beachball and hanging but Camino crashes almost *always* when I'm downloading more than one file!!

Comment 119

[Chris Lawson (gone)]

(In reply to comment #118)
> I'm not sure whither does my problem fit here or as an new ticket, however, I
> never got the beachball and hanging but Camino crashes almost *always* when I'm
> downloading more than one file!!

This bug describes a hang, not a crash, so you should file a new bug for that, after you make sure bug 351504 isn't what you're seeing.

Comment 120

[Nick Kreeger]

(In reply to comment #118)
> I'm not sure whither does my problem fit here or as an new ticket, however, I
> never got the beachball and hanging but Camino crashes almost *always* when I'm
> downloading more than one file!!
> 

Can you post the crash stack for the crash you are seeing here please?

Comment 121

[Nick Kreeger]

Also, are you running any hacks (like the Growl hack)

Comment 122

[Stuart Morgan]

(In reply to comment #120)
> Can you post the crash stack for the crash you are seeing here please?

Actually, please don't; file a new bug and we'll follow up there. We really don't want to mix it in with this bug.

Comment 123

[Nick Kreeger]

Please post this information in a new bug...