Merge lp:~rohangarg/apport/fix-for-1282713 into lp:~apport-hackers/apport/trunk

[Rohan Garg]

[Martin Pitt (pitti)]

"introduces another bug which causes apport to launch a dialog saying that it could launch the browser, when in reality it did"

I take it you mean "could NOT launch the browser". I doubt that this is apport itself, presumably it's from xdg-open? Do you have a screenshot of this?

I wonder if we can work around the crash without introducing this confusing dialog. A shot into the blue: What happens if in apport/ui.py "def open_url" you replace the two sys.exit() calls with os._exit()? That'll bypass the Python and sip destructors. I wonder if that crash happens in open_url only, or at a different place.

Thanks!

review: Needs Information

[Rohan Garg (rohangarg)]

> "introduces another bug which causes apport to launch a dialog saying that it
> could launch the browser, when in reality it did"
>
> I take it you mean "could NOT launch the browser". I doubt that this is apport
> itself, presumably it's from xdg-open? Do you have a screenshot of this?
>

Heh, yes, however, from looking at the code [1] I think it's from apport itself,
mandatory screenshot : http://wstaw.org/m/2014/03/31/plasma-desktopVB2282.png

> I wonder if we can work around the crash without introducing this confusing
> dialog. A shot into the blue: What happens if in apport/ui.py "def open_url"
> you replace the two sys.exit() calls with os._exit()? That'll bypass the
> Python and sip destructors. I wonder if that crash happens in open_url only,
> or at a different place.
>

os._exit doesn't help unfortunately

[1] http://bazaar.launchpad.net/~apport-hackers/apport/trunk/view/head:/apport/ui.py#L1129

[Martin Pitt (pitti)]

Ah right, thanks. There's no exception in the error dialog, but perhaps the piping doesn't work; as that's the only place where it explicitly does exit(1) it ought to write the exception string into the pipe, but doesn't. So we need to find out where in open_url() things go wrong by sprinkling lots of print() statements there. If you feel up to that, please do. Otherwise I can try and reproduce this next week (installing python3-pykde4 pulls in half of KDE, can't download that much at the moment).

[Rohan Garg (rohangarg)]

I actually did successfully extract the exit status of xdg-open, apparently status is 256 in int ( or as os.wait documentation tells me, the high byte of a 16 bit number, making it a series of 8 1's ).

Should have mentioned that in the last comment :)

[Martin Pitt (pitti)]

Rohan,

ah, that's good information. Apport does not currently check the return code of xdg-open, but the fact that it even got that far already rules out some error paths. Could you add some print statements there for debugging?

        try:
            try:
                subprocess.call(sudo_prefix + ['xdg-open', url])
                print('XXX calling xdg-open successful')
            except OSError as e:
                print('XXX OSError trying to call xdg-open:', e)
                # fall back to webbrowser
                webbrowser.open(url, new=True, autoraise=True)
                sys.exit(0)
        except Exception as e:
            print('XXX unexpected error trying to call xdg-open:', e)
            os.write(w, str(e))
            sys.exit(1)

What does that show if you run it?

[Rohan Garg (rohangarg)]

Gives me : XXX calling xdg-open successful

[Rohan Garg (rohangarg)]

I also get :

Error in atexit._run_exitfuncs:
Traceback (most recent call last):
  File "/usr/lib/python3.4/shutil.py", line 452, in rmtree
    orig_st = os.lstat(path)
FileNotFoundError: [Errno 2] No such file or directory: '/tmp/launchpadlib.cache.utowmzgw'

[Martin Pitt (pitti)]

> Gives me : XXX calling xdg-open successful

Ah, thanks. So xdg-open is fine, the two "except" are not triggered, and the actual open_url() forked function exits cleanly. That's also consistent with os._exit() not making any difference. So the exit 1 somehow happens during cleanup in some atexit/destructors.

Can you please try what happens with this:

        except Exception as e:
            os.write(w, str(e))
            sys.exit(1)
+ os._exit(0)

I. e. just append the os._exit(0) at the very end of open_url(). That circumvents the cleanup handlers, which is a good idea anyway because that's just a forked child, not the real application. This also ought to make the setdestroyonexit() unnecessary. Crossing fingers :-)

[Martin Pitt (pitti)]

> I also get: FileNotFoundError

That's already fixed in trunk.

[Rohan Garg (rohangarg)]

Adding os._exit(0) helps, but I still need setdestroyonexit because without it apport still crashes

[Martin Pitt (pitti)]

Just to confirm, with both patches (os._exit() and setdestroyonexit) the crash is fixed and the confusing "can't launch web browser" doesn't happen? Great!

review: Approve

[Martin Pitt (pitti)]

I committed the os._exit() fix and merged this. Thanks!

review: Approve

=== modified file 'kde/apport-kde'
--- kde/apport-kde	2014-01-27 09:30:42 +0000
+++ kde/apport-kde	2014-03-31 15:05:03 +0000
@@ -31,6 +31,13 @@
                               KStandardGuiItem)
     import apport.ui
     from apport import unicode_gettext as _
+    import sip
+    ## Work around for LP: 1282713
+    try:
+        sip.setdestroyonexit(False)
+    except AttributeError:
+       pass
+
 except ImportError as e:
     # this can happen while upgrading python packages
     apport.fatal('Could not import module, is a package upgrade in progress?  Error: %s', str(e))