resume journal entry race may duplicate resumed activity id

[quozl]

A race condition exists during the resume of a journal entry, which may result in two activity launches with the same activity id.

If two button release events are received on the favorites view for an activity icon that has journal entries, and resume mode is true, then two attempts will be made to resume the journal entry.

This is because the shell model list of activities is updated by the D-Bus NotifyLaunch signal, which will not have been processed by the time the second button release event is processed.

This ticket based on static analysis of the code in HEAD of sugar and sugar-toolkit repositories.

See also analysis of a 0.84 reproducible symptom on  dev.laptop.org #10016

[alsroot]

Proper fix was attached to #1814 since patch heavily depends of #1814. 0.84 fix will be posted to downstream tracker.

[alsroot]

looks like patch could be less invasive

[erikos]

After discussing with Aleksey on #sugar, we move it out to a bug fix release as it is not a show stopper: already hard to reproduce on slow machines.

[quozl]

Replying to erikos:

After discussing with Aleksey on #sugar, we move it out to a bug fix release as it is not a show stopper: already hard to reproduce on slow machines.

I'm surprised at this justification. It is trivial to reproduce on an XO-1.5, especially with #1656 not fixed. I admit, an XO-1.5 is a slow machine, but I thought Sugar was intended to operate on slow machines. Is 0.88 to require a non-slow machine?

The impact of the bug is low, and that is a sufficient reason to delay it. I don't understand why severity was raised to Major.

[alsroot]

Replying to quozl:

Replying to erikos:

After discussing with Aleksey on #sugar, we move it out to a bug fix release as it is not a show stopper: already hard to reproduce on slow machines.

I'm surprised at this justification. It is trivial to reproduce on an XO-1.5, especially with #1656 not fixed. I admit, an XO-1.5 is a slow machine, but I thought Sugar was intended to operate on slow machines. Is 0.88 to require a non-slow machine?

The impact of the bug is low, and that is a sufficient reason to delay it. I don't understand why severity was raised to Major.

Well, 0.88 is not last sugar release :), there will be at least 0.88.x. And also in case of XO, this patch could be backported to 0.84 despite of 0.80 acceptance.

[erikos]

It is just not a show stopper and is a candidate for 0.88.x, the 0.88 bugfix release at the end of April (  http://wiki.sugarlabs.org/go/0.88/Roadmap#Schedule ). As we are in hard code freeze we have to be a bit more aggressive with rejecting code changes. I hope this clarifies the reasoning a bit.

[tomeu]

The launching property is used in other places, we shouldn't assume that LAUNCHED == 0.

[tomeu]

This patch is very different from what I reviewed previously, would be helpful if you could write a short comment explaining why.

        if home_activity.props.launch_status == home_activity.LAUNCHING: 

Constants truly belong to the classes or modules they are defined in, not to instances, so Activity.LAUNCHING.

        self._launch = self.LAUNCHING 

launch is a verb, what's wrong with self._launch_status like in other parts of this patch?

    def push_launcher(self, activity_id, launcher): 
...
    def pop_launcher(self, activity_id): 

I find a bit confusing that we use push/pop for something that is not really a stack.

def _on_failed_launch(self): 
    # TODO 
    pass 

I think this deserves a ticket (referenced from the comment) and an email to the design team if you have time. I guess a badge would be useful here.

Otherwise the patch looks good, but as I have had to fix several bugs in this same code before, it would have helped if you had explained why you need to do the refactoring you did.

[alsroot]

1st one was proposed during code freeze and was less invasive.
Last one just is more reliable (in my mind) since Activity.launch property could not be changed out of Activity only is a reaction of launch related signals. That was major idea.

[alsroot]

fixed patch

[tomeu]

Replying to alsroot:

fixed patch

Thanks, pushed as  http://git.sugarlabs.org/projects/sugar/repos/mainline/commits/8e6af12d6