Open Containing Folder spawns multiple instances of explorer.exe

RESOLVED FIXED in mozilla9

Status

()

Core
XPCOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: John, Assigned: bbondy)

Tracking

5 Branch
mozilla9
x86_64
Windows 7
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

6 years ago
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.
(Reporter)

Updated

6 years ago
OS: Other → Windows 7
Hardware: All → x86_64
Component: General → Download Manager
Product: Firefox → Toolkit
QA Contact: general → download.manager
(Assignee)

Comment 1

6 years ago
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)

Updated

6 years ago
Assignee: nobody → netzen
(Assignee)

Comment 2

6 years ago
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.
(Assignee)

Comment 3

6 years ago
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.
Attachment #546777 - Flags: review?(benjamin)
(Assignee)

Updated

6 years ago
Assignee: netzen → nobody
Component: Download Manager → XPCOM
Product: Toolkit → Core
QA Contact: download.manager → xpcom
(Assignee)

Updated

6 years ago
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.
(Assignee)

Comment 6

6 years ago
Created attachment 547212 [details] [diff] [review]
Patch for Open Containing Folder spawns multiple instances of explorer.exe

Removed redundant calls to ResolveAndStat
Attachment #546777 - Attachment is obsolete: true
Attachment #547212 - Flags: review?(robert.bugzilla)
Attachment #546777 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 7

6 years ago
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.
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)

Updated

6 years ago
Blocks: 156422

Comment 9

6 years ago
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

6 years ago
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-
Attachment #549414 - Flags: review?(robert.bugzilla)
(Assignee)

Comment 11

6 years ago
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.
Attachment #549414 - Attachment is obsolete: true
Attachment #550203 - Flags: review?(jmathies)

Comment 12

6 years ago
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+
(Assignee)

Comment 13

6 years ago
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.
Attachment #550203 - Attachment is obsolete: true
Attachment #551512 - Flags: review?(jmathies)
(Assignee)

Comment 14

6 years ago
Created attachment 551513 [details] [diff] [review]
Patch for Open Containing Folder spawns multiple instances of explorer.exe w/ review comments
Attachment #551512 - Attachment is obsolete: true
Attachment #551513 - Flags: review?(jmathies)
Attachment #551512 - Flags: review?(jmathies)

Updated

6 years ago
Attachment #551513 - Flags: review?(jmathies) → review+
This was r+'d 2 weeks ago. Are you planning to land it?
(Assignee)

Comment 16

6 years ago
bjacob, I'l run it through try today and if all goes well I'll push it by tomorrow.
(Assignee)

Comment 17

6 years ago
Pushed to try: 
https://tbpl.mozilla.org/?tree=Try&usebuildbot=1&rev=e341b065b122
(Assignee)

Comment 18

6 years ago
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
Last Resolved: 6 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.
Depends on: 685847
(Assignee)

Comment 21

6 years ago
Will be fixed in 685847

Updated

6 years ago
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.
(Assignee)

Comment 24

6 years ago
> 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.
Blocks: 716449
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.