Closed Bug 483481 Opened 16 years ago Closed 14 years ago

Under some situations we fail to set the initial directory for the file picker instance under Windows 7

Categories

(Firefox :: File Handling, defect)

Product:
Firefox
Component:
File Handling
Platform:
x86
Windows 7
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: Gavin, Unassigned)

References

Details

It would be great if a set of steps to reproduce can be laid out as stated in bug 464795 comment 23 and 24.  Also things like error console reports may be helpful as well.
Keywords: qawanted
Whiteboard: [PB-P3]
The steps are simple and easy:

1. Create a fresh profile
2. Open http://www.google.de/ and save the Google logo to a given folder
3. Start Private Browsing mode
4. Open http://www.google.de/ and save the Google logo to another folder
5. Stop Private Browsing mode
6. Open http://www.google.de/ and save the Google logo again

With step 6 you will see that the selected folder from within the Private Browsing mode is open.

I don't know what I could do more here right now.
Keywords: qawanted
(In reply to comment #2)
> The steps are simple and easy:
> 
> 1. Create a fresh profile
> 2. Open http://www.google.de/ and save the Google logo to a given folder
> 3. Start Private Browsing mode
> 4. Open http://www.google.de/ and save the Google logo to another folder
> 5. Stop Private Browsing mode
> 6. Open http://www.google.de/ and save the Google logo again
> 
> With step 6 you will see that the selected folder from within the Private
> Browsing mode is open.
> 
> I don't know what I could do more here right now.

Is anything reported to the error console during these steps?

Also, can you please report the value of the browser.download.lastDir pref after steps 2, 4 and 6 above?

Also, can you please test to see if this problem happens with a build without my second patch in bug 464795 on Windows 7 as well?
Keywords: qawanted
(In reply to comment #3)
> Also, can you please report the value of the browser.download.lastDir pref
> after steps 2, 4 and 6 above?

Do you have a short JS snippet which I can use to extract the folder as string?
 
> Also, can you please test to see if this problem happens with a build without
> my second patch in bug 464795 on Windows 7 as well?

Can try sure. But will wait for an answer to the above question.
Loading javascript:atob("<pref value>") should work.
(In reply to comment #4)
> (In reply to comment #3)
> > Also, can you please report the value of the browser.download.lastDir pref
> > after steps 2, 4 and 6 above?
> 
> Do you have a short JS snippet which I can use to extract the folder as string?

I don't understand the question.  For me, the value of that pref as seen in about:config is a plain string representing the path.

(In reply to comment #5)
> Loading javascript:atob("<pref value>") should work.

Hmm, is the value base64-encoded?
I see.  Out of curiosity, what do Mac path names contain which makes us base64-encode them?
I don't know the exact details, but we're serializing an AliasHandle object rather than simply a file path. I think the reasoning behind it is that in OS 9 the string representation of file paths weren't guaranteed to be unique.
Oh, ok. When it's OS X only I don't have to use it on Windows 7. I'll try to run the test within the next days.
(In reply to comment #3)
> > 2. Open http://www.google.de/ and save the Google logo to a given folder

target: C:\download
browser.download.lastDir = C:\download

> > 4. Open http://www.google.de/ and save the Google logo to another folder

before: browser.download.lastDir = C:\download
target: C:\download\Google_files
after: browser.download.lastDir = C:\download

> > 6. Open http://www.google.de/ and save the Google logo again

before: browser.download.lastDir = C:\download
target: C:\download\Google_files
after: C:\download\Google_files

With step 6 the google_files folder is preselected but not visible via the pref. No idea from where we get the folder information.

> Is anything reported to the error console during these steps?

No, the Error Console is empty.

> Also, can you please test to see if this problem happens with a build without
> my second patch in bug 464795 on Windows 7 as well?

Yes, same behavior with a trunk build from 03/13.
Everything looks like it should be.  I'm suspecting that this might be Windows 7's file picker component playing tricks on us.

Can you please modify toolkit/components/filepicker/Makefile.in and remove the checks for $(MOZ_WIDGET_TOOLKIT) (so that the XUL-based file picker component gets used instead of the OS component), build Firefox, and try the STRs again?
Ehsan, if you could find time to do this it would be great. I'm not really familiar with this stuff.
(In reply to comment #13)
> Ehsan, if you could find time to do this it would be great. I'm not really
> familiar with this stuff.

I don't have access to Windows 7 myself, but I submitted a try server build which you can use.  I'll post a link to it once it's available.
(In reply to comment #14)
> I don't have access to Windows 7 myself, but I submitted a try server build
> which you can use.  I'll post a link to it once it's available.

https://build.mozilla.org/tryserver-builds/2009-04-11_07:54-ehsan.akhgari@gmail.com-pbsaveaswin7/
Mmh, this file picker dialog doesn't look different to the Windows 7 one. The problem is still persistent. If I would debug where do I have to add a breakpoint? Probably this would help you more as when you have to poke in the darkness.
(In reply to comment #16)
> Mmh, this file picker dialog doesn't look different to the Windows 7 one.

Weird, maybe there's some other way to make the XUL file picker override the native OS one...

> The
> problem is still persistent. If I would debug where do I have to add a
> breakpoint? Probably this would help you more as when you have to poke in the
> darkness.

Yeah, that'd be great.

In Venkman, please find contentAreaUtils.js, and set a breakpoint on this line: <http://hg.mozilla.org/mozilla-central/file/9bda3eab3b2d/toolkit/content/contentAreaUtils.js#l589> in |getTargetFile|.  You want to see the value of |dir.path| on this line at step 6.  If Venkman doesn't work, it would be great if you can modify that file in toolkit.jar and add something like |Components.utils.reportError(dir.path);| after that line and watch the error console for the value of dir.path.

The value is expected to be C:\download.  If it's C:\download\Google_files, then please proceed with the STR again and this time put a breakpoint/dump code at <http://hg.mozilla.org/mozilla-central/file/9bda3eab3b2d/toolkit/content/contentAreaUtils.js#l555> and see how lastDir ends up with that value.

Thanks!
(In reply to comment #17)
> (In reply to comment #16)
> > Mmh, this file picker dialog doesn't look different to the Windows 7 one.
> 
> Weird, maybe there's some other way to make the XUL file picker override the
> native OS one...

IIRC, you have to set ui.allow_platform_file_picker to false in about:config, and on recent builds the default is true.
(In reply to comment #18)
> IIRC, you have to set ui.allow_platform_file_picker to false in about:config,
> and on recent builds the default is true.

No, that doesn't work. I'll have to use the debugger to get more information here.
Ok, so after my debugging session I can see that the folder we assign to the file picker is correct but somehow it doesn't get set. I tried under several situations and can even reproduce it without using pb mode. I change the lastDir folder manually via about:config and can see that it is not shown up. Instead the last folder is still shown. Does Windows 7 only use its internal data? But finally that should be the reason why it can be seen when the PB mode was used.

I don't have a debugging environment on that box and cannot debug the nsFilePicker instance but I believe that we fail in setting the mDisplayDirectory member (it get probably assigned a null value) and we are ending-up with the last used folder:

http://mxr.mozilla.org/mozilla-central/source/widget/src/windows/nsFilePicker.cpp#142

It would be great if a developer could have a look at this. I think that's all what QA can do here for now.

No idea if file handling is the correct component. But setting it for now and asking for blocking Firefox3.5.
Component: Private Browsing → File Handling
Flags: blocking-firefox3.5?
Keywords: qawanted
QA Contact: private.browsing → file.handling
Summary: "Save as" directory isn't persisted correctly when entering/leaving private browsing on Windows 7 → Under some situations we fail to set the initial directory for the file picker instance under Windows 7
Flags: wanted-firefox3.5+
Flags: blocking-firefox3.5?
Flags: blocking-firefox3.5-
Whiteboard: [PB-P3]
Hmm, reading <http://msdn.microsoft.com/en-us/library/ms646839%28v=vs.85%29.aspx>, the page says:

lpstrInitialDir
    LPCTSTR

    The initial directory. The algorithm for selecting the initial directory varies on different platforms.

    Windows 7:

        If lpstrInitialDir has the same value as was passed the first time the application used an Open or Save As dialog box, the path most recently selected by the user is used as the initial directory.
        Otherwise, if lpstrFile contains a path, that path is the initial directory.
        Otherwise, if lpstrInitialDir is not NULL, it specifies the initial directory.
        If lpstrInitialDir is NULL and the current directory contains any files of the specified filter types, the initial directory is the current directory.
        Otherwise, the initial directory is the personal files directory of the current user.
        Otherwise, the initial directory is the Desktop folder.

    Windows 2000/XP/Vista:

        If lpstrFile contains a path, that path is the initial directory.
        Otherwise, lpstrInitialDir specifies the initial directory.
        Otherwise, if the application has used an Open or Save As dialog box in the past, the path most recently used is selected as the initial directory. However, if an application is not run for a long time, its saved selected path is discarded.
        If lpstrInitialDir is NULL and the current directory contains any files of the specified filter types, the initial directory is the current directory.
        Otherwise, the initial directory is the personal files directory of the current user.
        Otherwise, the initial directory is the Desktop folder.

Note that the order of the initial directory lookup algorithm has been changed in Windows 7 (thanks, Microsoft!)

It might be possible to override this behavior by adding the OFN_NOCHANGEDIR flag to the OPEMFILENAME structure, although the docs are vague.  Jim, do you think that would make sense?
> If lpstrInitialDir has the same value as was passed the first time the
> application used an Open or Save As dialog box, the path most recently selected
> by the user is used as the initial directory.

Hmph, that is rather rude. They're adding functionality we've added since they didn't support it, and it doesn't appear they gave us a way to opt out.

> It might be possible to override this behavior by adding the OFN_NOCHANGEDIR
> flag to the OPEMFILENAME structure, although the docs are vague.  Jim, do you
> think that would make sense?

Look like we already do:

ofn.Flags = OFN_NOCHANGEDIR | OFN_SHAREAWARE | OFN_LONGNAMES | OFN_OVERWRITEPROMPT | OFN_HIDEREADONLY | OFN_PATHMUSTEXIST;

Maybe we could copy initialDir into fileBuffer and leave lpstrInitialDir null? Sounds like that might set the priority right and disable all the lpstrInitialDir processing.
Actually, this might have been fixed on trunk.  I didn't manage to reproduce it on trunk, but it seems to be easily reproducible in 3.6.
Keywords: qawanted
Whiteboard: WFM?
Seems to have been fixed somehow: bug 643152 comment 6.
Status: NEW → RESOLVED
Closed: 14 years ago
Keywords: qawanted
Resolution: --- → FIXED
Whiteboard: WFM?
You need to log in before you can comment on or make changes to this bug.