Closed Bug 670068 Opened 8 years ago Closed 8 years ago

Open Containing Folder spawns multiple instances of explorer.exe

Categories

(Core :: XPCOM, defect)

5 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: j-ottone, Assigned: bbondy)

References

Details

Attachments

(2 files, 5 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:5.0) Gecko/20100101 Firefox/5.0
Build ID: 20110615151330

Steps to reproduce:

After downloading a file, on the Downloads window, user right clicks the file(to display context menu), and selects "Open Containing Folder".

This is verifed on Windows 7 64 bit. I did not test this on Windows Vista or Windows XP...


Actual results:

A new instance of explorer.exe is spawned. This occurs EVERY single time. So after a day of downloading, a user could have tons of explorer.exe instances running. Even after you close the folder, all accumulated explorer.exe processes remain in memory.


Expected results:

A new instance of explorer.exe should not be spawned. There should only be ONE instance of explorer.exe running at any given time.
OS: Other → Windows 7
Hardware: All → x86_64
Component: General → Download Manager
Product: Firefox → Toolkit
QA Contact: general → download.manager
I can reproduce this problem.  It always starts a new explorer.exe even know the following option is turned off.

> Tools | Options | Launch folder windows in a separate process

This option corresponds to this reg entry which is also set to 0 for me:
HKEY_CURRENT_USER\Software\Microsoft\Windows\CurrentVersion\Explorer\Advanced\SeparateProcess = 0

Marking Status to NEW, and I'll take this bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Assignee: nobody → netzen
The explorer.exe process will eventually reclaim themselves if the explorer window is closed, but it can take a long time and each window that is opened starts an explorer.exe ~13MB of RAM, so this should be fixed.
This patch fixes the problem, i.e. will not re-launch another explorer.exe process.

This patch will first try revealing the path using the new shell method which was introduced in Windows XP:
http://msdn.microsoft.com/en-us/library/bb762232(v=vs.85).aspx

If that fails, or if the OS build was targeted for a version before XP it will fall back to the old explorer.exe command line way.
Attachment #546777 - Flags: review?(benjamin)
Assignee: netzen → nobody
Component: Download Manager → XPCOM
Product: Toolkit → Core
QA Contact: download.manager → xpcom
Assignee: nobody → netzen
Comment on attachment 546777 [details] [diff] [review]
Open Containing Folder spawns multiple instances of explorer.exe

I'm going to bump this review to :rs since he's the expert in this stuff.
Attachment #546777 - Flags: review?(benjamin) → review?(robert.bugzilla)
Comment on attachment 546777 [details] [diff] [review]
Open Containing Folder spawns multiple instances of explorer.exe

Quick drive by before I do a full review... looks like there are redundant calls to ResolveAndStat.
Removed redundant calls to ResolveAndStat
Attachment #546777 - Attachment is obsolete: true
Attachment #547212 - Flags: review?(robert.bugzilla)
Attachment #546777 - Flags: review?(robert.bugzilla)
Same as last patch, but last patch had to be rebased due pushed changes in mozilla-central.
Attachment #547212 - Attachment is obsolete: true
Attachment #549414 - Flags: review?(robert.bugzilla)
Attachment #547212 - Flags: review?(robert.bugzilla)
Comment on attachment 549414 [details] [diff] [review]
Patch for Open Containing Folder spawns multiple instances of explorer.exe

Asking jimm for review since I've been away for a week and it will be awhile before I dig myself out.
Attachment #549414 - Flags: review?(jmathies)
Blocks: 156422
Note that the old code simply launches a folder, rather then selecting it, while the new code appears to select the object whether or not it's a folder.
Comment on attachment 549414 [details] [diff] [review]
Patch for Open Containing Folder spawns multiple instances of explorer.exe

Review of attachment 549414 [details] [diff] [review]:
-----------------------------------------------------------------

Line endings are out of whack in some of these files.

::: xpcom/io/nsLocalFileWin.cpp
@@ +2731,5 @@
> +  rv = parentDirectory->GetPath(parentDirectoryPath);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  // Set the directory to open
> +  ITEMIDLIST *dir = ILCreateFromPathW(parentDirectoryPath.get());

This will break builds running on 2K. If we can grab this dynamically without much fuss lets do it.

@@ +2745,5 @@
> +  const ITEMIDLIST* selection[] = { item };
> +  UINT count = sizeof(selection) / sizeof(ITEMIDLIST);
> +
> +  //Perform selection
> +  HRESULT hr = SHOpenFolderAndSelectItems(dir, count, selection, 0);

