Merge lp:~chihchun/unity-scopes-shell/lp1535377 into lp:unity-scopes-shell

[Rex Tsai]

Fixed addUpdateResults crashes (LP: #1535377, #1540704)

The new result could be larger then current result list, moving the results to a bigger then qlist size will result null elements, which cause oldResultsMap.rebuild failure.

Fixed addUpdateResults crashes (LP: #1535377, #1540704)

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:282
http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-ci/408/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-amd64-ci/102
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-armhf-ci/102
        deb: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-armhf-ci/102/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-vivid-i386-ci/102
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-amd64-ci/65/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-armhf-ci/65/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-scopes-shell-wily-i386-ci/65/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/unity-scopes-shell-ci/408/rebuild

review: Needs Fixing (continuous-integration)

[Paweł Stołowski (stolowski)]

Thanks for looking into this. As discussed on IRC though, I think that while your fix likely avoids the crash, it probably hides the real problem at the same time, plus I think that getting rid of "row + (row > oldPos ? 1 : 0)" in the beginMoveRows notification for the qml view will cause subtle issues.

It's expected and absolutely fine that the size of new results list is different than current list; as the new results list is iterated in the 0..results.count() loop, new (previously unseen) results are inserted into m_lists (so it grows) and it's guaranteed that any moves happen only in the 0..row range, which is valid for m_results list. That's the theory at least, I cannot exclude a bug in this implementation.

I'm currently working on a branch which has a bunch of stress tests for model updates (it wasn't part of the existing code, because I started working on them just recently for some other changes in that area) and am trying to come up with a case that would fail with existing implementation, giving me the opportunity to better understand what happens.

review: Abstain

[Paweł Stołowski (stolowski)]

Rejecting as I think the other MP is the proper fix. My MP is currently in silo 80 and confirmed by Rex to fix the issue.

review: Disapprove

Unmerged revisions

282. By Rex Tsai on 2016-02-04

Fixed addUpdateResults crashes (LP: #1535377, #1540704)

The new result could be larger then current result list,
moving the results to a bigger then qlist size will result
null elements, which cause oldResultsMap.rebuild failure.

=== modified file 'src/Unity/resultsmodel.cpp'
--- src/Unity/resultsmodel.cpp	2015-11-30 09:23:32 +0000
+++ src/Unity/resultsmodel.cpp	2016-02-04 21:38:34 +0000
@@ -110,8 +110,9 @@
         if (hadBefore) {
             if (row != oldPos) {
                 // move row
-                beginMoveRows(QModelIndex(), oldPos, oldPos, QModelIndex(), row + (row > oldPos ? 1 : 0));
-                m_results.move(oldPos, row);
+                int pos = (row >= m_results.count()) ? (m_results.count() - 1) : row;
+                beginMoveRows(QModelIndex(), oldPos, oldPos, QModelIndex(), pos);
+                m_results.move(oldPos, pos);
                 oldResultsMap.rebuild(m_results);
                 endMoveRows();
             }