Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Right clicking an empty area causes crash #35

Closed
GeorgeHahn opened this issue Jun 1, 2015 · 14 comments
Closed

Right clicking an empty area causes crash #35

GeorgeHahn opened this issue Jun 1, 2015 · 14 comments

Comments

@GeorgeHahn
Copy link

@GeorgeHahn GeorgeHahn commented Jun 1, 2015

Reproduction:

  • Open any directory and any view style
  • Right click an empty area of the view

Additional details:
Windows 10 x64, build 10130

@GeorgeHahn
Copy link
Author

@GeorgeHahn GeorgeHahn commented Jun 1, 2015

After some investigation, I concluded that this is caused by the OneDrive filesyncshell.dll.

I was able to overcome it by turning on SEH exception handling in the Helper project and wrapping the QueryContextMenu call in the AddMenuEntries function of ContextMenuManager.cpp:

HRESULT hr;
try {
    hr = itr->pContextMenuActual->QueryContextMenu(
        hMenu, iStartPos, iMinID + iOffset, iMaxID,
        CMF_NORMAL | CMF_EXPLORE);
}
catch (...) {
    // Swallow context menu error
    continue;
}

@jtbrown3
Copy link

@jtbrown3 jtbrown3 commented Aug 5, 2015

Verified that this critical crash behavior is still present in the official Windows 10 x64 release (build 10240).
Thx George for finding the workaroud. Would be great to get this committed into a fresh build. Otherwise it's a 'No-Go' for Win 10...

@macchky
Copy link

@macchky macchky commented Nov 25, 2015

Temp fix is Launch as Administrator

@linquize
Copy link
Contributor

@linquize linquize commented Nov 25, 2015

Do Windows Explorer has such empty space to test with?

@derceg derceg added the bug label Dec 26, 2015
@derceg
Copy link
Owner

@derceg derceg commented Dec 26, 2015

It would be good to find out exactly what's going wrong. It's quite possible there's a bug within the context menu code, particularly if the menu item appears correctly in other file managers (I haven't tested this myself, but I would assume it does - would anybody be able to confirm?).

It's interesting that it works when run as an administrator, I'm not sure why that would be the case, and obviously we don't have access to the OneDrive source code.

@GeorgeHahn
My concern with swallowing the exception is that it might result in an inconsistent state, which could lead to further crashes or corruption.

If anybody wants to debug through the code on Windows 10 and do some experimentation, that would be of great help. I'm happy to assist.

@juanitogan
Copy link

@juanitogan juanitogan commented Jan 16, 2016

I ended up here because the new File Explorer in Win 10 is killing me. I get lost too easily scrolling the tree with its tiny indentations. And I work a lot in the tree. This right-click thing is almost a show stopper. For now I have Admin permanently set in the Compatibility tab for the file. I worry about the others frustrated with the new File Explorer and less savvy to figure out the workaround. Anyhow, thank you for what looks to be a quality solution to my File Explorer pains.

@derceg
Copy link
Owner

@derceg derceg commented Feb 14, 2016

From some testing I've been doing, it looks like the OneDrive extension causes an access violation. It could be because of a bug in Explorer++, or a bug in OneDrive. Since the problem occurs inside the QueryContextMenu call, it would be difficult to figure out which is at fault.

I think the best way of handling this would be to blacklist the extension using its CLSID (which will at least stop the crashes). Then, if the issue is resolved in the future, the extension can be loaded again.

dilosec added a commit to dilosec/explorerplusplus that referenced this issue Mar 8, 2016
The crash occurs in the QueryContextMenu call in OneDrive's
FileSyncShell[64].dll - this fix is based on code proposed by
@GeorgeHahn in issue derceg#35.
dilosec added a commit to dilosec/explorerplusplus that referenced this issue Mar 8, 2016
The crash occurs in the QueryContextMenu call in OneDrive's
FileSyncShell[64].dll - this fix is based on code proposed by
@GeorgeHahn in issue derceg#35.
@Boiethios
Copy link

@Boiethios Boiethios commented Oct 13, 2016

Hello, the Explorer++ still crashes on Windows 8.x

When I right-click somewhere out of a file, it immediately crashes.

@endo64
Copy link

@endo64 endo64 commented Oct 25, 2016

I confirm @Boiethios, Explorer++ still crashes on Win 8.1 x64.
Is this fix included in the latest compiled version on web site?

