From e05714fdbc4d6a767f207b08a94ba3ebf147213e Mon Sep 17 00:00:00 2001
From: Genadi Samokovarov <gsamokovarov@gmail.com>
Date: Sun, 16 Nov 2014 17:17:06 +0200
Subject: Don't let #{application,framework,full}_trace be nil

Those three can be nil when exception backtrace is nil. This happens and
that forced a couple of nil guards in the code. I'm proposing to make
those always return an array, even on nil backtrace.
---
 .../middleware/exception_wrapper.rb                | 24 +++++++-------
 .../middleware/templates/rescues/_source.erb       | 38 ++++++++++------------
 actionpack/test/dispatch/exception_wrapper_test.rb | 31 ++++++++++++++++++
 3 files changed, 62 insertions(+), 31 deletions(-)

(limited to 'actionpack')

diff --git a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
index e0140b0692..b8381aba70 100644
--- a/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
+++ b/actionpack/lib/action_dispatch/middleware/exception_wrapper.rb
@@ -62,14 +62,12 @@ module ActionDispatch
       framework_trace_with_ids = []
       full_trace_with_ids = []
 
-      if full_trace
-        full_trace.each_with_index do |trace, idx|
-          trace_with_id = { id: idx, trace: trace }
+      full_trace.each_with_index do |trace, idx|
+        trace_with_id = { id: idx, trace: trace }
 
