Merge lp:~gary-lasker/software-center/lp1041004 into lp:software-center

Proposed by Gary Lasker
Status: Merged
Merged at revision: 3218
Proposed branch: lp:~gary-lasker/software-center/lp1041004
Merge into: lp:software-center
Diff against target: 58 lines (+19/-5)
2 files modified
softwarecenter/ui/gtk3/views/appdetailsview.py (+5/-5)
tests/gtk3/test_appdetailsview.py (+14/-0)
To merge this branch: bzr merge lp:~gary-lasker/software-center/lp1041004
Reviewer Review Type Date Requested Status
software-store-developers Pending
Review via email: mp+127640@code.launchpad.net

Description of the change

Small branch to fix the crasher in bug 1041004. This is one of the critical ca-escalated bugs. This one appears to have come in with the aptdaemon-based size calculation branch, and this corresponds roughly to when this bug was first reported.

This is currently #4 in the list of Quantal's top crashers at errors.ubuntu.com.

Unit test is included.

Many thanks for your review!

To post a comment you must log in.
3215. By Gary Lasker on 2012-10-03

RED: add a unit test that checks for the errant AttributeError from the bug

3216. By Gary Lasker on 2012-10-03

add the exception details to the error message

3217. By Gary Lasker on 2012-10-03

GREEN: restore the previous fix, test case runs again

Michael Vogt (mvo) wrote :

Thanks for your work on this branch.

I was a bit puzzled under what circumstances AppDetailsView.app_details can become None (because db.get_appdetails() always returns a != None value).

It turns out that the root cause is a race condition if the second appdetailsview
for the installed pane is initialized. https://bugs.launchpad.net/ubuntu/+source/software-center/+bug/1041004/comments/4 has the steps to reproduce. As this will cause other issues as well I created a branch:
lp:~mvo/software-center/fix-size-calc-race that fixes the race (a description of it is in the branch merge description) and as a side-effect fixes this bug here as well.

This is a similar issue as the one lp:~mvo/software-center/downloader-fix-race839462-again.

Michael Vogt (mvo) wrote :

I merge this branch now as the chec for self.app_details is a good idea. I will not merge the test as I can't see any way that "self.view.app.get_details()" could return None, there is a test that can trigger the crash in lp:~mvo/software-center/fix-size-calc-race now but its a bit tricky as a crash in a gtk signal just aborts the signal so its a bit hard to test reliable. The test in that code now tests indirectly.

Gary Lasker (gary-lasker) wrote :

Thanks Michael! Indeed, the test seems quite artificial, but it was useful in verifying the fix by generating the conditions that caused the crash. I agree it is unneeded as a permanent unit test.

Gary Lasker (gary-lasker) wrote :

Hi Michael, it seems that the self.appdetails check is not in trunk yet? Just checking as you mentioned you thought it might be useful. Nice find for lp:~mvo/software-center/fix-size-calc-race! I'm reviewing it now and will merge it soon.

Thanks again!

Michael Vogt (mvo) wrote :

I will merge the check, it should not be needed anymore with the fix-size-calc-race as app/app_details are either both None or both != None but it certainly does not hurt to be defensive.

Gary Lasker (gary-lasker) wrote :

Agreed, I didn't think it needed anymore. But thank you for the merge! It's tiny I guess, and can't hurt. :)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'softwarecenter/ui/gtk3/views/appdetailsview.py'
2--- softwarecenter/ui/gtk3/views/appdetailsview.py 2012-10-01 14:23:31 +0000
3+++ softwarecenter/ui/gtk3/views/appdetailsview.py 2012-10-03 20:40:32 +0000
4@@ -810,6 +810,10 @@
5 self.distro = distro
6 self.icons = icons
7 self.cache = cache
8+ # app specific data
9+ self.app = None
10+ self.app_details = None
11+ self.pkg_state = None
12 self.cache.connect("query-total-size-on-install-done",
13 self._on_query_total_size_on_install_done)
14 self.backend = get_install_backend()
15@@ -836,11 +840,6 @@
16 self.set_name("view")
17
18 self.section = None
19- # app specific data
20- self.app = None
21- self.app_details = None
22- self.pkg_state = None
23-
24 self.reviews = UIReviewsList(self)
25
26 self.adjustment_value = None
27@@ -2030,6 +2029,7 @@
28 install_size = GLib.format_size(total_install_size)
29 label_string += _("%s when installed") % (install_size)
30 elif (total_install_size == 0 and
31+ self.app_details and
32 self.app_details.pkg_state == PkgStates.INSTALLED and
33 not self.addons_manager.addons_to_install and
34 not self.addons_manager.addons_to_remove):
35
36=== modified file 'tests/gtk3/test_appdetailsview.py'
37--- tests/gtk3/test_appdetailsview.py 2012-09-01 11:36:53 +0000
38+++ tests/gtk3/test_appdetailsview.py 2012-10-03 20:40:32 +0000
39@@ -275,6 +275,20 @@
40 self.view.show_app(self.view.app)
41 self.assertEqual(self.view.videoplayer.uri, video_url)
42
43+ @patch('softwarecenter.db.application.Application.get_details')
44+ def test_size_event_before_appdetailsview_is_ready(self,
45+ mock_get_details):
46+ # simulate a size event from the cache when the details view
47+ # has not yet had its app_details value set (LP: #1041004)
48+ mock_get_details.return_value = None
49+ self.view.app_details = self.view.app.get_details()
50+ try:
51+ self.view._on_query_total_size_on_install_done(None, 400, 0)
52+ except AttributeError as e:
53+ self.fail("received an unexpected attribute error on a call to"
54+ "AppDetailsView._on_query_total_size_on_install_done() "
55+ "%s " % e)
56+
57
58 class MultipleVersionsTestCase(BaseViewTestCase):
59

Subscribers

People subscribed via source and target branches