Bugzilla – Full Text Bug Listing
| Summary: | Wrong toolbar debug button state with two windows | ||||||
|---|---|---|---|---|---|---|---|
| Product: | [Eclipse Project] Platform | Reporter: | Simeon Andreev <simeon.danailov.andreev> | ||||
| Component: | Debug | Assignee: | Simeon Andreev <simeon.danailov.andreev> | ||||
| Status: | VERIFIED FIXED | QA Contact: | |||||
| Severity: | normal | ||||||
| Priority: | P3 | CC: | daniel_megert, loskutov, paul-eclipse, sarika.sinha | ||||
| Version: | 4.12 | ||||||
| Target Milestone: | 4.15+ | ||||||
| Hardware: | PC | ||||||
| OS: | Linux | ||||||
| See Also: |
https://bugs.eclipse.org/bugs/show_bug.cgi?id=560274 https://git.eclipse.org/r/158082 https://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?id=aef281e1db65fd7b8e7525a0ac39de5ed54cb12b https://git.eclipse.org/r/159672 https://git.eclipse.org/r/#/c/159671/ https://bugs.eclipse.org/bugs/show_bug.cgi?id=563154 |
||||||
| Whiteboard: | |||||||
| Attachments: |
|
||||||
I'm seeing only one set of calls to SWT ToolItem.setEnabled(), but there are 2 windows. I'm not sure how that is supposed to work. Also note that the debug commands have correct enabled states. In a window where the toolbar resume button is disabled, I can still use F8 to resume (as the command works, it must also be enabled). I think I no longer see the problem with this change:
diff --git a/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/commands/actions/DebugCommandService.java b/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/commands/actions/DebugCommandService.java
index 2a8885796..4ce270efd 100644
--- a/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/commands/actions/DebugCommandService.java
+++ b/org.eclipse.debug.ui/ui/org/eclipse/debug/internal/ui/commands/actions/DebugCommandService.java
@@ -19,7 +19,6 @@ import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
-import org.eclipse.core.runtime.jobs.Job;
import org.eclipse.debug.core.DebugPlugin;
import org.eclipse.debug.core.commands.IDebugCommandHandler;
import org.eclipse.debug.ui.DebugUITools;
@@ -122,7 +121,7 @@ public class DebugCommandService implements IDebugContextListener {
*/
public void postUpdateCommand(Class<?> commandType, IEnabledTarget action) {
synchronized (fCommandUpdates) {
- Job.getJobManager().cancel(commandType);
+ // Job.getJobManager().cancel(commandType);
List<IEnabledTarget> actions = fCommandUpdates.get(commandType);
if (actions == null) {
actions = new ArrayList<>();
After the evaluation triggered due to mouse-over on "args", I observe that an update job "AbstractDebugCommand.UpdateJob" is scheduled multiple times (I'm guessing at least 2x times due to having 2 different windows, not sure why in some cases its more than 2 times). However only some of those actually run. I assume this is due to the cancellation above. Its done with with the handler classes, e.g. in "org.eclipse.debug.internal.core.commands.SuspendCommand":
@Override
protected Object getEnabledStateJobFamily(IDebugCommandRequest request) {
return ISuspendHandler.class;
}
In bug 560274, Paul suggested a queue and a single job. We might want this to fix this bug; the cancel ignores the contents in "org.eclipse.debug.internal.ui.commands.actions.UpdateActionsRequest.fActions", which I think is what makes the update work with 2 windows. I could be wrong, since the objects in "fActions" are some lambdas, no idea what is actually behind the lambdas.
New Gerrit change created: https://git.eclipse.org/r/158082 (In reply to Eclipse Genie from comment #3) > New Gerrit change created: https://git.eclipse.org/r/158082 This is a "minimal effort" suggestion. The cancellation in DebugCommandService.postUpdateCommand() is there to avoid unnecessary work. I see the following options: 1. Make the cancellation more fine-grained, by providing better job families. No idea how. 2. Rework AbstractDebugCommand.UpdateJob, e.g. with actions-to-update per command queue. Considering that both family and rule for this job are APIs, probably not the best approach. 3. Stop the cancellation with 2 or more windows, when its broken. This is in the review. Let me know what you think. (In reply to Simeon Andreev from comment #4) > The cancellation in DebugCommandService.postUpdateCommand() is > there to avoid unnecessary work. Was added bug 245314 to fix a performance problems in a (my impression) quite custom debug view modification. (just for the record) With the limitation on multiple windows I think your change is ok. Even for the rare case of multiple windows and more than usual action updates the worst consequence should be a slowed down application but no sever problems. (In reply to Simeon Andreev from comment #4) > Considering that both family and rule for this job are APIs, > probably not the best approach. I don't see a problem with that at first glance. The scheduling rule guarantee the order of those updates where the rule applies. There is no guarantee for parallelism. If you run for example all updates strictly sequential it would make the rules useless but is not an API break in my opinion. However dependent on how a specific update request is processed a single threaded update processing might not be the best option. Queue per command could be problematic with some exclusion rules (not sure yet). A queue per rule might work but again I'm not sure yet. Say if you want to give option 2 a try. If not I might try it eventually. I assume you are already fine after this and your other change. (unless you have yet another ugly concurrency bug in the queue) A note to option 1: it might work to "reimplement" the cancel method in postUpdateCommand and additional filter on the DebugService instance (which is window bound) which is somehow passed into the UpdateJob. I wouldn't favor this option as I disliked the throwaway job pattern in the first place. (In reply to Paul Pazderski from comment #5) > Say if you want to give option 2 a try. If not I might try it eventually. I > assume you are already fine after this and your other change. (unless you > have yet another ugly concurrency bug in the queue) If the current patch suggestion is accepted, we'll probably stick with it. We do have other debug related problems in the backlog; if they again lead to AbstractDebugCommand.UpdateJob, we might consider bigger changes. Gerrit change https://git.eclipse.org/r/158082 was merged to [master]. Commit: http://git.eclipse.org/c/platform/eclipse.platform.debug.git/commit/?id=aef281e1db65fd7b8e7525a0ac39de5ed54cb12b @Simeon: thanks, but please don't forget to validate with latest build (and set status to validated). New Gerrit change created: https://git.eclipse.org/r/159672 @Dani, We will need 4.15+ Target milestone for back porting. (In reply to Andrey Loskutov from comment #8) > @Simeon: thanks, but please don't forget to validate with latest build (and > set status to validated). @Simeon, Can you verify in the I build? So that the change can be back ported to 4.15+. (In reply to Sarika Sinha from comment #11) > @Simeon, > Can you verify in the I build? > So that the change can be back ported to 4.15+. I'll check tomorrow. Was on vacation this week. Following the steps from the description, I can no longer reproduce the problem with: Eclipse SDK Version: 2020-06 (4.16) Build id: I20200321-0440 I.e. the problem is fixed. |
Created attachment 281876 [details] Video showing the problem with the reproduction steps from the description. To reproduce: 1. Debug Eclipse with: 1.1. Enabled breakpoint at "fEvaluationRunnable = null;" in "org.eclipse.jdt.internal.debug.core.model.JDIThread.runEvaluation(IEvaluationRunnable, IProgressMonitor, int, boolean)" (currently line 585). 1.2. Disabled breakpoint at first line of "org.eclipse.debug.core.commands.AbstractDebugCommand.UpdateJob.run(IProgressMonitor)". 2. Open a 2nd window in the debuggee Eclipse, open the Debug view in the 2nd window. 3. In the debuggee Eclipse, debug a snippet with a breakpoint at the only line in main(): public class Test { public static void main(String[] args) { System.out.println(); // set a breakpoint here } } 4. Hover over "args" to trigger an evaluation. 5. Enable the breakpoint from 1.2. (the breakpoint at "AbstractDebugCommand.UpdateJob.run()"). 6. Resume until no more breakpoints are hit. 7. Observe that the main toolbar debug commands for steping in, stepping over, resume, etc. are not always enabled. They should be however. Sometimes this is observed in the first window, sometimes in the 2nd window. See the attached video "wrong_toolbar_debug_buttons_state_in_2nd_window.mp4". We see this with 4.12 but also with latest platform + JDT.