User-Agent: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; en-GB; rv:1.8.1.9) Gecko/20071025 Firefox/2.0.0.9 Forum thread here: http://forums.mozillazine.org/viewtopic.php?t=591558 If a file is downloaded through the standard dialogue or dragged onto a folder from a web page, Firefox will add the '+I' bit to it that tells Windows not to index that file. This is a major issue for Vista users, as it prevents the file from appearing in instant searches. Files downloaded through the "Save As..." context menu item do NOT suffer from this issue. (I'm not sure if this is major or not. Instant search is undoubtedly a major thing being broken, but it's not itself a component of Fx!) Reproducible: Always Steps to Reproduce: 1. Download a file from the standard file download dialogue, or by dragging it from a page to a folder. 2. Run ATTRIB on it from the command line, or check it's advanced attribute properties from the Properties context item. Actual Results: The file was set to be ignored by Windows' indexer. Expected Results: The file should not have been set to be ignored by Windows' indexer. It should not have added the +I attribute.

Soooo...did I forget to do something here?

I can confirm this. Exactly the same thing happening to me on a brand new English language version of Windows Vista Home Premium 64-bit + Service Pack 1. Left clicking the link in post and the file downloaded has the "Index this file for faster searching" is cleared, thus Windows Vista's built-in-fast search won't find the file. But right clicking the link, and choosing "Save as" will result a correct file with the "Indexing" bit set, and thus Vista will find the file instantly. Tested with FireFox 3 Beta 4. In Opera/IE the file has correct bit set no matter how I download the file. Any ideas?

Jim, can you look at this and tell me if it needs to block? Right now I'm thinking it's something we can fix after shipping ...

Files get written out in various different ways, and the attrib FILE_ATTRIBUTE_NOT_CONTENT_INDEXED does not get set correctly in certain circumstances. The "save as" seems to do a direct file save to the destination, in which case the bit gets set. (The file is created using OpenFile in nsILocalFileWin, where attribs passed are NULL. Vista is probably setting the right flag in this case.) In the case of a file that gets clicked on, those get saved out to a temp location first, then get moved to the destination using MoveFileEx. I'll bet the temp file creation causes the lack of the attrib. We probably need to add a SetFileAttributes call in nsILocalFileWin's CopySingleFile. (Not sure if we should add that for all cases where CopySingleFile is used at this point.) I also checked our ole drag and drop saves, those look to be setting it correctly.

> tell me if it needs to block? I'd say yes. It looks to be a pretty easy fix.

tested - fixes the problem.

blah - bad patch fixed.

Argh. Doug please don't shoot me. Bad bug number in comment fixed!

Comment on attachment 313427 [details] [diff] [review] set attrib patch v.1 i am not sure I like this. If I move a file that is readonly (or any other flag), we are going to strip the attributes with this change, right? Maybe we should conditionally do this or do a GetFileAttributesW, then set with the result masking out the FILE_ATTRIBUTE_NOT_CONTENT_INDEXED flag?

good point! Also note, use of FILE_ATTRIBUTE_NOT_CONTENT_INDEXED should be ok, I found it in the original winnt.h distributed with VC6.

Comment on attachment 313447 [details] [diff] [review] set attrib patch v.2 why do you are about checking: INVALID_FILE_ATTRIBUTES

If the call fails, that's what it would return. It's basically an error check on the result of GetFileAttribs before changing the attributes on the file.

This will not block the final release of Firefox 3. Any patch will need unit tests in order to be approved.

I'll roll some tests and repost here in a bit.. this should get in even though it's not blocking.

P2, wanted.

I experimented with a full blown c++ test case but it just seemed like total overkill for a simple file attribute change.

(In reply to comment #11) > (From update of attachment 313447 [details] [diff] [review]) > why do you are about checking: > INVALID_FILE_ATTRIBUTES Did that make sense Doug? The call returns INVALID_FILE_ATTRIBUTES in cases where the file can not be accessed / found.

Comment on attachment 313447 [details] [diff] [review] set attrib patch v.2 drop the :: in front of the method names. It is not required. drop the explict (void). It is also not needed -- i know you don't care about the error in this case because you are checking it. :-)

The patch looks very similar to bug 224692's patch, except it deals with attribute rather than permissions. Sooo can't it use the same "trick" of actually applying folder's attributes to the file? Instead of disabling just one particular attribute, just blanket copy them all from parent directory. This would also solve reminder of bug 224692 because it would apply compression and encryption as expected.

(In reply to comment #19) > The patch looks very similar to bug 224692's patch, except it deals with > attribute rather than permissions. > > Sooo can't it use the same "trick" of actually applying folder's attributes to > the file? Instead of disabling just one particular attribute, just blanket copy > them all from parent directory. > > This would also solve reminder of bug 224692 because it would apply compression > and encryption as expected. Sounds like we need to sort out all the various file situations in the download manager. I know there's some strangeness when the download is canceled and restarted, I don't believe the part file comes into play then. There's also an open bug (bug 452461) about .part file issues that might play into this as well.

Comment on attachment 313447 [details] [diff] [review] set attrib patch v.2 hold off on this patch. the problem as pointed out on irc by syskin is that we are inheriting the flags of the temp directory during download. clearly we do not need a bandaide in localfile to fix this. other ideas?

(In reply to comment #21) > (From update of attachment 313447 [details] [diff] [review]) > hold off on this patch. > > the problem as pointed out on irc by syskin is that we are inheriting the flags > of the temp directory during download. clearly we do not need a bandaide in > localfile to fix this. > > other ideas? I'll wait for sysKin to file that patch, and figure out who wants to work on it. I like the idea of a more broad solution that applies all attribs. now that we know there are other issues. The download manage behavior is quirky at best, I think I'll dig into it a bit and figure out the logic behind all the temp files and moves, etc..

(In reply to comment #22) > (In reply to comment #21) > > (From update of attachment 313447 [details] [diff] [review] [details]) > > hold off on this patch. > > > > the problem as pointed out on irc by syskin is that we are inheriting the flags > > of the temp directory during download. clearly we do not need a bandaide in > > localfile to fix this. > > > > other ideas? > > I'll wait for sysKin to file that patch, and figure out who wants to work on er, "bug" not "patch".

> er, "bug" not "patch". Phew, I was about to start writing a patch ;) Fortunately I decided to dig some more and it seems that my solution was too simplistic to be possible. As much as I was right saying that some attributes are inherited from temporary directory, and it's incorrect (and FILE_ATTRIBUTE_NOT_CONTENT_INDEXED is just one of casualties) it's also the only attribute that can be fixed within scope of this bug. See http://msdn.microsoft.com/en-us/library/aa363874%28VS.85%29.aspx , bottom table. Compression and encryption can't be activated by setting an attribute at all, they need their own function calls. So, I apologise for the mess, but blanket copy of all attributes is not the solution. Ideally this patch would copy FILE_ATTRIBUTE_NOT_CONTENT_INDEXED from download directory, but the only use case for that would be if user doesn't want his downloads indexed. Unlikely. In other words, attachment 313447 [details] [diff] [review] is as good as it gets, and compression and encryption need more code.

Comment on attachment 345123 [details] [diff] [review] set attrib patch v.3 Updated. Can you hook me back up with that r+ Doug?

Thanks for working on this. It's been quite a problem for a while now since it introduces so many false negatives into Vista search results.

Hi, I can't help noticing that my premature comment 19 stopped a good patch in its tracks. Please kindly please don't make me feel I stopped the fix ;_;

Yeah sorry this totally dropped off the radar. I'll get a hold of dougt and see if he'll hook me up with an r+ so this can land. It's probably not going to make it into 3.1 though, but we can ask for approval.

Comment on attachment 359940 [details] [diff] [review] unbitroten patch v.3 I do not like the idea of generally applying this "Remove the FILE_ATTRIBUTE_NOT_CONTENT_INDEX flag". I would be happy with you adding accessors to Set/Get FileAttributes() which will enable you to make these modifications to the downloaded file in the download manager: http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsILocalFileWin.idl#43

(In reply to comment #31) > (From update of attachment 359940 [details] [diff] [review]) > I do not like the idea of generally applying this "Remove the > FILE_ATTRIBUTE_NOT_CONTENT_INDEX flag". > > I would be happy with you adding accessors to Set/Get FileAttributes() which > will enable you to make these modifications to the downloaded file in the > download manager: > > http://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsILocalFileWin.idl#43 Sounds good, will do. Thanks.

Comment on attachment 371465 [details] [diff] [review] fileattributeswin patch v.1 This is just the base file attributes code. I'll make changed to the download manager in another patch.

- added download manager code - touched up idl commenting - added a shunt on < XP systems so the call doesn't fail when generically setting the search attrib..

Comment on attachment 371878 [details] [diff] [review] fileattributeswin patch v.2 review? -> Shawn for the download manager changes

Shawn, had one question - should that include of the nsLocalFileWin in download manager be ifdef'd? I wasn't sure if that was needed.

So, I'm a little concerned about this. What about downloads that are saved with "Open With..."? They will be in the temp directory, but we'll still turn on indexing? Also, while dougt said no casting to void when you don't check the return variable, that is the code style in nsDownloadManager.cpp

(In reply to comment #38) > So, I'm a little concerned about this. What about downloads that are saved > with "Open With..."? They will be in the temp directory, but we'll still turn > on indexing? Is there a spot in the download manager where we move a file from temp to it's target that I could slip this into? > > Also, while dougt said no casting to void when you don't check the return > variable, that is the code style in nsDownloadManager.cpp Will do.

(In reply to comment #39) > Is there a spot in the download manager where we move a file from temp to it's > target that I could slip this into? Not really. There is some complex interaction going on between nsDownloadManager and nsExternalHelperAppsService sadly. You should probably just check if the folder this is in is the OS temp dir.

Comment on attachment 371878 [details] [diff] [review] fileattributeswin patch v.2 Canceling request until comment 38 and comment 40 are addressed.

(In reply to comment #38) > What about downloads that are saved > with "Open With..."? They will be in the temp directory, but we'll still turn > on indexing? Is this actually a problem? The file is valid and has a correct name, so it might as well show up on search results.

(In reply to comment #42) > Is this actually a problem? The file is valid and has a correct name, so it > might as well show up on search results. They'll be deleted on program exit.

(In reply to comment #43) > They'll be deleted on program exit. Uh you're right. I thought deleted files are never shown in search results (indexed or not) but you're correct, they do show, at least on vista - and with very bad UI too.

File in the temp folder won't show up in searches because the whole directory is excluded from the index. It seems to me like a good idea to leave the files inside it indexable so that they work as expected should they be moved to an indexed location.

I'm wondering if the solution here might be to apply the parent folder's attributes to new files. That way we don't have to hard code any temp directory checking.

So now I'm wondering if all this attrib stuff is needed, since we recently landed a patch that takes care of applying security data of the parent folder on downloaded files. (Bug 224692) There's also an open bug for encryption properties. (Bug 483192) While I wouldn't mind keeping the fileattrib stuff I've added to nsLocalFileWin, I'm wondering if applying this attrib should be more automatic. Doug didn't like the idea of that but now that we've added security data inheritance, I have to wonder if we should be doing the same for content indexing properties.

(In reply to comment #47) > While I wouldn't mind keeping the fileattrib stuff > I've added to nsLocalFileWin, I'm wondering if applying this attrib should be > more automatic. This sounds painfully similar to my comment #19 which I had to debunk in comment #24, but which still caused untold damage ;) to this bug's fixing process. In short: each of those (security, compression, encryption, attributes) is a completely separate Windows OS call and there's no single process for getting them across. Each needs a separate (if somewhat similar) patch.

(In reply to comment #48) > (In reply to comment #47) > > While I wouldn't mind keeping the fileattrib stuff > > I've added to nsLocalFileWin, I'm wondering if applying this attrib should be > > more automatic. > > This sounds painfully similar to my comment #19 which I had to debunk in > comment #24, but which still caused untold damage ;) to this bug's fixing > process. > > In short: each of those (security, compression, encryption, attributes) is a > completely separate Windows OS call and there's no single process for getting > them across. Each needs a separate (if somewhat similar) patch. Alright, I'm going to add the temp dir check to the download manager code. We'll go with that until we (if ever) revamp how we handle downloads.

Comment on attachment 371878 [details] [diff] [review] fileattributeswin patch v.2 based on last comment.

Alright, here's the update. Tested it on various different types of downloads, everything looks to be working correctly.

Comment on attachment 372683 [details] [diff] [review] fileattributeswin patch v.3 nit: no brackets around single line if statements r=sdwilsh with that fixed.

Comment on attachment 372683 [details] [diff] [review] fileattributeswin patch v.3 -> dougt for xpcom changes.

Comment on attachment 372683 [details] [diff] [review] fileattributeswin patch v.3 please make sure that this builds on WINCE. Otherwise looks fine.

Note, the test for this was disabled permanently due to tinderbox using FAT32 file systems. (Bug 489285) http://hg.mozilla.org/mozilla-central/rev/647a8e601187

Can we check what filesystem we are on and conditionally run these tests?

(In reply to comment #57) > Can we check what filesystem we are on and conditionally run these tests? Thought about that, but wasn't sure if we would want situations where locally a test is run but on tinderbox it's not creating a disconnect between the two.

(In reply to comment #58) > Thought about that, but wasn't sure if we would want situations where locally a > test is run but on tinderbox it's not creating a disconnect between the two. We should probably get boxes to test NTFS then too, especially if it'd be common for our users to have.