670068 - Open Containing Folder spawns multiple instances of explorer.exe

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[John]

Attachment: FIREFOX_EXPLORER.EXE_MULTIPLE_INSTANCE_PROOF

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.

Comment 1

[Brian R. Bondy [:bbondy]]

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.

Comment 2

[Brian R. Bondy [:bbondy]]

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.

Comment 3

[Brian R. Bondy [:bbondy]]

Attachment: Open Containing Folder spawns multiple instances of explorer.exe

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.

Comment 4

[Benjamin Smedberg]

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.

Comment 5

[Robert Strong (they/them - no direct email)]

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.

Comment 6

[Brian R. Bondy [:bbondy]]

Attachment: Patch for Open Containing Folder spawns multiple instances of explorer.exe

Removed redundant calls to ResolveAndStat

Comment 7

[Brian R. Bondy [:bbondy]]

Attachment: Patch for Open Containing Folder spawns multiple instances of explorer.exe

Same as last patch, but last patch had to be rebased due pushed changes in mozilla-central.

Comment 8

[Robert Strong (they/them - no direct email)]

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.

Comment 9

[[e-mail]]

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 10

[Jim Mathies [:jimm]]

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

Comment 11

[Brian R. Bondy [:bbondy]]

Attachment: Patch for Open Containing Folder spawns multiple instances of explorer.exe

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

Comment 12

[Jim Mathies [:jimm]]

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.

Comment 13

[Brian R. Bondy [:bbondy]]

Attachment: Patch for Open Containing Folder spawns multiple instances of explorer.exe w/ review comments

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

Comment 14

[Brian R. Bondy [:bbondy]]

Attachment: Patch for Open Containing Folder spawns multiple instances of explorer.exe w/ review comments

Comment 15

[Benoit Jacob [:bjacob] (mostly away)]

This was r+'d 2 weeks ago. Are you planning to land it?

Comment 16

[Brian R. Bondy [:bbondy]]

bjacob, I'l run it through try today and if all goes well I'll push it by tomorrow.

Comment 17

[Brian R. Bondy [:bbondy]]

Pushed to try: 
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e341b065b122

Comment 18

[Brian R. Bondy [:bbondy]]

Pushed to mozilla-inbound: 
http://hg.mozilla.org/integration/mozilla-inbound/rev/0907fce1e6fc

Comment 19

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/0907fce1e6fc

Comment 20

[Jim Jeffery not reading bug-mail 1/2/11]

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.

Comment 21

[Brian R. Bondy [:bbondy]]

Will be fixed in 685847

Comment 23

[[e-mail]]

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.

Comment 24

[Brian R. Bondy [:bbondy]]

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

Comment 25

[[e-mail]]

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

Comment 26

[Masatoshi Kimura [:emk]]

(In reply to [e-mail] 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!)