Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

Bug 352502

Summary:

[breakpoints] Toggle breakpoints type logic can select an invalid breakpoint type.

Product:

[Eclipse Project] Platform

Reporter:

Pawel Piech <pawel.1.piech>

Component:

Debug

Assignee:

Pawel Piech <pawel.1.piech>

Status:

VERIFIED FIXED

QA Contact:

Severity:

normal

Priority:

CC:

daniel_megert, Michael_Rennie, mober.at+eclipse

Version:

3.7

Flags:

Michael_Rennie: review+

Target Milestone:

3.7.1

Hardware:

OS:

Linux

Whiteboard:

Attachments:

Description	Flags
Proposed fix.	none
Test code.	none

Description Pawel Piech

2011-07-19 15:45:59 EDT

If an IToggleBreakpointsTargetFactory is registered without an enablement expression, and this factory returns a valid target ID for getDefaultToggleTarget(), then such toggle factory may cause another toggle factory to be disabled such that no breakpoints can be set.

The problem is in the logic that selects the default toggle breakpoints target.  It polls all factories that are enabled for their default toggle target ID.  This default target ID can be set as the default for a set of available IDs, even if this default ID is not one of the currently active IDs.  

This bug showed up in Wind River product with the addition of PHP breakpoint toggle factory (see bug 352492).

Comment 1 Pawel Piech

2011-07-19 15:47:46 EDT

Created attachment 199942 [details]
Proposed fix.

This fix adjusts the default toggle target selection to only consider the toggle target IDs that were returned by IToggleBreakpointsTargetFactory.getToggleTargets() calls.

Comment 2 Pawel Piech

2011-07-19 15:55:10 EDT

I committed the fix to head and 3.7.1 branch.  Mike please have a look at the changes when you have a chance.

Comment 3 Pawel Piech

2011-07-19 18:16:08 EDT

Created attachment 199949 [details]
Test code.

I used this patch to the PDA example to reproduce the bug.  Must launch with clean workspace to see the bug though.

Comment 4 Dani Megert

2011-07-20 03:24:40 EDT

(In reply to comment #2)
> I committed the fix to head and 3.7.1 branch.  Mike please have a look at the
> changes when you have a chance.

Hi Pawel, next time please wait for the review+ on the bug before putting something into a maintenance branch. Also, depending on the fix (didn't look at this one) it is often good to see how it behaves in HEAD and only if it behaves well, do the backport. Both things are there to make the maintenance stream more robust and to save you work in case the reviewer wants some tweaks to the fix or the fix causes regressions.

Comment 5 Martin Oberhuber

2011-07-20 08:29:19 EDT

CQ:WIND00288111

Comment 6 Michael Rennie

2011-07-21 12:47:37 EDT

+1 for 3.7.1

> Both things are there to make the maintenance stream
> more robust and to save you work in case the reviewer wants some tweaks to the
> fix or the fix causes regressions.

It is also common practice to keep the patches for maintenance branches to be as small as possible, for example this fix contained a lot of Javadoc changes that should not have been back-ported.