-          appplication_trace_with_ids << trace_with_id if application_trace.include?(trace)
-          framework_trace_with_ids << trace_with_id if framework_trace.include?(trace)
-          full_trace_with_ids << trace_with_id
-        end
+        appplication_trace_with_ids << trace_with_id if application_trace.include?(trace)
+        framework_trace_with_ids << trace_with_id if framework_trace.include?(trace)
+        full_trace_with_ids << trace_with_id
       end
 
       {
@@ -84,7 +82,7 @@ module ActionDispatch
     end
 
     def source_extract
-      exception.backtrace.map do |trace|
+      backtrace.map do |trace|
         file, line = trace.split(":")
         line_number = line.to_i
         {
@@ -92,11 +90,15 @@ module ActionDispatch
           file: file,
           line_number: line_number
         }
-      end if exception.backtrace
+      end
     end
 
     private
 
+    def backtrace
+      Array(@exception.backtrace)
+    end
+
     def original_exception(exception)
       if registered_original_exception?(exception)
         exception.original_exception
@@ -111,9 +113,9 @@ module ActionDispatch
 
     def clean_backtrace(*args)
       if backtrace_cleaner
-        backtrace_cleaner.clean(@exception.backtrace, *args)
+        backtrace_cleaner.clean(backtrace, *args)
       else
-        @exception.backtrace
+        backtrace
       end
     end
 
diff --git a/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.erb b/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.erb
index eabac3a9d2..101cea13f9 100644
--- a/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.erb
+++ b/actionpack/lib/action_dispatch/middleware/templates/rescues/_source.erb
@@ -1,29 +1,27 @@
-<% if @source_extract %>
-  <% @source_extract.each_with_index do |extract_source, index| %>
-    <% if extract_source[:code] %>
-      <div class="source <%="hidden" if @show_source_idx != index%>" id="frame-source-<%=index%>">
-        <div class="info">
-          Extracted source (around line <strong>#<%= extract_source[:line_number] %></strong>):
-        </div>
-        <div class="data">
-          <table cellpadding="0" cellspacing="0" class="lines">
-            <tr>
-              <td>
-                <pre class="line_numbers">
-                  <% extract_source[:code].each_key do |line_number| %>
+<% @source_extract.each_with_index do |extract_source, index| %>
+  <% if extract_source[:code] %>
+    <div class="source <%="hidden" if @show_source_idx != index%>" id="frame-source-<%=index%>">
+      <div class="info">
+        Extracted source (around line <strong>#<%= extract_source[:line_number] %></strong>):
+      </div>
+      <div class="data">
+        <table cellpadding="0" cellspacing="0" class="lines">
+          <tr>
+            <td>
+              <pre class="line_numbers">
+                <% extract_source[:code].each_key do |line_number| %>
 <span><%= line_number -%></span>
-                  <% end %>
-                </pre>
-              </td>
+                <% end %>
+              </pre>
+            </td>
 <td width="100%">
 <pre>
 <% extract_source[:code].each do |line, source| -%><div class="line<%= " active" if line == extract_source[:line_number] -%>"><%= source -%></div><% end -%>
 </pre>
 </td>
-            </tr>
-          </table>
-        </div>
+          </tr>
+        </table>
       </div>
-    <% end %>
+    </div>
   <% end %>
 <% end %>
diff --git a/actionpack/test/dispatch/exception_wrapper_test.rb b/actionpack/test/dispatch/exception_wrapper_test.rb
index 3ddaa7294b..57292d3191 100644
--- a/actionpack/test/dispatch/exception_wrapper_test.rb
+++ b/actionpack/test/dispatch/exception_wrapper_test.rb
@@ -10,6 +10,12 @@ module ActionDispatch
       end
     end
 
+    class BadlyDefinedError < StandardError
+      def backtrace
+        nil
+      end
+    end
+
     setup do
       Rails.stubs(:root).returns(Pathname.new('.'))
 
@@ -28,6 +34,7 @@ module ActionDispatch
       assert_equal [ code: 'foo', file: 'lib/file.rb', line_number: 42 ], wrapper.source_extract
     end
 
+
     test '#application_trace returns traces only from the application' do
       exception = TestError.new(caller.prepend("lib/file.rb:42:in `index'"))
       wrapper = ExceptionWrapper.new(@environment, exception)
@@ -35,6 +42,14 @@ module ActionDispatch
       assert_equal [ "lib/file.rb:42:in `index'" ], wrapper.application_trace
     end
 
+    test '#application_trace cannot be nil' do
+      nil_backtrace_wrapper = ExceptionWrapper.new(@environment, BadlyDefinedError.new)
+      nil_cleaner_wrapper = ExceptionWrapper.new({}, BadlyDefinedError.new)
+
+      assert_equal [], nil_backtrace_wrapper.application_trace
+      assert_equal [], nil_cleaner_wrapper.application_trace
+    end
+
     test '#framework_trace returns traces outside the application' do
       exception = TestError.new(caller.prepend("lib/file.rb:42:in `index'"))
       wrapper = ExceptionWrapper.new(@environment, exception)
@@ -42,6 +57,14 @@ module ActionDispatch
       assert_equal caller, wrapper.framework_trace
     end
 
+    test '#framework_trace cannot be nil' do
+      nil_backtrace_wrapper = ExceptionWrapper.new(@environment, BadlyDefinedError.new)
+      nil_cleaner_wrapper = ExceptionWrapper.new({}, BadlyDefinedError.new)
+
+      assert_equal [], nil_backtrace_wrapper.framework_trace
+      assert_equal [], nil_cleaner_wrapper.framework_trace
+    end
+
     test '#full_trace returns application and framework traces' do
       exception = TestError.new(caller.prepend("lib/file.rb:42:in `index'"))
       wrapper = ExceptionWrapper.new(@environment, exception)
@@ -49,6 +72,14 @@ module ActionDispatch
       assert_equal exception.backtrace, wrapper.full_trace
     end
 
+    test '#full_trace cannot be nil' do
+      nil_backtrace_wrapper = ExceptionWrapper.new(@environment, BadlyDefinedError.new)
+      nil_cleaner_wrapper = ExceptionWrapper.new({}, BadlyDefinedError.new)
+
+      assert_equal [], nil_backtrace_wrapper.full_trace
+      assert_equal [], nil_cleaner_wrapper.full_trace
+    end
+
     test '#traces returns every trace by category enumerated with an index' do
       exception = TestError.new("lib/file.rb:42:in `index'", "/gems/rack.rb:43:in `index'")
       wrapper = ExceptionWrapper.new(@environment, exception)
-- 
cgit v1.2.3