Details

    • Type: Bug Bug
    • Status: Resolved Resolved
    • Priority: Minor Minor
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: JRuby 1.7.0.RC1
    • Component/s: Parser
    • Labels:
      None
    • Number of attachments :
      1

      Description

      Hi there,

      I've been trying out the great new coverage changes made in JRUBY-6106. I have a Sinatra app for which simplecov is now working perfectly, but I've hit an interesting problem when trying the same thing on a Rails app.

      I'm building against the current git master
      (4e471f489ee6e6aa5d74d64893d709c85a312ee1) with:

        ant clean && ant jar-jruby-complete
      

      When I use the resulting jar to run my Rails app with simplecov enabled, I get exceptions like this:

        CoverageModule.java:40:in `result': java.lang.NullPointerException
                from CoverageModule$INVOKER$s$0$0$result.gen:-1:in `call'
                from CachingCallSite.java:292:in `cacheAndCall'
                from CachingCallSite.java:135:in `call'
                from CallNoArgNode.java:63:in `interpret'
                from CallOneArgNode.java:57:in `interpret'
      

      After a bit of debugging I think I've tracked the problem down to what might be two different issues.

      The first issue comes about when calling require on an empty file (or a file consisting of nothing but comments). This seems to cause a null to be added to the coverage data, which blows up when generating results. I can reproduce that with:

        $ GEM_HOME=$PWD/test_gems java -jar lib/jruby-complete.jar -S gem install simplecov
      
        $ echo > empty.rb
      
        $ cat | GEM_HOME=$PWD/test_gems java -jar lib/jruby-complete.jar -I. --debug --1.9 -
      require "rubygems"
      require 'simplecov'
      SimpleCov.start
      
      require "empty.rb"
      
      ^D
      

      For me, that blows up with an exception like the one shown above.

      I'm finding the other issue a bit harder to nail down. The ParserConfiguration.coverLine(int i) method takes a line number and marks it off as being covered. But when running my Rails app under simplecov, I see that 'i' isn't always positive--I see numbers ranging from -9 and up, and the negative number causes a RuntimeException when it tries to access an invalid array offset.

      I haven't made much headway in figuring out why the parser would report a negative line number--it only seems to happen in my app when parsing .erb files, but I can't reproduce the problem when loading the same .erb templates from the command line. I thought I'd just check if line numbers sometimes being negative was expected before diving in further.

      I've attached a patch that sorts things out for my local instance, but realise I'm probably just masking the symptoms. I thought it might help to show the spots I'm talking about, though.

      Thanks for all your great work, and please let me know if I can do any debugging from this end.

      Cheers,

      Mark

        Activity

        Hide
        Charles Oliver Nutter added a comment -

        Great investigation!

        I had not considered the case of a file with no lines; it should indeed initialize to at least an empty array, so future array dereferences don't cause NPE.

        The other one is definitely more confusing, for the reasons you state: I wouldn't expect the parser to produce negative line numbers.

        I'll have a look at your patch. Without knowing why the parser produces negative line numbers, masking the issue might be the simplest fix. It's possible that bad line numbers are being fed to ERB processing somehow, or those negative line numbers might actually be expected. It may be that we should not be doing coverage processing at all for eval-related parses that don't have an explicit file, but I (or someone) would have to research to see if that's the case.

        Show
        Charles Oliver Nutter added a comment - Great investigation! I had not considered the case of a file with no lines; it should indeed initialize to at least an empty array, so future array dereferences don't cause NPE. The other one is definitely more confusing, for the reasons you state: I wouldn't expect the parser to produce negative line numbers. I'll have a look at your patch. Without knowing why the parser produces negative line numbers, masking the issue might be the simplest fix. It's possible that bad line numbers are being fed to ERB processing somehow, or those negative line numbers might actually be expected. It may be that we should not be doing coverage processing at all for eval-related parses that don't have an explicit file, but I (or someone) would have to research to see if that's the case.
        Hide
        Charles Oliver Nutter added a comment -

        Here's a similar patch that ignores negative line numbers and always adds at least an empty Integer[] for coverage lines. I'm not certain this is the right approach, but it would fix the issues you saw.

        https://gist.github.com/3507988

        Let me know what you think of this.

        WRT to the empty file case, I think always inserting an empty array is right; MRI appears to add an array of length == file line count whether there's > 0 lines or not.

        The other case...I'm not sure.

        Show
        Charles Oliver Nutter added a comment - Here's a similar patch that ignores negative line numbers and always adds at least an empty Integer[] for coverage lines. I'm not certain this is the right approach, but it would fix the issues you saw. https://gist.github.com/3507988 Let me know what you think of this. WRT to the empty file case, I think always inserting an empty array is right; MRI appears to add an array of length == file line count whether there's > 0 lines or not. The other case...I'm not sure.
        Hide
        Mark Triggs added a comment -

        Thanks Charles. I've tested your patch and it works for my app, and I agree that looks like the way to go.

        I've been wandering through the parser code, but haven't made a huge amount of progress. It seems that the negative line numbers are stemming from ERB calling eval() on the code it produces from .erb files, but I haven't made it much further than that. I'll have another look tomorrow and see if I can dig anything else up.

        Thanks again for the quick fix

        Mark

        Show
        Mark Triggs added a comment - Thanks Charles. I've tested your patch and it works for my app, and I agree that looks like the way to go. I've been wandering through the parser code, but haven't made a huge amount of progress. It seems that the negative line numbers are stemming from ERB calling eval() on the code it produces from .erb files, but I haven't made it much further than that. I'll have another look tomorrow and see if I can dig anything else up. Thanks again for the quick fix Mark
        Hide
        Mark Triggs added a comment -

        Obviously by "tomorrow" I meant "right now"... starting from the parser and working backwards, I think the negative line numbers can be explained by this bit from tilt-1.3.3/lib/tilt/template.rb:

            # JRuby doesn't allow Object#instance_eval to yield to the block it's
            # closed over. This is by design and (ostensibly) something that will
            # change in MRI, though no current MRI version tested (1.8.6 - 1.9.2)
            # exhibits the behavior. More info here:
            #
            # http://jira.codehaus.org/browse/JRUBY-2599
            #
            # We redefine evaluate_source to work around this issues.
            if defined?(RUBY_ENGINE) && RUBY_ENGINE == 'jruby'
              undef evaluate_source
              def evaluate_source(scope, locals, &block)
                source, offset = precompiled(locals)
                file, lineno = eval_file, (line - offset)
        
                scope.instance_eval { Kernel::eval(source, binding, file, lineno) }
              end
            end
        

        That 'lineno' is -5 when it fails on my local instance. I'll do some digging around tomorrow (seriously) to see what's going on there, but at least it suggests JRuby can just ignore the bogus line numbers.

        Show
        Mark Triggs added a comment - Obviously by "tomorrow" I meant "right now"... starting from the parser and working backwards, I think the negative line numbers can be explained by this bit from tilt-1.3.3/lib/tilt/template.rb: # JRuby doesn't allow Object#instance_eval to yield to the block it's # closed over. This is by design and (ostensibly) something that will # change in MRI, though no current MRI version tested (1.8.6 - 1.9.2) # exhibits the behavior. More info here: # # http://jira.codehaus.org/browse/JRUBY-2599 # # We redefine evaluate_source to work around this issues. if defined?(RUBY_ENGINE) && RUBY_ENGINE == 'jruby' undef evaluate_source def evaluate_source(scope, locals, &block) source, offset = precompiled(locals) file, lineno = eval_file, (line - offset) scope.instance_eval { Kernel::eval(source, binding, file, lineno) } end end That 'lineno' is -5 when it fails on my local instance. I'll do some digging around tomorrow (seriously) to see what's going on there, but at least it suggests JRuby can just ignore the bogus line numbers.
        Hide
        Charles Oliver Nutter added a comment -

        The bug referenced in that tilt comment appears to have been fixed some time ago. I wonder if it was just left in place because nobody knew it had been fixed? I will file an issue for them to reevaluate the hack.

        In any case, I'll go with my patch for now. Thanks for the extra legwork!

        Show
        Charles Oliver Nutter added a comment - The bug referenced in that tilt comment appears to have been fixed some time ago. I wonder if it was just left in place because nobody knew it had been fixed? I will file an issue for them to reevaluate the hack. In any case, I'll go with my patch for now. Thanks for the extra legwork!
        Hide
        Charles Oliver Nutter added a comment -

        FYI, it appears the JRuby hack was removed from tilt master, at least:

        commit 570f1408d741674687df7e3ffd41d58d3e7a3a69
        Author: Ryan Tomayko <rtomayko@gmail.com>
        Date:   Mon Sep 19 04:12:55 2011 -0700
        
            remove evaluate_source method completely
            
            This should no longer be used anywhere. It's nice to remove the
            JRuby hack as well.
        
        Show
        Charles Oliver Nutter added a comment - FYI, it appears the JRuby hack was removed from tilt master, at least: commit 570f1408d741674687df7e3ffd41d58d3e7a3a69 Author: Ryan Tomayko <rtomayko@gmail.com> Date: Mon Sep 19 04:12:55 2011 -0700 remove evaluate_source method completely This should no longer be used anywhere. It's nice to remove the JRuby hack as well.
        Hide
        Charles Oliver Nutter added a comment -
        commit 93d09db0c216be917aaaf1def7f5576b3c9bc2c3
        Author: Charles Oliver Nutter <headius@headius.com>
        Date:   Wed Aug 29 10:30:54 2012 -0500
        
            Fix JRUBY-6868
            
            Exceptions thrown when running with Coverage enabled
            
            * Empty files should add an empty Integer[] array to the coverage
            data "lines" table.
            
            * Negative line numbers should be ignored.
        
        :100644 100644 8833289... 7cc6a81... M	src/org/jruby/ext/coverage/CoverageData.java
        :100644 100644 3db299c... 991c94f... M	src/org/jruby/parser/ParserConfiguration.java
        
        Show
        Charles Oliver Nutter added a comment - commit 93d09db0c216be917aaaf1def7f5576b3c9bc2c3 Author: Charles Oliver Nutter <headius@headius.com> Date: Wed Aug 29 10:30:54 2012 -0500 Fix JRUBY-6868 Exceptions thrown when running with Coverage enabled * Empty files should add an empty Integer[] array to the coverage data "lines" table. * Negative line numbers should be ignored. :100644 100644 8833289... 7cc6a81... M src/org/jruby/ext/coverage/CoverageData.java :100644 100644 3db299c... 991c94f... M src/org/jruby/parser/ParserConfiguration.java
        Hide
        Mark Triggs added a comment -

        Aha! Brilliant. Many thanks for all the work

        Show
        Mark Triggs added a comment - Aha! Brilliant. Many thanks for all the work

          People

          • Assignee:
            Charles Oliver Nutter
            Reporter:
            Mark Triggs
          • Votes:
            0 Vote for this issue
            Watchers:
            3 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved: