From b2f78b796b5b73d4f0732975ffd66f8aa392c001 Mon Sep 17 00:00:00 2001
From: Gabriel de Dietrich <gabriel.dedietrich@qt.io>
Date: Mon, 5 Dec 2016 14:21:40 -0800
Subject: [PATCH] QCocoaMenu: Avoid exception when inserting item already in
 this menu

This should not happen, but it's clearly not the user's fault.
So we should try to carry on as gracefully as possible instead
of letting Cocoa abort the application.

The patch also factors the repeated calls to QCocoaMenuItem::
nsItem() in QCocoaMenu::insertNative() and improves a warning
from QCocoaMenuIten::sync().

Change-Id: Id00135c219aaf40fb565b19a65cab68f6d9863b2
Task-number: QTBUG-57404
Reviewed-by: Richard Moe Gustavsen <richard.gustavsen@qt.io>
---
 src/plugins/platforms/cocoa/qcocoamenu.mm     | 20 +++++++++-----
 src/plugins/platforms/cocoa/qcocoamenuitem.mm |  2 +-
 .../widgets/widgets/qmenubar/tst_qmenubar.cpp | 27 +++++++++++++++++++
 3 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/src/plugins/platforms/cocoa/qcocoamenu.mm b/src/plugins/platforms/cocoa/qcocoamenu.mm
index 88ffd48538b..915463a52a9 100644
--- a/src/plugins/platforms/cocoa/qcocoamenu.mm
+++ b/src/plugins/platforms/cocoa/qcocoamenu.mm
@@ -331,11 +331,12 @@ void QCocoaMenu::insertMenuItem(QPlatformMenuItem *menuItem, QPlatformMenuItem *
 
 void QCocoaMenu::insertNative(QCocoaMenuItem *item, QCocoaMenuItem *beforeItem)
 {
-    item->nsItem().target = m_nativeMenu.delegate;
-    item->nsItem().action = @selector(itemFired:);
+    NSMenuItem *nativeItem = item->nsItem();
+    nativeItem.target = m_nativeMenu.delegate;
+    nativeItem.action = @selector(itemFired:);
     // Someone's adding new items after aboutToShow() was emitted
-    if (isOpen() && item->menu() && item->nsItem())
-        item->menu()->setAttachedItem(item->nsItem());
+    if (isOpen() && nativeItem)
+        item->menu()->setAttachedItem(nativeItem);
 
     item->setParentEnabled(isEnabled());
 
@@ -348,15 +349,20 @@ void QCocoaMenu::insertNative(QCocoaMenuItem *item, QCocoaMenuItem *beforeItem)
         beforeItem = itemOrNull(m_menuItems.indexOf(beforeItem) + 1);
     }
 
+    if (nativeItem.menu) {
+        qWarning() << "Menu item" << item->text() << "already in menu" << QString::fromNSString(nativeItem.menu.title);
+        return;
+    }
+
     if (beforeItem) {
         if (beforeItem->isMerged()) {
             qWarning("No non-merged before menu item found");
             return;
         }
-        NSUInteger nativeIndex = [m_nativeMenu indexOfItem:beforeItem->nsItem()];
-        [m_nativeMenu insertItem: item->nsItem() atIndex: nativeIndex];
+        const NSInteger nativeIndex = [m_nativeMenu indexOfItem:beforeItem->nsItem()];
+        [m_nativeMenu insertItem:nativeItem atIndex:nativeIndex];
     } else {
-        [m_nativeMenu addItem: item->nsItem()];
+        [m_nativeMenu addItem:nativeItem];
     }
     item->setMenuParent(this);
 }
diff --git a/src/plugins/platforms/cocoa/qcocoamenuitem.mm b/src/plugins/platforms/cocoa/qcocoamenuitem.mm
index e32ff26ff57..21f2b4de851 100644
--- a/src/plugins/platforms/cocoa/qcocoamenuitem.mm
+++ b/src/plugins/platforms/cocoa/qcocoamenuitem.mm
@@ -288,7 +288,7 @@ NSMenuItem *QCocoaMenuItem::sync()
         }
 
         default:
-            qWarning() << "menu item" << m_text << "has unsupported role" << (int)m_role;
+            qWarning() << "Menu item" << m_text << "has unsupported role" << m_role;
         }
 
         if (mergeItem) {
diff --git a/tests/auto/widgets/widgets/qmenubar/tst_qmenubar.cpp b/tests/auto/widgets/widgets/qmenubar/tst_qmenubar.cpp
index 3a4c4545df0..2c20d03066f 100644
--- a/tests/auto/widgets/widgets/qmenubar/tst_qmenubar.cpp
+++ b/tests/auto/widgets/widgets/qmenubar/tst_qmenubar.cpp
@@ -132,6 +132,7 @@ private slots:
     void taskQTBUG53205_crashReparentNested();
 #ifdef Q_OS_MACOS
     void taskQTBUG56275_reinsertMenuInParentlessQMenuBar();
+    void QTBUG_57404_existingMenuItemException();
 #endif
 
     void platformMenu();
@@ -1539,6 +1540,32 @@ void tst_QMenuBar::taskQTBUG56275_reinsertMenuInParentlessQMenuBar()
 
     QVERIFY(tst_qmenubar_taskQTBUG56275(&menubar));
 }
+
+void tst_QMenuBar::QTBUG_57404_existingMenuItemException()
+{
+    QMainWindow mw1;
+    QMainWindow mw2;
+    mw1.show();
+    mw2.show();
+
+    QMenuBar *mb = new QMenuBar(&mw1);
+    mw1.setMenuBar(mb);
+    mb->show();
+    QMenu *editMenu = new QMenu(QLatin1String("Edit"), &mw1);
+    mb->addMenu(editMenu);
+    QAction *copyAction = editMenu->addAction("&Copy");
+    copyAction->setShortcut(QKeySequence("Ctrl+C"));
+    QTest::ignoreMessage(QtWarningMsg, "Menu item \"&Copy\" has unsupported role QPlatformMenuItem::MenuRole(NoRole)");
+    copyAction->setMenuRole(QAction::NoRole);
+
+    QVERIFY(QTest::qWaitForWindowExposed(&mw2));
+    QTest::qWait(100);
+    QTest::ignoreMessage(QtWarningMsg, "Menu item \"&Copy\" already in menu \"Edit\"");
+    mw2.close();
+    mw1.activateWindow();
+    QTest::qWait(100);
+    // No crash, all fine.
+}
 #endif // Q_OS_MACOS
 
 QTEST_MAIN(tst_QMenuBar)