unity-scopes-shell

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

  1. lp1535377
  2. Merge into trunk
Proposed by Rex Tsai
Status: Rejected
Rejected by: Paweł Stołowski
Proposed branch: lp://qastaging/~chihchun/unity-scopes-shell/lp1535377
Merge into: lp://qastaging/unity-scopes-shell
Diff against target: 15 lines (+3/-2)
1 file modified
src/Unity/resultsmodel.cpp (+3/-2)
To merge this branch: bzr merge lp://qastaging/~chihchun/unity-scopes-shell/lp1535377
Reviewer Review Type Date Requested Status
Paweł Stołowski (community) Disapprove
PS Jenkins bot (community) continuous-integration Needs Fixing
Review via email: mp+285123@code.qastaging.launchpad.net

Commit message

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.

Description of the change

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

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

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)
Revision history for this message
Paweł Stołowski (stolowski) wrote :

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
Revision history for this message
Paweł Stołowski (stolowski) wrote :

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

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.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches

to all changes: