#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)
Change History (17)
comment:1 Changed 14 years ago by alsroot
- Owner changed from tomeu to alsroot
- Status changed from new to assigned
comment:2 Changed 14 years ago by alsroot
- Resolution set to duplicate
- Status changed from assigned to closed
Changed 14 years ago by alsroot
comment:3 Changed 14 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: ↓ 5 Changed 14 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: ↓ 6 Changed 14 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 14 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 14 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 13 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 13 years ago by alsroot
comment:9 Changed 13 years ago by alsroot
- Keywords r? added; r- removed
comment:10 Changed 13 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 13 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 13 years ago by alsroot
comment:12 follow-up: ↓ 13 Changed 13 years ago by alsroot
- Keywords r? added; r! removed
fixed patch
comment:13 in reply to: ↑ 12 Changed 13 years ago by tomeu
- Keywords r+ added; r? removed
- Resolution set to fixed
- Status changed from reopened to closed
Replying to alsroot:
fixed patch
Thanks, pushed as http://git.sugarlabs.org/projects/sugar/repos/mainline/commits/8e6af12d6
Proper fix was attached to #1814 since patch heavily depends of #1814. 0.84 fix will be posted to downstream tracker.