More work on #888 (new IPC path).

First trying the legacy path and then using the new one works fine now, but the
permissions are still wrong.

qutebrowser/misc/ipc.py

@@ -20,6 +20,7 @@
 """Utilities for IPC with existing instances."""
 
 import os
+import sys
 import time
 import json
 import getpass
@@ -39,11 +40,8 @@ READ_TIMEOUT = 5000
 PROTOCOL_VERSION = 1
 
 
-def _get_socketname(basedir, runtime_dir, legacy=True, user=None):
+def _get_socketname(basedir, legacy=False):
     """Get a socketname to use."""
-    if user is None:
-        user = getpass.getuser()
-
     if basedir is None:
         basedir_md5 = None
     else:
@@ -51,15 +49,22 @@ def _get_socketname(basedir, runtime_dir, legacy=True, user=None):
         basedir_md5 = md5.hexdigest()
 
     if legacy or os.name == 'nt':
-        parts = ['qutebrowser', user]
+        parts = ['qutebrowser', getpass.getuser()]
         if basedir_md5 is not None:
             parts.append(basedir_md5)
         return '-'.join(parts)
-    else:
+
+    if sys.platform.startswith('linux'):
+        target_dir = standarddir.runtime()
         parts = ['qutebrowser-ipc']
-        if basedir_md5 is not None:
+    else:  # pragma: no cover
-            parts.append(basedir_md5)
+        # OS X or other Unix
-        return os.path.join(runtime_dir, '-'.join(parts))
+        parts = ['ipc']
+        target_dir = standarddir.temp()
+
+    if basedir_md5 is not None:
+        parts.append(basedir_md5)
+    return os.path.join(target_dir, '-'.join(parts))
 
 
 class Error(Exception):
@@ -186,7 +191,7 @@ class IPCServer(QObject):
         if self._socket is None:
             # Sometimes this gets called from stale sockets.
             msg = "In on_error with None socket!"
-            if os.name == 'nt':  # pragma: no coverage
+            if os.name == 'nt':  # pragma: no cover
                 # This happens a lot on Windows, so we ignore it there.
                 log.ipc.debug(msg)
             else:
@@ -319,7 +324,28 @@ class IPCServer(QObject):
         self._remove_server()
 
 
-def send_to_running_instance(socketname, command, *, socket=None):
+def _has_legacy_server(name):
+    """Check if there is a legacy server.
+
+    Args:
+        name: The name to try to connect to.
+
+    Return:
+        True if there is a server with the given name, False otherwise.
+    """
+    socket = QLocalSocket()
+    log.ipc.debug("Trying to connect to {}".format(name))
+    socket.connectToServer(name)
+    if socket.error() != QLocalSocket.ServerNotFoundError:
+        return True
+    socket.disconnectFromServer()
+    if socket.state() != QLocalSocket.UnconnectedState:
+        socket.waitForDisconnected(100)
+    return False
+
+
+def send_to_running_instance(socketname, command, *, legacy_name=None,
+                             socket=None):
     """Try to send a commandline to a running instance.
 
     Blocks for CONNECT_TIMEOUT ms.
@@ -328,14 +354,23 @@ def send_to_running_instance(socketname, command, *, socket=None):
         socketname: The name which should be used for the socket.
         command: The command to send to the running instance.
         socket: The socket to read data from, or None.
+        legacy_name: The legacy name to first try to connect to.
 
     Return:
         True if connecting was successful, False if no connection was made.
     """
     if socket is None:
         socket = QLocalSocket()
-    log.ipc.debug("Connecting to {}".format(socketname))
+
-    socket.connectToServer(socketname)
+    if (legacy_name is not None and
+            _has_legacy_server(legacy_name)):
+        name_to_use = legacy_name
+    else:
+        name_to_use = socketname
+
+    log.ipc.debug("Connecting to {}".format(name_to_use))
+    socket.connectToServer(name_to_use)
+
     connected = socket.waitForConnected(100)
     if connected:
         log.ipc.info("Opening in existing instance")
