403014 - Windows' "Don't Index" bit set on download files

Status: RESOLVED FIXED

(Firefox :: Shell Integration, defect, P2)

Description

[Tom Edwards]

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.

Comment 1

[Tom Edwards]

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

Comment 2

[Henry Palmroos]

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?

Comment 3

[Mike Beltzner [:beltzner, not reading bugmail]]

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 ...

Comment 4

[Jim Mathies [:jimm]]

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.

Comment 5

[Jim Mathies [:jimm]]

> tell me if it needs to block?

I'd say yes. It looks to be a pretty easy fix.

Comment 6

[Jim Mathies [:jimm]]

Attachment: set attrib patch v.1

tested - fixes the problem.

Comment 7

[Jim Mathies [:jimm]]

Attachment: set attrib patch v.1

blah - bad patch fixed.

Comment 8

[Jim Mathies [:jimm]]

Attachment: set attrib patch v.1

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

Comment 9

[Doug Turner (:dougt)]

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?

Comment 10

[Jim Mathies [:jimm]]

Attachment: set attrib patch v.2

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 11

[Doug Turner (:dougt)]

Comment on attachment 313447 [details] [diff] [review]
set attrib patch v.2

why do you are about checking:
INVALID_FILE_ATTRIBUTES

Comment 12

[Jim Mathies [:jimm]]

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.

Comment 13

[Mike Beltzner [:beltzner, not reading bugmail]]

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

Comment 14

[Jim Mathies [:jimm]]

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

Comment 15

[Mike Connor [:mconnor]]

P2, wanted.

Comment 16

[Jim Mathies [:jimm]]

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

Comment 17

[Jim Mathies [:jimm]]

(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 18

[Doug Turner (:dougt)]

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. :-)

Comment 19

[Radek 'sysKin' Czyz]

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.

Comment 20

[Jim Mathies [:jimm]]

(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 21

[Doug Turner (:dougt)]

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?

Comment 22

[Jim Mathies [:jimm]]

(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..

Comment 23

[Jim Mathies [:jimm]]

(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".

Comment 24

[Radek 'sysKin' Czyz]

>  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 25

[Jim Mathies [:jimm]]

Attachment: set attrib patch v.3

Comment 26

[Jim Mathies [:jimm]]

Comment on attachment 345123 [details] [diff] [review]
set attrib patch v.3

Updated. Can you hook me back up with that r+ Doug?

Comment 27

[mozilla]

 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.

Comment 28

[Radek 'sysKin' Czyz]

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 ;_;

Comment 29

[Jim Mathies [:jimm]]

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 30

[Jim Mathies [:jimm]]

Attachment: unbitroten patch v.3

Comment 31

[Doug Turner (:dougt)]

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

Comment 32

[Jim Mathies [:jimm]]

(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 33

[Jim Mathies [:jimm]]

Attachment: fileattributeswin patch v.1

Comment 34

[Jim Mathies [:jimm]]

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.

Comment 35

[Jim Mathies [:jimm]]

Attachment: fileattributeswin patch v.2

- 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 36

[Jim Mathies [:jimm]]

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

review? -> Shawn for the download manager changes

Comment 37

[Jim Mathies [:jimm]]

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

Comment 38

[Shawn Wilsher :sdwilsh]

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

Comment 39

[Jim Mathies [:jimm]]

(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.

Comment 40

[Shawn Wilsher :sdwilsh]

(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 41

[Shawn Wilsher :sdwilsh]

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

Canceling request until comment 38 and comment 40 are addressed.

Comment 42

[Radek 'sysKin' Czyz]

(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.

Comment 43

[Shawn Wilsher :sdwilsh]

(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.

Comment 44

[Radek 'sysKin' Czyz]

(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.

Comment 45

[Tom Edwards]

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.

Comment 46

[Jim Mathies [:jimm]]

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.

Comment 47

[Jim Mathies [:jimm]]

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.

Comment 48

[Radek 'sysKin' Czyz]

(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.

Comment 49

[Jim Mathies [:jimm]]

(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 50

[Doug Turner (:dougt)]

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

based on last comment.

Comment 51

[Jim Mathies [:jimm]]

Attachment: fileattributeswin patch v.3

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

Comment 52

[Shawn Wilsher :sdwilsh]

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 53

[Jim Mathies [:jimm]]

Comment on attachment 372683 [details] [diff] [review]
fileattributeswin patch v.3

-> dougt for xpcom changes.

Comment 54

[Doug Turner (:dougt)]

Comment on attachment 372683 [details] [diff] [review]
fileattributeswin patch v.3

please make sure that this builds on WINCE.  Otherwise looks fine.

Comment 55

[Jim Mathies [:jimm]]

Attachment: landed patch

http://hg.mozilla.org/mozilla-central/rev/a6b1b49f7d7e

Comment 56

[Jim Mathies [:jimm]]

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

Comment 57

[Shawn Wilsher :sdwilsh]

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

Comment 58

[Jim Mathies [:jimm]]

(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.

Comment 59

[Shawn Wilsher :sdwilsh]

(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.