Comment 1 Markus Keller 2012-01-16 08:47:53 EST

This happens even if the resource leak problems are set to "ignore" (out-of-the-box setting).

Comment 2 Stephan Herrmann 2012-01-16 08:52:39 EST

I'll take care of that.

Comment 3 Stephan Herrmann 2012-01-16 09:47:46 EST

Thanks for the clear steps.

Here's a minimal test case:

import java.io.*;
import java.util.zip.*;
public class X {
  void doit() throws IOException {
    InputStream in = new FileInputStream("somefile");
    in = new BufferedInputStream(new InflaterInputStream(in, inflater(), 8192), 8192); // nested self-wrapping
    process(in);
  }
  Inflater inflater() { return null; }
  void process(InputStream is) { }
}

The problem is in the nested self-wrapping.

Comment 4 Stephan Herrmann 2012-01-16 09:53:37 EST

Created attachment 209559 [details]
avoiding the recursive structure

This patch addresses the immediate severe issue:

When connecting tracking variables with inners we already check for
direct cycles. The patch adds a loop that includes indirect cycles
in the check. Given that this is the only location that assigns non-null
to FTV#innerTracker this should be safe for avoiding the endless loop.

The test currently flags a warning against the inner resource,
which isn't optimal. I'll give it a quick check if that can be fixed
in the same patch.

Comment 5 Stephan Herrmann 2012-01-16 10:12:01 EST

I have a patch that also fixes the non-optimal warning mentioned in comment 4. I do that by propagating the tracking variable for the LHS 'in' deeper into the AllocationExpression (method FTV#preConnectTrackerAcrossAssignment).
Now the nested wrapper is correctly detected as a wrapper for 'in', which couldn't succeed before, because the inner "new InflaterInputStream(in ...)" didn't know that we are going to assign to 'in'.

Interestingly, with both changes in place the fix from comment 4 is no longer triggered, because now wrappers are linked as expected.
Anyway I'll leave both changes in as extra safety against the endless loop.

I'll release the combined fix as soon as all JDT/Core tests and field experiments with Eclipse SDK and JGit signal green.

Comment 6 Stephan Herrmann 2012-01-16 11:16:17 EST

Release for 3.8 M5 via commit 2dc8c8168c71292aca0a9b4cb34971871475b18a

Comment 7 Srikanth Sankaran 2012-01-16 19:59:15 EST

If the major chunks of the analysis code lend themselves
to being protected with a if option on style, we could
consider that. Unfortunately, currently, retrieving compiler
options can be very expensive as things stand and caching
during construction of an abstraction may not always make
sense as we have seen in other instance.

Stephan, please take a quick look-see to if we can opt for
defensive coding in this feature.

Comment 8 Srikanth Sankaran 2012-01-17 04:54:37 EST

(In reply to comment #7)
> If the major chunks of the analysis code lend themselves
> to being protected with a if option on style, 

[...]


> Stephan, please take a quick look-see to if we can opt for
> defensive coding in this feature.

Ideally, all decorations, propagation of attributes, analysis
and diagnostics should be keyed off the option to provide for
an escape hatch. Particularly a user who hasn't turned on the
option should see no impact. We need to devise some ways of
ensuring that the cost of retrieving the compiler option is not
making us resort to less defensive coding style which also has
a performance angle to it.

Comment 9 Satyam Kandula 2012-01-23 10:21:20 EST

Verified for 3.8M5 using build  I20120122-2000