Unity

Merge lp://qastaging/~3v1n0/unity/fix-load-icon-crash-926658 into lp://qastaging/unity

  1. fix-load-icon-crash-926658
  2. Merge into trunk
Proposed by Marco Trevisan (Treviño)
Status: Rejected
Rejected by: Marco Trevisan (Treviño)
Proposed branch: lp://qastaging/~3v1n0/unity/fix-load-icon-crash-926658
Merge into: lp://qastaging/unity
Prerequisite: lp://qastaging/~3v1n0/unity/barrier-timeout
Diff against target: 641 lines (+150/-144)
7 files modified
UnityCore/ModelRowAdaptor.h (+1/-1)
dash/LensView.cpp (+40/-5)
dash/LensView.h (+4/-0)
dash/ResultRendererTile.cpp (+80/-98)
dash/ResultRendererTile.h (+1/-2)
dash/ResultView.cpp (+21/-32)
dash/ResultView.h (+3/-6)
To merge this branch: bzr merge lp://qastaging/~3v1n0/unity/fix-load-icon-crash-926658
Reviewer Review Type Date Requested Status
Michal Hruby (community) Needs Fixing
Francis Ginther Abstain
jenkins (community) continuous-integration Needs Fixing
Review via email: mp+116091@code.qastaging.launchpad.net

This proposal supersedes a proposal from 2012-07-20.

Commit message

ResultRendererTile: don't crash in LoadIcon if row.renderer<TextureContainer*>() is null

Description of the change

In ResultRendererTile::LoadIcon we assumed that row.renderer<TextureContainer*>() is always not null; even if this should not happen because it has already just set into ResultRendererTile::Preload it looks like that there are cases that when doing row.set_renderer(new TextureContainer()); in the subsequent row.renderer<TextureContainer*>() returns null.
The implementation of that depends on dee_model_{set,get}_tag (it's basically like doing dee_model_set_tag and dee_model_get_tag), so it seems like that is failing. I've not been able to get a failure in stress tests, btw.

So, at this point, even if it would not the best solution, it's better to add a safety check to prevent crashes.

To post a comment you must log in.
Revision history for this message
Michal Hruby (mhr3) wrote :

Sorry, but I don't like this one bit, dee_model_get_tag can crash if the passed model is not valid (already unreferenced). IMO guarding against null is not going to help, you'll just push the crash to a different place (in the better case, in worse you'll mask the underlying problem completely and there'll be even more random crashes).

review: Needs Fixing
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

Yes, that's true... But according to the stacktraces I got (attached to the bug), the crash is not happening because dee_model_get_tag returns an unreferenced pointer, but because it's returning a null one...

I'm wondering if that could happen that could be happen this:

ResultRendererTile::Preload()
ResultRendererTile::Unload()
ResultRendererTile::LoadIcon()

But it would be weird as all seems done in the same thread...

Revision history for this message
jenkins (martin-mrazik+qa) wrote :

PASSED: Continuous integration, rev:2540
http://jenkins.qa.ubuntu.com/job/unity-ci/108/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=amd64/108/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=i386/108/console

review: Approve (continuous-integration)
Revision history for this message
Michal Hruby (mhr3) wrote :

As you alone said in the comment, the renderer is set in Preload(), so the only explanation is that the model is already invalid at that point, but dee notices that and ignores the call (while logging a critical), doing dee_model_get_tag([invalid], tag) will then return NULL of course.

Other times dee doesn't notice that the model is invalid, and crashes inside itself.

Revision history for this message
jenkins (martin-mrazik+qa) wrote :

PASSED: Continuous integration, rev:2545
http://jenkins.qa.ubuntu.com/job/unity-ci/110/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=amd64/110/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=i386/110/console

review: Approve (continuous-integration)
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

Well, it's the same I thought, but I'm not getting that on the stacktrace...
However, protecting us from possible invalid pointers is something we should do in any case.

Revision history for this message
jenkins (martin-mrazik+qa) wrote :

PASSED: Continuous integration, rev:2547
http://jenkins.qa.ubuntu.com/job/unity-ci/113/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=amd64/113/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=i386/113/console

review: Approve (continuous-integration)
Revision history for this message
Marco Trevisan (Treviño) (3v1n0) wrote :

I've reffed the model in ModelRowAdaptor... As you said it's not the full solution, but it's something we can safely do anyway. Let me know if this is better for you.

Revision history for this message
jenkins (martin-mrazik+qa) wrote :

PASSED: Continuous integration, rev:2551
http://jenkins.qa.ubuntu.com/job/unity-ci/115/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=amd64/115/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=i386/115/console

review: Approve (continuous-integration)
Revision history for this message
jenkins (martin-mrazik+qa) wrote :

PASSED: Continuous integration, rev:2552
http://jenkins.qa.ubuntu.com/job/unity-ci/117/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=amd64/117/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=i386/117/console

review: Approve (continuous-integration)
Revision history for this message
Michal Hruby (mhr3) wrote :

> I've reffed the model in ModelRowAdaptor... As you said it's not the full
> solution, but it's something we can safely do anyway. Let me know if this is
> better for you.

As mentioned on IRC, I'd rather see a *single* weak ref on the model in LensView? than this. (since adding a reference for each model row isn't a good idea)

review: Needs Fixing
Revision history for this message
jenkins (martin-mrazik+qa) wrote :

FAILED: Continuous integration, rev:2556
http://jenkins.qa.ubuntu.com/job/unity-ci/130/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=amd64/130/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/unity-ci/./distribution=quantal,flavor=i386/130/console

review: Needs Fixing (continuous-integration)
Revision history for this message
Francis Ginther (fginther) wrote :

Review was claimed by accident, please ignore.

review: Abstain
Revision history for this message
Michal Hruby (mhr3) wrote :

Discussed on IRC, but so make sure it doesn't get lost:

1) the model can change, you need to listen to changed signal
2) in the weak notify callback you're basically iterating over the model, which you can't since the model is already dead

review: Needs Fixing

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.