@paulbrennanOJT
Copy link

@paulbrennanOJT paulbrennanOJT commented Mar 16, 2017

Confirmed that uninstalling OneDrive (if it is an option for you) does resolve the issue :)

@kenstuddy
Copy link

@kenstuddy kenstuddy commented Jun 11, 2017

Even though I already had OneDrive disabled via GPO, I still had this issue. I resolved this issue by renaming C:\Users\user\AppData\Local\Microsoft\OneDrive to C:\Users\user\AppData\Local\Microsoft\OneDrivebak.

@derceg derceg closed this in 240d770 Jun 11, 2017
@endo64
Copy link

@endo64 endo64 commented Jun 17, 2017

@kenstuddy Disabling doesn't solve the crash issue, I completely uninstall OneDrive then issue solved. When you rename that folder your OneDrive installation still works normally?

@kenstuddy
Copy link

@kenstuddy kenstuddy commented Jun 17, 2017

I disabled OneDrive in GPO and from starting up, however the crash still happened until I renamed the OneDrive folder. However, as of June 11, this bug has been fixed.

@demahum
Copy link

@demahum demahum commented Jan 13, 2018

It would be good to solve this issue completely if possible (i.e. to be able to have both OneDrive and Explorer++ running).

derceg added a commit that referenced this issue Jan 27, 2021
Previously, the background context menu was always being built by
manually instantiating the necessary shell extensions. However, this has
several drawbacks:

- It's incomplete. Not all items that are shown in the background
  context menu in Windows Explorer appear (see #210, #76).
- It's error prone. There have been issues reported with this
  functionality in the past (e.g. #35, #119).
- It's low level. The documentation for IShellExtInit
  (https://docs.microsoft.com/en-us/windows/win32/api/shobjidl_core/nn-shobjidl_core-ishellextinit),
  for example, states "You do not use this interface directly. The Shell
  calls it to initialize the handler.". It shouldn't be necessary to
  directly iterate over registry keys, either.
- The functionality provided by ContextMenuManager is similar to that
  provided by FileContextMenuManager and some of the code overlaps
  directly.

Because of these issues, the context menu is now loaded using
IShellView::GetItemObject() along with the SVGIO_BACKGROUND flag. This
has the major advantage that the menu is exactly the same as that shown
in Explorer.

However, there are some downsides to this approach:

- The menu that's returned contains menu items that are specific to
  Explorer. For example, "View", "Sort by", "Group by", etc. This is a
  problem, as selecting one of those menu items won't have any effect in
  Explorer++.
  Therefore, Explorer specific items are either replaced altogether, or
  handled internally by Explorer++ (e.g. the "Refresh" item is handled
  that way).
- Following on from the previous point, the way in which menu items are
  replaced is by their language-independent verb.
  However, verbs are only set for most of the system items in the menu
  from Windows 8 onwards. In Windows 7, most of the system menu items
  have no verbs set, meaning there's no way to reliably identify them.
  The menu item text can't be used, as it will vary with the language
  set.
  Therefore, the menu will only be retrieved in the way described above
  from Windows 8 onwards. In Windows 7, the menu will still be generated
  in the original way.
- While the context menu is exactly the same as that shown in Explorer,
  there is at least one case where there are minor differences.
  In library directories, there's an "Arrange by" menu item that's shown
  first. This menu item doesn't have a verb set, making it hard to
  reliably remove. This menu also fails to be populated.
  Additionally, the "Group by" menu doesn't appear in library
  directories, though it is present in the menu shown in Explorer.
  Aside from that, the menus appear to be the same.
- While almost all menu items appear to work by default (when passed
  through to IContextMenu::InvokeCommand()), there are a few that don't
  work.
  One is the "Customize this folder..." item, which is handled
  internally by Explorer++.
  The undo/redo items also don't work and are removed.
  Aside from that, all other menu items appear to work, though it's hard
  to test exhaustively and it's possible there may be another menu item
  in a particular folder that doesn't work.

Finally, note that the context menu that's shown doesn't contain the
"Bookmark Folder..." and "Copy Folder Path" items that are shown on the
custom background context menu. This is because the background context
menu can be quite long and additional items are more likely to be
overlooked. Both of those items are still accessible from other places
(e.g. the main menu, main toolbar).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.