Merge lp:~nick-dedekind/qmenumodel/lp1239394 into lp:~phablet-team/qmenumodel/trunk

[Nick Dedekind]

Added pointer checks for items/iterators. (lp#1239394)

Added pointer checks for items/iterators. (lp#1239394)

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:95
http://jenkins.qa.ubuntu.com/job/qmenumodel-ci/37/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qmenumodel-saucy-amd64-ci/34
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qmenumodel-saucy-armhf-ci/34
        deb: http://jenkins.qa.ubuntu.com/job/qmenumodel-saucy-armhf-ci/34/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/qmenumodel-ci/37/rebuild

review: Approve (continuous-integration)

[Lars Karlitski (larsu)]

g_sequence_get() never returns NULL, those checks are unnecessary.

g_sequence_get_iter_at_pos() always returns a valid iter, so e don't need the check for NULL.

Having the is_end_iter() check makes sense to me, but we should look for the actual cause of this crash: something is trying to access an invalid row name. Maybe we should even warn when that happens instead of silently failing?

review: Needs Fixing

[Nick Dedekind (nick-dedekind)]

> g_sequence_get() never returns NULL, those checks are unnecessary.
>
> g_sequence_get_iter_at_pos() always returns a valid iter, so e don't need the
> check for NULL.
>
> Having the is_end_iter() check makes sense to me, but we should look for the
> actual cause of this crash: something is trying to access an invalid row name.
> Maybe we should even warn when that happens instead of silently failing?

I think it's a qml object (a menu item from unitymenumodel) which is updated as it's being deleted (removed from model), not sure we can get around it.

[lp:~nick-dedekind/qmenumodel/lp1239394]

96. By Nick Dedekind on 2013-10-14

removed unnecessary pointer checks

[Nick Dedekind (nick-dedekind)]

> g_sequence_get() never returns NULL, those checks are unnecessary.
>
> g_sequence_get_iter_at_pos() always returns a valid iter, so e don't need the
> check for NULL.
>
> Having the is_end_iter() check makes sense to me, but we should look for the
> actual cause of this crash: something is trying to access an invalid row name.
> Maybe we should even warn when that happens instead of silently failing?

Removed checks for null pointer on g_sequence_get_iter_at_pos

[PS Jenkins bot (ps-jenkins)]

PASSED: Continuous integration, rev:96
http://jenkins.qa.ubuntu.com/job/qmenumodel-ci/38/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qmenumodel-saucy-amd64-ci/35
    SUCCESS: http://jenkins.qa.ubuntu.com/job/qmenumodel-saucy-armhf-ci/35
        deb: http://jenkins.qa.ubuntu.com/job/qmenumodel-saucy-armhf-ci/35/artifact/work/output/*zip*/output.zip

Click here to trigger a rebuild:
http://10.97.0.26:8080/job/qmenumodel-ci/38/rebuild

review: Approve (continuous-integration)

[Lars Karlitski (larsu)]

Cool, thanks for the patch.

review: Approve

=== modified file 'libqmenumodel/src/unitymenumodel.cpp'
--- libqmenumodel/src/unitymenumodel.cpp	2013-10-11 21:00:09 +0000
+++ libqmenumodel/src/unitymenumodel.cpp	2013-10-14 16:31:42 +0000
@@ -395,9 +395,18 @@
 
 QVariant UnityMenuModel::data(const QModelIndex &index, int role) const
 {
+    GSequenceIter *it;
     GtkMenuTrackerItem *item;
 
-    item = (GtkMenuTrackerItem *) g_sequence_get (g_sequence_get_iter_at_pos (priv->items, index.row()));
+    it = g_sequence_get_iter_at_pos (priv->items, index.row());
+    if (g_sequence_iter_is_end (it)) {
+        return QVariant();
+    }
+
+    item = (GtkMenuTrackerItem *) g_sequence_get (it);
+    if (!item) {
+        return QVariant();
+    }
 
     switch (role) {
         case LabelRole:
@@ -497,12 +506,14 @@
     UnityMenuModel *model;
 
     it = g_sequence_get_iter_at_pos (priv->items, position);
-    if (g_sequence_iter_is_end (it))
+    if (g_sequence_iter_is_end (it)) {
         return NULL;
+    }
 
     item = (GtkMenuTrackerItem *) g_sequence_get (it);
-    if (!gtk_menu_tracker_item_get_has_submenu (item))
+    if (!item || !gtk_menu_tracker_item_get_has_submenu (item)) {
         return NULL;
+    }
 
     model = (UnityMenuModel *) g_object_get_qdata (G_OBJECT (item), unity_submenu_model_quark ());
     if (model == NULL) {
@@ -603,10 +614,19 @@
 
 bool UnityMenuModel::loadExtendedAttributes(int position, const QVariantMap &schema)
 {
+    GSequenceIter *it;
     GtkMenuTrackerItem *item;
     QVariantMap *extendedAttrs;
 
-    item = (GtkMenuTrackerItem *) g_sequence_get (g_sequence_get_iter_at_pos (priv->items, position));
+    it = g_sequence_get_iter_at_pos (priv->items, position);
+    if (g_sequence_iter_is_end (it)) {
+        return false;
+    }
+
+    item = (GtkMenuTrackerItem *) g_sequence_get (it);
+    if (!item) {
+        return false;
+    }
 
     extendedAttrs = new QVariantMap;
 
@@ -647,9 +667,18 @@
 
 void UnityMenuModel::activate(int index, const QVariant& parameter)
 {
+    GSequenceIter *it;
     GtkMenuTrackerItem *item;
 
-    item = (GtkMenuTrackerItem *) g_sequence_get (g_sequence_get_iter_at_pos (priv->items, index));
+    it = g_sequence_get_iter_at_pos (priv->items, index);
+    if (g_sequence_iter_is_end (it)) {
+        return;
+    }
+
+    item = (GtkMenuTrackerItem *) g_sequence_get (it);
+    if (!item) {
+        return;
+    }
 
     if (parameter.isValid()) {
         gchar *action;
@@ -665,12 +694,20 @@
 
 void UnityMenuModel::changeState(int index, const QVariant& parameter)
 {
+    GSequenceIter *it;
     GtkMenuTrackerItem* item;
     GVariant* data;
     GVariant* current_state;
 
-    item = (GtkMenuTrackerItem *) g_sequence_get (g_sequence_get_iter_at_pos (priv->items, index));
-    if (!item) return;
+    it = g_sequence_get_iter_at_pos (priv->items, index);
+    if (g_sequence_iter_is_end (it)) {
+        return;
+    }
+
+    item = (GtkMenuTrackerItem *) g_sequence_get (it);
+    if (!item) {
+        return;
+    }
 
     current_state = gtk_menu_tracker_item_get_action_state (item);
     if (current_state) {
@@ -712,22 +749,21 @@
 
         GSequenceIter *it;
         it = g_sequence_get_iter_at_pos (priv->items, ummrce->position);
-        if (it) {
-            beginInsertRows(QModelIndex(), ummrce->position, ummrce->position);
-
-            it = g_sequence_insert_before (it, g_object_ref (ummrce->item));
-            g_object_set_qdata (G_OBJECT (ummrce->item), unity_menu_model_quark (), this);
-            g_signal_connect (ummrce->item, "notify", G_CALLBACK (UnityMenuModelPrivate::menuItemChanged), it);
-
-            endInsertRows();
-        }
+
+        beginInsertRows(QModelIndex(), ummrce->position, ummrce->position);
+
+        it = g_sequence_insert_before (it, g_object_ref (ummrce->item));
+        g_object_set_qdata (G_OBJECT (ummrce->item), unity_menu_model_quark (), this);
+        g_signal_connect (ummrce->item, "notify", G_CALLBACK (UnityMenuModelPrivate::menuItemChanged), it);
+
+        endInsertRows();
         return true;
     } else if (e->type() == UnityMenuModelRemoveRowEvent::eventType) {
         UnityMenuModelRemoveRowEvent *ummrre = static_cast<UnityMenuModelRemoveRowEvent*>(e);
 
         GSequenceIter *it;
         it = g_sequence_get_iter_at_pos (priv->items, ummrre->position);
-        if (it) {
+        if (!g_sequence_iter_is_end (it)) {
             beginRemoveRows(QModelIndex(), ummrre->position, ummrre->position);
 
             g_sequence_remove (it);
@@ -799,6 +835,10 @@
         const gchar *action_namespace;
 
         item = (GtkMenuTrackerItem *) g_sequence_get (iter);
+        if (!item) {
+            return g_strdup (name);
+        }
+
         action_namespace = gtk_menu_tracker_item_get_action_namespace (item);
         if (action_namespace != NULL)
           return g_strjoin (".", action_namespace, name, NULL);