Bug 346116

Comment 1 Michael Rennie 2011-05-17 12:21:40 EDT

Quickly tested this on 3.7 and it works as expected:

1. import the test case and use the steps given
2. after stepping in, the source from the archive is shown [OK]
3. I can inspect 'this' without issue [OK].

I will load up my 3.6.2 test workspace and try there...

Comment 2 Michael Rennie 2011-05-17 12:32:28 EDT

The same steps also work for me using:

Version: 3.6.2
Build id: M20110210-1200

Angel, since the .java file source is available in the archive why are you expecting to see the .class file? Note: showing the class file or the Java file from the archive does not affect the inspect action.

Comment 3 Angel Vera 2011-05-17 12:57:57 EDT

When I tried inspecting 'this' in the .java file it didn't worked for me. I got the problem that I documented in the description.  I tried this on Win32, I wouldn't expect that the platform makes a difference, but just in case. 

Looking through the code the problem seemed to be that in previous versions JDT org.eclipse.jdt.internal.launching.JavaSourceLookupUtil.translate(IRuntimeClasspathEntry[]) would return a PackageFragmentRootSourceContainer because a root exists for that project, but in 362 a root doesn't exists so it creates a ExternalArchiveSourceContainer.

Comment 4 Angel Vera 2011-05-17 12:58:53 EDT

I am using 362 M20110210-1200

Comment 5 Michael Rennie 2011-05-17 15:06:25 EDT

(In reply to comment #2)
> The same steps also work for me using:
> 
> Version: 3.6.2
> Build id: M20110210-1200
> 
> Angel, since the .java file source is available in the archive why are you
> expecting to see the .class file? Note: showing the class file or the Java file
> from the archive does not affect the inspect action.

The critical missing step to reproduce is that you have to select 'this' in the editor and try to inspect it.

Comment 6 Michael Rennie 2011-05-17 16:20:46 EDT

Debugging this I found that part of the fix for bug 297039 moved a call to RuntimeClasspathEntry#getPath() out of a double-nested for-loop and in doing so changed the path that would be compared:

was:

if (!root.isExternal() && root.getPath().equals(entry.getPath())) {
..
}

changed to:

if (root.isExternal() && root.getPath().equals(entryPath)) {
..
}

where entryPath is defined as: Path entryPath = new Path(entry.getLocation());

I will have to change the definition of entryPath to be based on entry.getPath() and see if it affects the fix for bug 297039

Comment 7 Angel Vera 2011-05-19 10:29:31 EDT

(In reply to comment #6)
> I will have to change the definition of entryPath to be based on
> entry.getPath() and see if it affects the fix for bug 297039

Michael, 

What is the status on this one? Did you get a chance to try out things?

Comment 8 Michael Rennie 2011-05-19 10:47:47 EDT

(In reply to comment #7)
> (In reply to comment #6)
> > I will have to change the definition of entryPath to be based on
> > entry.getPath() and see if it affects the fix for bug 297039
> 
> Michael, 
> 
> What is the status on this one? Did you get a chance to try out things?

I did, I reverted the logic to compare the correct path elements and removed the check for if the class path entry is external or not, and it seems to be working - much more smoke testing is required and I want to add some JUnit tests for this scenario.

I'll post a patch

Comment 9 Michael Rennie 2011-05-19 10:49:02 EDT

Created attachment 196120 [details]
testing patch

Here is a patch of the code I have been playing with.

Comment 10 Angel Vera 2011-05-23 21:38:44 EDT

I ran through my scenario and it seems to working OK, I am certainly concern about the risk of regression as per comment# 6. I will leave it up to you for your testing to confirm that this is the right fix and that it doesn't break other scenarios or that introduces regressions.

Comment 11 Michael Rennie 2011-05-26 13:13:07 EDT

Created attachment 196680 [details]
proposed fix

Here is a complete patch with new tests.

I am still on the fence about inclusion in RC3 / RC4 for fear of regression in another edge case, although smoke testing did not uncover any other issues.

Comment 12 Michael Rennie 2011-05-26 13:15:06 EDT

Created attachment 196682 [details]
test projects

Here are the test projects that must be un-zipped in the org.eclipse.jdt.debug.tests/testresources/ folder. Using these projects the new tests set up a scenario as described in this bug, and perform source look up, classpath entry translation and stepping / inspecting.

Comment 13 Michael Rennie 2011-05-26 13:21:05 EDT

Curtis, Dani, Markus, please review the patch

Comment 14 Markus Keller 2011-05-27 14:24:21 EDT

Code-wise, the proposed fix is really the same as the testing patch. It looks good, and I verified that it works for internal and external JARs, and that it doesn't break bug 297039.

However, this problem has not been reported so far, although bug 297039 has been in since 3.5.2. Angel, do you really think it's critical for 3.7, or could this wait till 3.7.1?

Comment 15 Dani Megert 2011-05-30 10:42:25 EDT

The fix looks OK to me but I can't guarantee that there are no unwanted side effects. Given we're in RC4 and the bug is in since 3.5.2 I -1 this fix for 3.7 RC4.

Comment 16 Michael Rennie 2011-05-30 10:45:23 EDT

(In reply to comment #15)
> Given we're in RC4 and the bug is in since 3.5.2 I -1 this fix for 3.7
> RC4.

I'm fine with that. We can target it for 3.7.1.

Comment 17 Curtis Windatt 2011-05-30 16:59:07 EDT

The fix works for me, but since we are moving this to 3.7.1 I will hold off on my +1.

Comment 18 Michael Rennie 2011-06-29 09:50:09 EDT

Applied patch + tests to HEAD

Comment 19 Michael Rennie 2011-06-29 10:36:37 EDT

Created attachment 198834 [details]
patch for 3.7.1

Comment 20 Michael Rennie 2011-06-29 10:58:34 EDT

committed fix to 3.7.1