Revert breakpoint clearing#3113
Conversation
codehag
left a comment
There was a problem hiding this comment.
hmm i think this might be breaking some stuff. lets chat about it before it gets merged!
| const bp = getBreakpoint(state, location); | ||
|
|
||
| if (sameSource) { | ||
| if (sameSource && !bp) { |
There was a problem hiding this comment.
There are a couple of issues with this fix.
- we are no longer syncing if the debuggee code is changed and the debuggee is reloaded. in other words, the debugger does not reflect the state of the server
- we lose the reference to where the breakpoint is on the server, so if we update the debuggee, and the breakpoint moves, we can no longer delete it.
An alternative to get the debuggee breakpoints working again: do not add the breakpoint if the location is the same. the best solution would be if we had a function in the client to do this, but when i was implementing this, i was nervous about touching the client code. For now we could do the following. This will need some work ... i did it quickly (probably isn't performant / the best solution), but it should work, and we don't need to check if the BP exists in check pending breakpoint. Later, when we set the bp while syncing, it won't matter if it exists in the reducer, since set is idempotent:
diff --git a/src/actions/breakpoints.js b/src/actions/breakpoints.js
index f525de5c..1e2e093b 100644
--- a/src/actions/breakpoints.js
+++ b/src/actions/breakpoints.js
@@ -62,9 +62,10 @@ async function syncClientBreakpoint(
sourceId: generatedSourceId
};
- // early return if breakpoint is disabled with overrides to update
- // the id as expected, without talking to server
- if (pendingBreakpoint.disabled) {
+ const existingClient = client.getBreakpointByLocation(oldGeneratedLocation);
+ // early return if breakpoint is disabled, or exist on the server with
+ // overrides to update the id as expected
+ if (pendingBreakpoint.disabled || existingClient) {
return {
id: generatedSourceId,
actualLocation: { ...pendingBreakpoint.location, id: sourceId },
diff --git a/src/client/firefox/commands.js b/src/client/firefox/commands.js
index f886401a..45308dbc 100644
--- a/src/client/firefox/commands.js
+++ b/src/client/firefox/commands.js
@@ -1,5 +1,6 @@
// @flow
+import _ from "lodash";
import type {
BreakpointId,
BreakpointResult,
@@ -74,6 +75,28 @@ function sourceContents(sourceId: SourceId): Source {
return sourceClient.source();
}
+function getBreakpointByLocation(location: Location) {
+ const values = _.values(bpClients);
+ console.log(values, location);
+ const bpClient = values.find(value => {
+ const { actor, line, column, condition } = value.location;
+ return (
+ location.line === line &&
+ location.sourceId === actor &&
+ location.column === column &&
+ location.condition === condition
+ );
+ });
+
+ if (bpClient) {
+ return {
+ id: bpClient.actor,
+ actualLocation: bpClient.location
+ };
+ }
+ return null;
+}
+
function setBreakpoint(
location: Location,
condition: boolean,
@@ -251,6 +274,7 @@ const clientCommands = {
stepOut,
stepOver,
breakOnNext,
+ getBreakpointByLocation,
sourceContents,
setBreakpoint,
removeBreakpoint,
fd24070 to
01b7152
Compare
Codecov Report
@@ Coverage Diff @@
## master #3113 +/- ##
==========================================
- Coverage 66.86% 66.61% -0.25%
==========================================
Files 76 76
Lines 2698 2714 +16
Branches 544 549 +5
==========================================
+ Hits 1804 1808 +4
- Misses 894 906 +12
Continue to review full report at Codecov.
|
|
Thanks so much @codehag. This was really great work. I hope we can simplify the server in the future, but i will sleep better knowing you can tame the system too. |
This fixes an issue we were seeing with breakpoints being added when they already existed on the server.
This fix is basically reverting the behavior we had before.