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

[Gary Lasker]

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!

[lp:~gary-lasker/software-center/lp1041004]

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)]

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)]

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)]

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)]

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)]

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)]

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

=== modified file 'softwarecenter/ui/gtk3/views/appdetailsview.py'
--- softwarecenter/ui/gtk3/views/appdetailsview.py	2012-10-01 14:23:31 +0000
+++ softwarecenter/ui/gtk3/views/appdetailsview.py	2012-10-03 20:40:32 +0000
@@ -810,6 +810,10 @@
         self.distro = distro
         self.icons = icons
         self.cache = cache
+        # app specific data
+        self.app = None
+        self.app_details = None
+        self.pkg_state = None
         self.cache.connect("query-total-size-on-install-done",
                            self._on_query_total_size_on_install_done)
         self.backend = get_install_backend()
@@ -836,11 +840,6 @@
         self.set_name("view")
 
         self.section = None
-        # app specific data
-        self.app = None
-        self.app_details = None
-        self.pkg_state = None
-
         self.reviews = UIReviewsList(self)
 
         self.adjustment_value = None
@@ -2030,6 +2029,7 @@
             install_size = GLib.format_size(total_install_size)
             label_string += _("%s when installed") % (install_size)
         elif (total_install_size == 0 and
+              self.app_details and
               self.app_details.pkg_state == PkgStates.INSTALLED and
               not self.addons_manager.addons_to_install and
               not self.addons_manager.addons_to_remove):

=== modified file 'tests/gtk3/test_appdetailsview.py'
--- tests/gtk3/test_appdetailsview.py	2012-09-01 11:36:53 +0000
+++ tests/gtk3/test_appdetailsview.py	2012-10-03 20:40:32 +0000
@@ -275,6 +275,20 @@
         self.view.show_app(self.view.app)
         self.assertEqual(self.view.videoplayer.uri, video_url)
 
+    @patch('softwarecenter.db.application.Application.get_details')
+    def test_size_event_before_appdetailsview_is_ready(self,
+                                                       mock_get_details):
+        # simulate a size event from the cache when the details view
+        # has not yet had its app_details value set (LP: #1041004)
+        mock_get_details.return_value = None
+        self.view.app_details = self.view.app.get_details()
+        try:
+            self.view._on_query_total_size_on_install_done(None, 400, 0)
+        except AttributeError as e:
+            self.fail("received an unexpected attribute error on a call to"
+                      "AppDetailsView._on_query_total_size_on_install_done() "
+                      "%s " % e)
+
 
 class MultipleVersionsTestCase(BaseViewTestCase):