Adding patch for webbrowser on macOS

rtv/__main__.py

@@ -26,6 +26,7 @@ except ImportError:
 # If we want to override the $BROWSER variable that the python webbrowser
 # references, it needs to be done before the webbrowser module is imported
 # for the first time.
+webbrowser_import_warning = ('webbrowser' in sys.modules)
 RTV_BROWSER, BROWSER = os.environ.get('RTV_BROWSER'), os.environ.get('BROWSER')
 if RTV_BROWSER:
     os.environ['BROWSER'] = RTV_BROWSER
@@ -36,7 +37,7 @@ from .packages import praw
 from .config import Config, copy_default_config, copy_default_mailcap
 from .oauth import OAuthHelper
 from .terminal import Terminal
-from .objects import curses_session, Color
+from .objects import curses_session, Color, patch_webbrowser
 from .subreddit_page import SubredditPage
 from .exceptions import ConfigError
 from .__version__ import __version__
@@ -156,6 +157,12 @@ def main():
         _logger.info('Packaged PRAW not found, falling back to system '
                      'installed version %s', praw.__version__)
 
+    # Update the webbrowser module's default behavior
+    patch_webbrowser()
+    if webbrowser_import_warning:
+        _logger.warning('webbrowser module was unexpectedly imported before'
+                        '$BROWSER could be overwritten')
+
     # Construct the reddit user agent
     user_agent = docs.AGENT.format(version=__version__)
 

rtv/objects.py

@@ -3,12 +3,14 @@ from __future__ import unicode_literals
 
 import re
 import os
+import sys
 import time
 import signal
 import inspect
 import weakref
 import logging
 import threading
+import webbrowser
 import curses
 import curses.ascii
 from contextlib import contextmanager
@@ -23,6 +25,26 @@ from .packages import praw
 _logger = logging.getLogger(__name__)
 
 
+def patch_webbrowser():
+    """
+    Patch webbrowser on macOS to support setting BROWSER=firefox,
+    BROWSER=chrome, etc..
+    
+    https://bugs.python.org/issue31348
+    """
+
+    if sys.platform == 'darwin':
+
+        # This is a copy of what's at the end of webbrowser.py, except that
+        # it adds MacOSXOSAScript entries instead of GenericBrowser entries.
+        if "BROWSER" in os.environ:
+            _userchoices = os.environ["BROWSER"].split(os.pathsep)
+            for cmdline in reversed(_userchoices):
+                if cmdline in ('safari', 'firefox', 'chrome', 'default'):
+                    browser = webbrowser.MacOSXOSAScript(cmdline)
+                    webbrowser.register(cmdline, None, browser, -1)
+
+
 @contextmanager
 def curses_session():
     """

rtv/terminal.py

@@ -13,6 +13,7 @@ import webbrowser
 import subprocess
 import curses.ascii
 from curses import textpad
+from multiprocessing import Process
 from contextlib import contextmanager
 from tempfile import NamedTemporaryFile
 
@@ -464,11 +465,12 @@ class Terminal(object):
         python webbrowser will try to determine the default to use based on
         your system.
 
-        For browsers requiring an X display, we call
+        For browsers requiring an X display, we open a new subprocess and
-        webbrowser.open_new_tab(url) and redirect stdout/stderr to devnull.
+        redirect stdout/stderr to devnull. This is a workaround to stop
-        This is a workaround to stop firefox from spewing warning messages to
+        BackgroundBrowsers (e.g. xdg-open, any BROWSER command ending in "&"),
-        the console. See http://bugs.python.org/issue22277 for a better
+        from spewing warning messages to the console. See
-        description of the problem.
+        http://bugs.python.org/issue22277 for a better description of the
+        problem.
 
         For console browsers (e.g. w3m), RTV will suspend and display the
         browser window within the same terminal. This mode is triggered either
@@ -479,36 +481,33 @@ class Terminal(object):
            headless
 
         There may be other cases where console browsers are opened (xdg-open?)
-        but are not detected here.
+        but are not detected here. These cases are still unhandled and will
+        probably be broken if we incorrectly assume that self.display=True.
         """
 
         if self.display:
-            # Note that we need to sanitize the url before inserting it into
+            with self.loader('Opening page in a new window'):
-            # the python code to prevent injection attacks.
+
-            command = (
+                def open_url_silent(url):
-                "import webbrowser\n"
+                    # This used to be done using subprocess.Popen().
-                "from six.moves.urllib.parse import unquote\n"
+                    # It was switched to multiprocessing.Process so that we
-                "webbrowser.open_new_tab(unquote('%s'))" % quote(url))
+                    # can re-use the webbrowser instance that has been patched
-            args = [sys.executable, '-c', command]
+                    # by RTV. It's also safer because it doesn't inject
-            with self.loader('Opening page in a new window'), \
+                    # python code through the command line.
-                    open(os.devnull, 'ab+', 0) as null:
+                    null = open(os.devnull, 'ab+', 0)
-                p = subprocess.Popen(args, stdout=null, stderr=null)
+                    sys.stdout, sys.stderr = null, null
-                # Give the browser 5 seconds to open a new tab. Because the
+                    webbrowser.open_new_tab(url)
+
+                p = Process(target=open_url_silent, args=(url,))
+                p.start()
+                # Give the browser 7 seconds to open a new tab. Because the
                 # display is set, calling webbrowser should be non-blocking.
                 # If it blocks or returns an error, something went wrong.
                 try:
-                    start = time.time()
+                    p.join(7)
-                    while time.time() - start < 10:
+                    if p.is_alive():
-                        code = p.poll()
-                        if code == 0:
-                            break  # Success
-                        elif code is not None:
-                            raise exceptions.BrowserError(
-                                'Program exited with status=%s' % code)
-                        time.sleep(0.01)
-                    else:
                         raise exceptions.BrowserError(
-                            'Timeout opening browser')
+                            'Timeout waiting for browser to open')
                 finally:
                     # Can't check the loader exception because the oauth module
                     # supersedes this loader and we need to always kill the

tests/test_objects.py

@@ -5,13 +5,15 @@ import time
 import curses
 from collections import OrderedDict
 
+import os
 import six
 import pytest
 import requests
+from six.moves import reload_module
 
 from rtv import exceptions
 from rtv.objects import Color, Controller, Navigator, Command, KeyMap, \
-    curses_session
+    curses_session, patch_webbrowser
 
 try:
     from unittest import mock
@@ -19,6 +21,28 @@ except ImportError:
     import mock
 
 
+@mock.patch.dict(os.environ, {'BROWSER': 'safari'})
+@mock.patch('sys.platform', 'darwin')
+@mock.patch('shutil.which', return_value=None)    # py3 method
+@mock.patch('os.path.isfile', return_value=None)  # py2 method
+def test_patch_webbrowser(which, isfile):
+
+    # Make sure that webbrowser re-generates the browser list using the
+    # mocked environment
+    import webbrowser
+    webbrowser = reload_module(webbrowser)
+
+    # By default, we expect that BROWSER will be loaded as a generic browser
+    # This is because "safari" is not a valid script in the system PATH
+    assert isinstance(webbrowser.get(), webbrowser.GenericBrowser)
+
+    # After patching, the default webbrowser should now be interpreted as an
+    # OSAScript browser
+    patch_webbrowser()
+    assert isinstance(webbrowser.get(), webbrowser.MacOSXOSAScript)
+    assert webbrowser._tryorder[0] == 'safari'
+
+
 @pytest.mark.parametrize('use_ascii', [True, False])
 def test_objects_load_screen(terminal, stdscr, use_ascii):
     terminal.config['ascii'] = use_ascii