@@ -386,10 +421,12 @@ def send_or_listen(args):
         The IPCServer instance if no running instance was detected.
         None if an instance was running and received our request.
     """
-    socketname = _get_socketname(args.basedir, standarddir.runtime())
+    socketname = _get_socketname(args.basedir)
+    legacy_socketname = _get_socketname(args.basedir, legacy=True)
     try:
         try:
-            sent = send_to_running_instance(socketname, args.command)
+            sent = send_to_running_instance(socketname, args.command,
+                                            legacy_name=legacy_socketname)
             if sent:
                 return None
             log.init.debug("Starting IPC server...")
@@ -401,7 +438,8 @@ def send_or_listen(args):
             # This could be a race condition...
             log.init.debug("Got AddressInUseError, trying again.")
             time.sleep(0.5)
-            sent = send_to_running_instance(socketname, args.command)
+            sent = send_to_running_instance(socketname, args.command,
+                                            legacy_name=legacy_socketname)
             if sent:
                 return None
             else:

qutebrowser/utils/standarddir.py

@@ -85,6 +85,7 @@ def _from_args(typ, args):
             override: boolean, if the user did override the path
             path: The overridden path, or None to turn off storage.
     """
+    # pylint: disable=too-many-return-statements
     typ_to_argparse_arg = {
         QStandardPaths.ConfigLocation: 'confdir',
         QStandardPaths.DataLocation: 'datadir',

tests/unit/misc/test_ipc.py

@@ -19,6 +19,8 @@
 
 """Tests for qutebrowser.misc.ipc."""
 
+import sys
+import os
 import getpass
 import collections
 import logging
@@ -73,7 +75,7 @@ def qlocalsocket(qapp):
 @pytest.fixture(autouse=True)
 def fake_runtime_dir(monkeypatch, tmpdir):
     monkeypatch.setenv('XDG_RUNTIME_DIR', str(tmpdir))
-    return str(tmpdir)
+    return tmpdir
 
 
 class FakeSocket(QObject):
@@ -165,21 +167,61 @@ def test_getpass_getuser():
     assert getpass.getuser()
 
 
-@pytest.mark.parametrize('username, basedir, legacy, expected', [
+class TestSocketName:
-    ('florian', None, True, 'qutebrowser-florian'),
-    ('florian', '/x', True,
-        'qutebrowser-florian-cc8755609ad61864910f145119713de9'),
-    (None, None, True, 'qutebrowser-{}'.format(getpass.getuser())),
 
-    ('florian', None, False, '/runtimedir/qutebrowser-ipc'),
+    MD5 = 'cc8755609ad61864910f145119713de9'  # The MD5 of 'x'
-    ('florian', '/x', False,
+
-        '/runtimedir/qutebrowser-ipc-cc8755609ad61864910f145119713de9'),
+    LEGACY_TESTS = [
-    (None, None, False, '/runtimedir/qutebrowser-ipc'),
+        (None, 'qutebrowser-{}'.format(getpass.getuser())),
-])
+        ('/x', 'qutebrowser-{}-{}'.format(getpass.getuser(), MD5)),
-def test_get_socketname(username, basedir, legacy, expected):
+    ]
-    socketname = ipc._get_socketname(basedir, '/runtimedir', legacy=legacy,
+
-                                     user=username)
+    @pytest.mark.parametrize('basedir, expected', LEGACY_TESTS)
-    assert socketname == expected
+    def test_legacy(self, basedir, expected):
+        socketname = ipc._get_socketname(basedir, legacy=True)
+        assert socketname == expected
+
+    @pytest.mark.parametrize('basedir, expected', LEGACY_TESTS)
+    @pytest.mark.windows
+    def test_windows(self, basedir, expected):
+        socketname = ipc._get_socketname(basedir)
+        assert socketname == expected
+
+    @pytest.mark.osx
+    @pytest.mark.parametrize('basedir, expected', [
+        (None, 'ipc'),
+        ('/x', 'ipc-{}'.format(MD5)),
+    ])
+    def test_os_x(self, basedir, expected):
+        socketname = ipc._get_socketname(basedir)
+        parts = os.path.split(socketname)
+        assert parts[-2] == 'qutebrowser_test'
+        assert parts[-1] == expected
+
+    @pytest.mark.linux
+    @pytest.mark.parametrize('basedir, expected', [
+        (None, 'qutebrowser-ipc'),
+        ('/x', 'qutebrowser-ipc-{}'.format(MD5)),
+    ])
+    def test_linux(self, basedir, fake_runtime_dir, expected):
+        socketname = ipc._get_socketname(basedir)
+        expected_path = str(fake_runtime_dir / 'qutebrowser_test' / expected)
+        assert socketname == expected_path
+
+    def test_other_unix(self):
+        """Fake test for POSIX systems which aren't Linux/OS X.
+
+        We probably would adjust the code first to make it work on that
+        platform.
+        """
+        if os.name == 'nt':
+            pass
+        elif sys.platform == 'darwin':
+            pass
+        elif sys.platform.startswith('linux'):
+            pass
+        else:
+            raise Exception("Unexpected platform!")
 
 
 class TestExceptions:
@@ -523,6 +565,15 @@ class TestSendOrListen:
             setattr(m, attr, getattr(QLocalSocket, attr))
         return m
 
+    @pytest.yield_fixture
+    def legacy_server(self, args, tmpdir, monkeypatch):
+        monkeypatch.setenv('TMPDIR', str(tmpdir))
+        legacy_name = ipc._get_socketname(args.basedir, legacy=True)
+        legacy_server = ipc.IPCServer(legacy_name)
+        legacy_server.listen()
+        yield legacy_server
+        legacy_server.shutdown()
+
     @pytest.mark.posix   # Flaky on Windows
     def test_normal_connection(self, caplog, qtbot, args):
         ret_server = ipc.send_or_listen(args)
@@ -538,6 +589,21 @@ class TestSendOrListen:
         assert ret_client is None
         ret_server.shutdown()
 
+    @pytest.mark.posix   # Unneeded on Windows
+    def test_legacy_name(self, caplog, qtbot, args, legacy_server):
+        with qtbot.waitSignal(legacy_server.got_args, raising=True):
+            ret = ipc.send_or_listen(args)
+        assert ret is None
+        msgs = [e.message for e in caplog.records()]
+        assert "Connecting to {}".format(legacy_server._socketname) in msgs
+
+    @pytest.mark.posix   # Unneeded on Windows
+    def test_correct_socket_name(self, args):
+        server = ipc.send_or_listen(args)
+        expected_dir = ipc._get_socketname(args.basedir)
+        assert '/' in expected_dir
+        assert server._socketname == expected_dir
+
     def test_address_in_use_ok(self, qlocalserver_mock, qlocalsocket_mock,
                                stubs, caplog, args):
         """Test the following scenario:
@@ -555,7 +621,9 @@ class TestSendOrListen:
 
         qlocalsocket_mock().waitForConnected.side_effect = [False, True]
         qlocalsocket_mock().error.side_effect = [
+            QLocalSocket.ServerNotFoundError,  # legacy name
             QLocalSocket.ServerNotFoundError,
+            QLocalSocket.ServerNotFoundError,  # legacy name
             QLocalSocket.UnknownSocketError,
             QLocalSocket.UnknownSocketError,  # error() gets called twice
         ]
@@ -591,8 +659,10 @@ class TestSendOrListen:
         # If it fails, that's the "not sent" case above.
         qlocalsocket_mock().waitForConnected.side_effect = [False, has_error]
         qlocalsocket_mock().error.side_effect = [
+            QLocalSocket.ServerNotFoundError,  # legacy name
             QLocalSocket.ServerNotFoundError,
             QLocalSocket.ServerNotFoundError,
+            QLocalSocket.ServerNotFoundError,  # legacy name
             QLocalSocket.ConnectionRefusedError,
             QLocalSocket.ConnectionRefusedError,  # error() gets called twice
         ]
@@ -641,13 +711,27 @@ class TestSendOrListen:
         assert records[0].msg == '\n'.join(error_msgs)
 
 
-def test_long_username(fake_runtime_dir):
+@pytest.mark.windows
+@pytest.mark.osx
+def test_long_username(monkeypatch):
     """See https://github.com/The-Compiler/qutebrowser/issues/888."""
-    name = ipc._get_socketname(basedir='/foo',
+    username = 'alexandercogneau'
-                               runtime_dir=fake_runtime_dir,
+    monkeypatch.setattr('qutebrowser.misc.ipc.standarddir.getpass.getuser',
-                               user='alexandercogneau')
+                        lambda: username)
+    name = ipc._get_socketname(basedir='/foo')
     server = ipc.IPCServer(name)
+    assert username in server._socketname
     try:
         server.listen()
     finally:
         server.shutdown()
+
+
+def test_connect_inexistant(qlocalsocket):
+    """Make sure connecting to an inexistant server fails immediately.
+
+    If this test fails, our connection logic checking for the old naming scheme
+    would not work properly.
+    """
+    qlocalsocket.connectToServer('qutebrowser-test-inexistent')
+    assert qlocalsocket.error() == QLocalSocket.ServerNotFoundError

tests/unit/utils/test_standarddir.py

@@ -232,7 +232,7 @@ class TestArguments:
             lambda _typ: str(tmpdir))
         args = types.SimpleNamespace(confdir=None, cachedir=None, datadir=None)
         standarddir.init(args)
-        assert standarddir.runtime() == str(tmpdir)
+        assert standarddir.runtime() == str(tmpdir / 'qutebrowser_test')
 
     @pytest.mark.parametrize('typ', ['config', 'data', 'cache', 'download',
                                      'runtime'])