Last Comment Bug 670068 - Open Containing Folder spawns multiple instances of explorer.exe
: Open Containing Folder spawns multiple instances of explorer.exe
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPCOM (show other bugs)
: 5 Branch
: x86_64 Windows 7
: -- normal with 1 vote (vote)
: mozilla9
Assigned To: Brian R. Bondy [:bbondy]
:
Mentors:
: 638695 (view as bug list)
Depends on: 685847 716449
Blocks: 156422
  Show dependency treegraph
 
Reported: 2011-07-07 19:49 PDT by John
Modified: 2012-02-02 00:45 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
FIREFOX_EXPLORER.EXE_MULTIPLE_INSTANCE_PROOF (158.41 KB, image/jpeg)
2011-07-07 19:49 PDT, John
no flags Details
Open Containing Folder spawns multiple instances of explorer.exe (5.26 KB, patch)
2011-07-19 06:52 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for Open Containing Folder spawns multiple instances of explorer.exe (5.06 KB, patch)
2011-07-20 13:06 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for Open Containing Folder spawns multiple instances of explorer.exe (5.12 KB, patch)
2011-07-29 10:41 PDT, Brian R. Bondy [:bbondy]
jmathies: review-
Details | Diff | Splinter Review
Patch for Open Containing Folder spawns multiple instances of explorer.exe (8.66 KB, patch)
2011-08-02 14:22 PDT, Brian R. Bondy [:bbondy]
jmathies: review+
Details | Diff | Splinter Review
Patch for Open Containing Folder spawns multiple instances of explorer.exe w/ review comments (8.66 KB, patch)
2011-08-08 11:33 PDT, Brian R. Bondy [:bbondy]
no flags Details | Diff | Splinter Review
Patch for Open Containing Folder spawns multiple instances of explorer.exe w/ review comments (8.67 KB, patch)
2011-08-08 11:34 PDT, Brian R. Bondy [:bbondy]
jmathies: review+
Details | Diff | Splinter Review

Description John 2011-07-07 19:49:58 PDT
Created attachment 544687 [details]
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] 2011-07-08 20:00:05 PDT
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] 2011-07-08 20:03:43 PDT
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] 2011-07-19 06:52:30 PDT
Created attachment 546777 [details] [diff] [review]
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 [:bsmedberg] 2011-07-20 08:56:50 PDT
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 [:rstrong] (use needinfo to contact me) 2011-07-20 12:13:02 PDT
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] 2011-07-20 13:06:25 PDT
Created attachment 547212 [details] [diff] [review]
Patch for Open Containing Folder spawns multiple instances of explorer.exe

Removed redundant calls to ResolveAndStat
Comment 7 Brian R. Bondy [:bbondy] 2011-07-29 10:41:47 PDT
Created attachment 549414 [details] [diff] [review]
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 [:rstrong] (use needinfo to contact me) 2011-08-01 16:56:42 PDT
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 neil@parkwaycc.co.uk 2011-08-02 11:32:10 PDT
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] 2011-08-02 11:52:40 PDT
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] 2011-08-02 14:22:14 PDT
Created attachment 550203 [details] [diff] [review]
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] 2011-08-08 09:04:08 PDT
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] 2011-08-08 11:33:03 PDT
Created attachment 551512 [details] [diff] [review]
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] 2011-08-08 11:34:59 PDT
Created attachment 551513 [details] [diff] [review]
Patch for Open Containing Folder spawns multiple instances of explorer.exe w/ review comments
Comment 15 Benoit Jacob [:bjacob] (mostly away) 2011-09-07 08:12:06 PDT
This was r+'d 2 weeks ago. Are you planning to land it?
Comment 16 Brian R. Bondy [:bbondy] 2011-09-07 08:13:30 PDT
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] 2011-09-07 09:25:34 PDT
Pushed to try: 
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e341b065b122
Comment 18 Brian R. Bondy [:bbondy] 2011-09-08 08:39:43 PDT
Pushed to mozilla-inbound: 
http://hg.mozilla.org/integration/mozilla-inbound/rev/0907fce1e6fc
Comment 20 Jim Jeffery not reading bug-mail 1/2/11 2011-09-09 03:59:45 PDT
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] 2011-09-09 06:28:46 PDT
Will be fixed in 685847
Comment 22 neil@parkwaycc.co.uk 2011-10-04 02:34:17 PDT
*** Bug 638695 has been marked as a duplicate of this bug. ***
Comment 23 neil@parkwaycc.co.uk 2011-10-04 02:41:02 PDT
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] 2011-10-04 04:29:09 PDT
> 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 neil@parkwaycc.co.uk 2011-10-07 01:27:59 PDT
(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] 2012-02-02 00:45:15 PST
(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!)

Note You need to log in before you can comment on or make changes to this bug.