ditto here
Attachment #549414 - Flags: review?(jmathies) → review-
- Fixed line endings to Unix
- Now load shell32.dll functions dynamically so we will work on Win2k without compiling as Win2k
- Now check if we have a directory, and if so open that item directly.  As per Neil's Comment 9.
Attachment #549414 - Attachment is obsolete: true
Attachment #550203 - Flags: review?(jmathies)
Comment on attachment 550203 [details] [diff] [review]
Patch for Open Containing Folder spawns multiple instances of explorer.exe

Review of attachment 550203 [details] [diff] [review]:
-----------------------------------------------------------------

nit - the header has some windows line endings in it. :)

::: xpcom/io/nsLocalFileWin.cpp
@@ +2704,5 @@
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  nsAutoString explorerPath;
> +  rv = winDir->GetPath(explorerPath);  
> +  NS_ENSURE_SUCCESS(rv, rv);
> +  explorerPath.Append(L"\\explorer.exe");

Use AppendLiteral with these.

@@ +2715,5 @@
> +  if (mFileInfo64.type != PR_FILE_DIRECTORY) // valid because we ResolveAndStat above
> +    explorerParams.Append(L"/n,/select,");
> +  explorerParams.Append(L'\"');
> +  explorerParams.Append(mResolvedPath);
> +  explorerParams.Append(L'\"');

ditto.
Attachment #550203 - Flags: review?(jmathies) → review+
- Fixed newlines
- Fixed Append to AppendLiteral

I changed these ones:
 
> explorerPath.Append(L"\\explorer.exe");
> explorerParams.Append(L"/n,/select,");

But I don't think we can change the single char ones because I don't think there is such an overload for a single char.
Attachment #550203 - Attachment is obsolete: true
Attachment #551512 - Flags: review?(jmathies)
Attachment #551512 - Attachment is obsolete: true
Attachment #551513 - Flags: review?(jmathies)
Attachment #551512 - Flags: review?(jmathies)
Attachment #551513 - Flags: review?(jmathies) → review+
This was r+'d 2 weeks ago. Are you planning to land it?
bjacob, I'l run it through try today and if all goes well I'll push it by tomorrow.
Pushed to mozilla-inbound: 
http://hg.mozilla.org/integration/mozilla-inbound/rev/0907fce1e6fc
Status: NEW → ASSIGNED
http://hg.mozilla.org/mozilla-central/rev/0907fce1e6fc
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9
With this patch the whole browser is now crashing on trying to open a file or folder from the Download Manager window.  

Thanks to Alice0775 for https://bugzilla.mozilla.org/show_bug.cgi?id=684506#c23.

Win7 x64, m-c build 32bit latest hourly.
Will be fixed in 685847
Duplicate of this bug: 638695
I was looking into this because someone mentioned that the new reveal code raises an existing folder rather than opening a new folder window. Is that expected?

It's also not actually selecting the revealed file for me, in case that's related.
> I was looking into this because someone mentioned that the new reveal code raises an existing folder rather than opening a new folder window.

I don't remember if I noticed that was changed behaviour or not but I did confirm that this is the new behaviour just now.  It is using a standard shell API (SHOpenFolderAndSelectItems http://msdn.microsoft.com/en-us/library/windows/desktop/bb762232(v=vs.85).aspx) for this and I don't think there is an option to always use a new folder.  Functionality matches IE, Chrome, and what Windows wants applications to do.  Please feel free to post a new bug though if you think we should try to find another way, and make a suggestion if you have any in mind. Thanks.

> It's also not actually selecting the revealed file for me, in case that's related.

That sounds like a bug unrelated to the other thing you mentioned, so please post a new bug on that.  I am not able to reproduce that bug so I think more detail is needed there.
(In reply to Brian R. Bondy from comment #24)
> > It's also not actually selecting the revealed file for me, in case that's related.
> 
> That sounds like a bug unrelated to the other thing you mentioned, so please
> post a new bug on that.  I am not able to reproduce that bug so I think more
> detail is needed there.

It turned out to be a local issue, the problem is fixed now thanks.
No longer blocks: 716449
Depends on: 716449
(In reply to neil@parkwaycc.co.uk from comment #23)
> I was looking into this because someone mentioned that the new reveal code
> raises an existing folder rather than opening a new folder window.
> Is that expected?
Before bug 156422, an existing folder was reused. The behavior was changed to avoid the Win2k bug. I had planned to fix the regression when Win2k support is dropped. (Of course I don't have to fix it on my own anymore. Thank you, Brian!)
You need to log in before you can comment on or make changes to this bug.