From e4433ad0fa8377e2f5e0e422a0e464687720c94f Mon Sep 17 00:00:00 2001
From: David Murphy <murphman@gmail.com>
Date: Sun, 8 Jul 2018 00:45:07 -0400
Subject: [PATCH] BMenu: Fix crash and keyboard navigation on 'recent items'
 menus

* Prevents crash mentioned in Trac, but also enables keyboard navigation
  to 'recent items' menus such as "Open files..." in MediaPlayer and DiskProbe
* Check selected menu and submenu exist in menu tracking thread before accessing
* Update BMenu::AttachedToWindow to pass in keydown param to _AddDynamicItems

Fixes #9251
Change-Id: I3031b8e9c1b9dd4ef1187c5a6b8ab7925e3496d2
---
 src/kits/interface/Menu.cpp | 65 +++++++++++++++++++++++--------------
 1 file changed, 41 insertions(+), 24 deletions(-)

diff --git a/src/kits/interface/Menu.cpp b/src/kits/interface/Menu.cpp
index 98b1054927..2ddcf51795 100644
--- a/src/kits/interface/Menu.cpp
+++ b/src/kits/interface/Menu.cpp
@@ -388,7 +388,14 @@ BMenu::AttachedToWindow()
 	_GetOptionKey(sOptionKey);
 	_GetMenuKey(sMenuKey);
 
-	fAttachAborted = _AddDynamicItems();
+	// The menu should be added to the menu hierarchy and made visible if:
+	// * the mouse is over the menu,
+	// * the user has requested the menu via the keyboard.
+	// So if we don't pass keydown in here, keyboard navigation breaks since
+	// fAttachAborted will return false if the mouse isn't over the menu
+	bool keyDown = Supermenu() != NULL
+		? Supermenu()->fState == MENU_STATE_KEY_TO_SUBMENU : false;
+	fAttachAborted = _AddDynamicItems(keyDown);
 
 	if (!fAttachAborted) {
 		_CacheFontInfo();
@@ -1661,31 +1668,41 @@ BMenu::_Track(int* action, long start)
 			// that our window gets any update message to
 			// redraw itself
 			UnlockLooper();
-			int submenuAction = MENU_STATE_TRACKING;
-			BMenu* submenu = fSelected->Submenu();
-			submenu->_SetStickyMode(_IsStickyMode());
 
-			// The following call blocks until the submenu
-			// gives control back to us, either because the mouse
-			// pointer goes out of the submenu's bounds, or because
-			// the user closes the menu
-			BMenuItem* submenuItem = submenu->_Track(&submenuAction);
-			if (submenuAction == MENU_STATE_CLOSED) {
-				item = submenuItem;
-				fState = MENU_STATE_CLOSED;
-			} else if (submenuAction == MENU_STATE_KEY_LEAVE_SUBMENU) {
-				if (LockLooper()) {
-					BMenuItem* temp = fSelected;
-					// close the submenu:
-					_SelectItem(NULL);
-					// but reselect the item itself for user:
-					_SelectItem(temp, false);
-					UnlockLooper();
+			// To prevent NULL access violation, ensure a menu has actually
+			// been selected and that it has a submenu. Because keyboard and
+			// mouse interactions set selected items differently, the menu
+			// tracking thread needs to be careful in triggering the navigation
+			// to the submenu.
+			if (fSelected != NULL) {
+				BMenu* submenu = fSelected->Submenu();
+				int submenuAction = MENU_STATE_TRACKING;
+				if (submenu != NULL) {
+					submenu->_SetStickyMode(_IsStickyMode());
+
+					// The following call blocks until the submenu
+					// gives control back to us, either because the mouse
+					// pointer goes out of the submenu's bounds, or because
+					// the user closes the menu
+					BMenuItem* submenuItem = submenu->_Track(&submenuAction);
+					if (submenuAction == MENU_STATE_CLOSED) {
+						item = submenuItem;
+						fState = MENU_STATE_CLOSED;
+					} else if (submenuAction == MENU_STATE_KEY_LEAVE_SUBMENU) {
+						if (LockLooper()) {
+							BMenuItem* temp = fSelected;
+							// close the submenu:
+							_SelectItem(NULL);
+							// but reselect the item itself for user:
+							_SelectItem(temp, false);
+							UnlockLooper();
+						}
+						// cancel  key-nav state
+						fState = MENU_STATE_TRACKING;
+					} else
+						fState = MENU_STATE_TRACKING;
 				}
-				// cancel  key-nav state
-				fState = MENU_STATE_TRACKING;
-			} else
-				fState = MENU_STATE_TRACKING;
+			}
 			if (!LockLooper())
 				break;
 		} else if ((item = _HitTestItems(location, B_ORIGIN)) != NULL) {