Opened 11 years ago

Closed 10 years ago

Last modified 7 years ago

#1719 closed defect (fixed)

resume journal entry race may duplicate resumed activity id

Reported by: quozl Owned by: alsroot
Priority: Unspecified by Maintainer Milestone:
Component: Sugar Version: Git as of bugdate
Severity: Major Keywords: r+
Cc: Distribution/OS: Unspecified
Bug Status: Assigned

Description

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

Attachments (3)

sugar-1719.patch (3.1 KB) - added by alsroot 11 years ago.
sugar-1719.2.patch (12.0 KB) - added by alsroot 10 years ago.
sugar-1719.3.patch (12.1 KB) - added by alsroot 10 years ago.

Download all attachments as: .zip

Change History (17)

comment:1 Changed 11 years ago by alsroot

  • Owner changed from tomeu to alsroot
  • Status changed from new to assigned

comment:2 Changed 11 years ago by alsroot

  • Resolution set to duplicate
  • Status changed from assigned to closed

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

Changed 11 years ago by alsroot

comment:3 Changed 11 years ago by alsroot

  • Keywords r? added
  • Milestone changed from Unspecified by Release Team to 0.88
  • Resolution duplicate deleted
  • Severity changed from Minor to Major
  • Status changed from closed to reopened
  • Version changed from Unspecified to Git as of bugdate

looks like patch could be less invasive

comment:4 follow-up: Changed 11 years ago by erikos

  • Bug Status changed from Unconfirmed to Assigned
  • Milestone changed from 0.88 to 0.88.x

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.

comment:5 in reply to: ↑ 4 ; follow-up: Changed 11 years ago by 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.

comment:6 in reply to: ↑ 5 Changed 11 years ago by 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.

comment:7 Changed 11 years ago by 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.

comment:8 Changed 10 years ago by tomeu

  • Keywords r- added; r? removed

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

Changed 10 years ago by alsroot

comment:9 Changed 10 years ago by alsroot

  • Keywords r? added; r- removed

comment:10 Changed 10 years ago by tomeu

  • Keywords r! added; r? removed

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.

comment:11 Changed 10 years ago by 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.

Changed 10 years ago by alsroot

comment:12 follow-up: Changed 10 years ago by alsroot

  • Keywords r? added; r! removed

fixed patch

comment:13 in reply to: ↑ 12 Changed 10 years ago by tomeu

  • Keywords r+ added; r? removed
  • Resolution set to fixed
  • Status changed from reopened to closed

comment:14 Changed 7 years ago by dnarvaez

  • Milestone 0.88.x deleted

Milestone 0.88.x deleted

Note: See TracTickets for help on using tickets.