Request.send_error shouldn't load and render the error template

[Ryan J Ollos]

The Request.send_error​ method has a method-scoped import for Chrome and does the work of loading and rendering the error template.

For a normal request the work of rendering a template is done in trac.web.main. I propose having a _send_error method in trac.web.main that does the work of loading and rendering the error template, and have Request.send_error behave more like Request.send.

Request.send_error has rendered the template since it was added in r2957. At that time, the Request.display method (appears to be similar to Request.send) would also render the HDF template. Request.send was added in r3832 and Request.display was removed in r10405 (when ClearSilver support was removed).

Similar to the changes in r14604, there is some nesting of exception handlers in trac.web.main that makes the code difficult to follow. There will be some proposed changes to attempt to clean up the logic.

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t13042_move_error_rendering.1. [391643004/rjollos.git] is targeted to Trac 1.5.1 (#12787).

[Ryan J Ollos]

Fixed test failure on AppVeyor in [4176ef214/rjollos.git].

[Jun Omae]

In [50be720ee/rjollos.git], match_request(req) is not invoked when req.path_info is empty or '/'. I think behavior of IRequestHandler would be changed (e.g. a component has match_request(req) which returns True if req.path_info is empty).

We should call match_request(req) even if req.path_info is empty or '/', otherwise we should add documentation to IRequestHandler.match_request about the changes.

I'm reviewing Request.send_error and _send_error yet….

[Ryan J Ollos]

I was thinking that the default handler should always handle a request to the path /. However, I'm open to possibilities that there might be a use case in which a plugin would handle the request rather than the default handler.

[Ryan J Ollos]

Removed [50be720ee/rjollos.git] and rebased: log:rjollos.git:t13042_move_error_rendering.3.

[Ryan J Ollos]

Committed to trunk in r16690 and r16691. Related: comment:13:ticket:12787.

[Ryan J Ollos]

Found an issue while comment:5:ticket:13038:

Traceback (most recent call last):
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/api.py", line 814, in send_error
    if template.endswith('.html'):
AttributeError: 'generator' object has no attribute 'endswith'

Proposed fix, but I'll also add a test:

trac/web/api.py

@@ -811 +811 @@
             that renders an error page will be removed in 1.5.1.
         """
         try:
-            if template.endswith('.html'):
+            if isinstance(template, basestring) and template.endswith('.html'):
                 if env:
                     from trac.web.chrome import Chrome, add_stylesheet
                     add_stylesheet(self, 'common/css/code.css')

[Ryan J Ollos]

I had an error in the TestCase that resulted in no test coverage with chunked encoding enabled. Fixed in r16692.

[Ryan J Ollos]

I noticed an issue while testing #12611. Previously an invalid value for tracd's projenv argument would result in a browser message like the following when navigating to the URL:

Error

TracError: No Trac environment found at /Users/rjollos/Documents/Workspace/trac-dev/not-an-env
[Errno 2] No such file or directory: '/Users/rjollos/Documents/Workspace/trac-dev/not-an-env/VERSION'

Now, an exception occurs:

Exception happened during processing of request from ('127.0.0.1', 64823)
Traceback (most recent call last):
  File "/Users/rjollos/.pyenv/versions/2.7.14/lib/python2.7/SocketServer.py", line 596, in process_request_thread
    self.finish_request(request, client_address)
  File "/Users/rjollos/.pyenv/versions/2.7.14/lib/python2.7/SocketServer.py", line 331, in finish_request
    self.RequestHandlerClass(request, client_address, self)
  File "/Users/rjollos/.pyenv/versions/2.7.14/lib/python2.7/SocketServer.py", line 652, in __init__
    self.handle()
  File "/Users/rjollos/.pyenv/versions/2.7.14/lib/python2.7/BaseHTTPServer.py", line 340, in handle
    self.handle_one_request()
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/wsgi.py", line 212, in handle_one_request
    gateway.run(self.server.application)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/wsgi.py", line 112, in run
    response = application(self.environ, self._start_response)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/standalone.py", line 95, in __call__
    return self.application(environ, start_response)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/main.py", line 633, in dispatch_request
    dispatcher = RequestDispatcher(env)
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/core.py", line 144, in __call__
    "First argument must be a ComponentManager instance"
AssertionError: First argument must be a ComponentManager instance

It looks like the issue is fixed by the following:

trac/web/main.py

@@ -630 +630 @@
         env.abs_href = req.abs_href
     translation.make_activable(lambda: req.locale, env.path if env else None)
     resp = []
-    dispatcher = RequestDispatcher(env)
-    dispatcher.set_default_callbacks(req)
     try:
         if not env and env_error:
             raise HTTPInternalServerError(env_error)
+        dispatcher = RequestDispatcher(env)
+        dispatcher.set_default_callbacks(req)
         try:
             dispatcher.dispatch(req)
         except RequestDone as req_done:

I will look at adding test coverage.

[Ryan J Ollos]

I've written a test, but I get slightly different error messages depending on whether Babel is installed. Without Babel installed, the first line is "Trac Error". With Babel installed, the first line is "Error". With Babel installed, an AttributeError is raised in HTTPException.title​, which results in "Error" being returned as the title property rather than self.reason. Printing the exception with traceback:

Traceback (most recent call last):
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/web/api.py", line 268, in title
    title = _("Error")
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/util/translation.py", line 200, in gettext
    if not self.isactive:
  File "/Users/rjollos/Documents/Workspace/trac-dev/teo-rjollos.git/trac/util/translation.py", line 195, in __getattr__
    return getattr(self.active, name)
AttributeError: NullTranslationsBabel instance has no attribute 'isactive'

I'm unsure whether this the correct behavior.

[Jun Omae]

Replying to Ryan J Ollos:

It looks like the issue is fixed by the following:

-    dispatcher = RequestDispatcher(env)
-    dispatcher.set_default_callbacks(req)
     try:
         if not env and env_error:
             raise HTTPInternalServerError(env_error)
+        dispatcher = RequestDispatcher(env)
+        dispatcher.set_default_callbacks(req)

Yeah. Component class cannot accept None to construct. The same issue exists in send_internal_error.

trac/web/main.py

@@ -754 +754 @@
                     plugin_name = info.get('home_page', '').rstrip('/') \
                                                            .split('/')[-1]
                     tracker_args = {'component': plugin_name}
-        interface_custom = Chrome(env).get_interface_customization_files()
+            interface_custom = Chrome(env).get_interface_customization_files()
 
     def get_description(_):
         if env and has_admin:
@@ -822 +822 @@
             'tracker': tracker, 'tracker_args': tracker_args,
             'description': description, 'description_en': description_en}
 
-    Chrome(env).add_jquery_ui(req)
+    if env:
+        Chrome(env).add_jquery_ui(req)
     _send_error(req, sys.exc_info(), status=500, env=env, data=data)
 
 

[Jun Omae]

Replying to Ryan J Ollos:

AttributeError: NullTranslationsBabel instance has no attribute 'isactive'

I'm unsure whether this the correct behavior.

I modified the isactive method of TranslationsProxy to catch the orignal AttributeError:

trac/util/translation.py

@@ -174 +174 @@
 
         @property
         def isactive(self):
-            if self._current.args is not None:
-                get_locale, env_path = self._current.args
-                self._current.args = None
-                self.activate(get_locale(), env_path)
-            # FIXME: The following always returns True: either a translation is
-            # active, or activation has failed.
-            return self._current.translations is not None \
-                   or self._activate_failed
+            try:
+                if self._current.args is not None:
+                    get_locale, env_path = self._current.args
+                    self._current.args = None
+                    self.activate(get_locale(), env_path)
+                # FIXME: The following always returns True: either a translation is
+                # active, or activation has failed.
+                return self._current.translations is not None \
+                       or self._activate_failed
+            except Exception as e:
+                from trac.util.text import exception_to_unicode
+                print(exception_to_unicode(e, traceback=True))
+                raise
 
         # Internal methods
 

I get the original AttributeError after the modification:

Traceback (most recent call last):
  File "/src/tracdev/git/trac/util/translation.py", line 181, in isactive
    self.activate(get_locale(), env_path)
  File "/src/tracdev/git/trac/web/main.py", line 631, in <lambda>
    translation.make_activable(lambda: req.locale, env.path if env else None)
  File "/src/tracdev/git/trac/web/api.py", line 602, in __getattr__
    raise AttributeError(name)
AttributeError: locale

We must prevent the AttributeError to retrieve req.locale. Work around:

trac/web/main.py

@@ -628 +628 @@
     # fixup env.abs_href if `[trac] base_url` was not specified
     if env and not env.abs_href.base:
         env.abs_href = req.abs_href
-    translation.make_activable(lambda: req.locale, env.path if env else None)
+    translation.make_activable(lambda: req.locale if hasattr(req, 'locale')
+                                       else None,
+                               env.path if env else None)
     resp = []
-    dispatcher = RequestDispatcher(env)
-    dispatcher.set_default_callbacks(req)
     try:
         if not env and env_error:
             raise HTTPInternalServerError(env_error)
+        dispatcher = RequestDispatcher(env)
+        dispatcher.set_default_callbacks(req)
         try:
             dispatcher.dispatch(req)
         except RequestDone as req_done:

Another work around:

trac/web/api.py

@@ -582 +582 @@
             'args': lambda req: arg_list_to_args(req.arg_list),
             'languages': Request._parse_languages,
             'incookie': Request._parse_cookies,
-            '_inheaders': Request._parse_headers
+            '_inheaders': Request._parse_headers,
+            'locale': lambda req: None,  # prevent AttributeError
         }
         self.redirect_listeners = []
 

[Ryan J Ollos]

Replying to Jun Omae:

Another work around:

trac/web/api.py

@@ -582 +582 @@
             'args': lambda req: arg_list_to_args(req.arg_list),
             'languages': Request._parse_languages,
             'incookie': Request._parse_cookies,
-            '_inheaders': Request._parse_headers
+            '_inheaders': Request._parse_headers,
+            'locale': lambda req: None,  # prevent AttributeError
         }
         self.redirect_listeners = []
 

Thanks, I missed the exception in isactive. Your fix looks nice.

[Ryan J Ollos]

Replying to Jun Omae:

Yeah. Component class cannot accept None to construct. The same issue exists in send_internal_error.

I'll add those changes. I can reproduce the issue by adding a raise Exception in the code.

trac/web/main.py

@@ -634 +634 @@
     resp = []
     try:
         if env_error:
+            raise Exception("Raw exception with no environment")
             raise HTTPInternalServerError(env_error)
         dispatcher = RequestDispatcher(env)
         dispatcher.set_default_callbacks(req)

I'm having trouble adding a test though. I don't see a way that we could have an Exception that results in send_internal_error without an environment since env_error is raised immediately, leading to handled HTTPException​. We could add coverage for just send_internal_error, but I don't see as much value in that, and that method should probably be internal anyway (_send_internal_error, like _send_user_error).

[Ryan J Ollos]

The browser error message on environment not found has duplicate backslashes on Windows:

Trac Error

TracError: No Trac environment found at c:\users\rjollos\documents\workspace\trac-dev\trac-1.0-stable\trac
[Errno 2] No such file or directory: 'c:\\users\\rjollos\\documents\\workspace\\trac-dev\\trac-1.0-stable\\trac\\VERSION'

The IOError must use repr for filename:

>>> try:
...   open('C:\Path\Does\Not\Exist')
... except Exception as e:
...   print(e)
...   print(e.filename)
...   repr(e)
...   repr(e.filename)
...   print('[Errno %s] %s %r' % (e.errno, e.strerror, e.filename))
...
[Errno 2] No such file or directory: 'C:\\Path\\Does\\Not\\Exist'
C:\Path\Does\Not\Exist
"IOError(2, 'No such file or directory')"
"'C:\\\\Path\\\\Does\\\\Not\\\\Exist'"
[Errno 2] No such file or directory 'C:\\Path\\Does\\Not\\Exist'

I don't know if it's worth trying to fix that up, or if there would be a better way than:

>>> s = ('%s' %e).replace('\\\\', '\\')
>>> print(s)
[Errno 2] No such file or directory: 'C:\Path\Does\Not\Exist'

[Ryan J Ollos]

Proposed changes in log:rjollos.git:t13042_fix_exception_handling.1.

[Jun Omae]

Replying to Ryan J Ollos:

I don't know if it's worth trying to fix that up, or if there would be a better way than:

>>> s = ('%s' %e).replace('\\\\', '\\')
>>> print(s)
[Errno 2] No such file or directory: 'C:\Path\Does\Not\Exist'

• Windows:
  • strerror can be localized error string (ANSI string).
  • filename can be unicode string or ANSI string.
• Linux:
  • strerror can be localized error string (LC_MESSAGES).
  • filename can be unicode string or 8-bit string.

For Linux, I think it is safe to use %r for filename. But we could improve to_unicode() with EnvironmentError for Windows.

trac/util/text.py

@@ -106 +106 @@
         except UnicodeDecodeError:
             return unicode(text, 'latin1')
     elif isinstance(text, Exception):
-        if os.name == 'nt' and isinstance(text, (OSError, IOError)):
+        if os.name == 'nt' and isinstance(text, EnvironmentError):
+            strerror = text.strerror
+            filename = text.filename
+            if strerror and filename:
+                try:
+                    strerror = to_unicode(strerror, 'mbcs')
+                    filename = to_unicode(filename, 'mbcs')
+                except UnicodeError:
+                    pass
+                else:
+                    if isinstance(text, WindowsError):
+                        return u"[Error %s] %s: '%s'" % (text.winerror,
+                                                         strerror, filename)
+                    else:
+                        return u"[Errno %s] %s: '%s'" % (text.errno, strerror,
+                                                         filename)
             # the exception might have a localized error string encoded with
             # ANSI codepage if OSError and IOError on Windows
             try:

Python 2.7.10 (default, May 23 2015, 09:44:00) [MSC v.1500 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import os
>>> from trac.util.text import to_unicode
>>> try:
...   open(r'C:\notfound.txt')
... except Exception as e:
...   print(to_unicode(e))
...
[Errno 2] No such file or directory: 'C:\notfound.txt'
>>> try:
...   open(u'C:\あ.txt')
... except Exception as e:
...   print(to_unicode(e))
...
[Errno 2] No such file or directory: 'C:\あ.txt'
>>> try:
...   os.mkdir('C:\\')
... except Exception as e:
...   print(to_unicode(e))
...
[Error 5] アクセスが拒否されました。: 'C:\'
>>> e.strerror  # "Access denied" in Japanese (cp932)
'\x83A\x83N\x83Z\x83X\x82\xaa\x8b\x91\x94\xdb\x82\xb3\x82\xea\x82\xdc\x82\xb5\x82\xbd\x81B'

[Ryan J Ollos]

I added the comment:21 changes in [6e0696697/rjollos.git]. I made one minor change ([7c94d0ca3/rjollos.git]) to address test failures. The change is patterned after tags/trac-1.3.3/trac/util/text.py@:113#L91​.

======================================================================
ERROR: test_get_authz_file_notfound_raises (trac.versioncontrol.tests.svn_authz.AuthzSourcePolicyTestCase)
ConfigurationError exception is raised if file not found.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\trac\trac\versioncontrol\tests\svn_authz.py", line 288, in test_get_authz_file_notfound_raises
    'BROWSER_VIEW', 'user', None, None)
  File "C:\Python27\lib\unittest\case.py", line 473, in assertRaises
    callableObj(*args, **kwargs)
  File "C:\projects\trac\trac\versioncontrol\svn_authz.py", line 140, in check_permission
    authz, users = self._get_authz_info()
  File "C:\projects\trac\trac\versioncontrol\svn_authz.py", line 217, in _get_authz_info
    "file: %s", exception_to_unicode(e))
  File "C:\projects\trac\trac\util\text.py", line 147, in exception_to_unicode
    message = '%s: %s' % (e.__class__.__name__, to_unicode(e))
  File "C:\projects\trac\trac\util\text.py", line 115, in to_unicode
    filename = unicode(filename, 'mbcs')
TypeError: decoding Unicode is not supported
======================================================================
ERROR: test_get_authz_file_removed_raises (trac.versioncontrol.tests.svn_authz.AuthzSourcePolicyTestCase)
ConfigurationError exception is raised if file is removed.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "C:\projects\trac\trac\versioncontrol\tests\svn_authz.py", line 311, in test_get_authz_file_removed_raises
    'BROWSER_VIEW', 'user', None, None)
  File "C:\Python27\lib\unittest\case.py", line 473, in assertRaises
    callableObj(*args, **kwargs)
  File "C:\projects\trac\trac\versioncontrol\svn_authz.py", line 140, in check_permission
    authz, users = self._get_authz_info()
  File "C:\projects\trac\trac\versioncontrol\svn_authz.py", line 217, in _get_authz_info
    "file: %s", exception_to_unicode(e))
  File "C:\projects\trac\trac\util\text.py", line 147, in exception_to_unicode
    message = '%s: %s' % (e.__class__.__name__, to_unicode(e))
  File "C:\projects\trac\trac\util\text.py", line 115, in to_unicode
    filename = unicode(filename, 'mbcs')
TypeError: decoding Unicode is not supported

[Jun Omae]

Sorry, fixed it and added unit tests in [cc5c36619/jomae.git] (jomae.git@t13042_fix_exception_handling.1). Unit tests pass on ​AppVeyor and ​Travis CI.

[Ryan J Ollos]

Committed minor changes in r16804, r16805.

[Ryan J Ollos]

comment:23 and other changes committed to trunk in r16806.