[Breakpoints] + [SourceMaps] breakpoints are out of sync with the server on reload by codehag · Pull Request #3039 · firefox-devtools/debugger

codehag · 2017-05-30T14:54:52Z

Associated Issue: #2904

WIP

codecov · 2017-05-30T17:42:02Z

Codecov Report

Merging #3039 into master will increase coverage by 0.41%.
The diff coverage is 88.23%.

@@            Coverage Diff             @@
##           master    #3039      +/-   ##
==========================================
+ Coverage   66.15%   66.56%   +0.41%     
==========================================
  Files          74       75       +1     
  Lines        2603     2668      +65     
  Branches      527      541      +14     
==========================================
+ Hits         1722     1776      +54     
- Misses        881      892      +11

Impacted Files	Coverage Δ
src/utils/utils.js	`18.18% <ø> (-6.82%)`	⬇️
src/client/firefox/commands.js	`19.58% <0%> (-0.42%)`	⬇️
src/actions/sources.js	`51.26% <100%> (-0.61%)`	⬇️
src/utils/prefs.js	`92.3% <60%> (-7.7%)`	⬇️
src/utils/breakpoint/index.js	`91.66% <91.66%> (ø)`
src/reducers/breakpoints.js	`89.03% <92.15%> (-1.55%)`	⬇️
src/actions/breakpoints.js	`88.04% <96.77%> (+2.12%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9d582f4...219286c. Read the comment docs.

jasonLaster · 2017-05-30T19:12:28Z

+
+    const breakpoint = _createBreakpoint(location, referenceBreakpoint);
+
+    let promise = addClientBreakpoint(


I'm a bit confused, so I'll walk through the bug and two workflows as I see them. I could be missing something, it could be what you're already doing too. I'll outline a simple case and syncing case for both workflows. To be clear, we can do the simple case and fix the bug.

Bug

User is working on an app that uses source maps (i.e. namely bundle.js w/ file a.js, b.js, c.js)

User debugs the app and adds a bp to b.js line 12, with the generated location bundle.js#45

User adds code to a.js

User reloads the app and see the debugger paused on b.js line 4, which happens to line up with the generated location bundle.js#45!

We will fix this, either simply moving the breakpoint to b.js line 4 when the debugger reloads or ideally re-removing the breakpoint at line bundle.js#45 and adding a breakpoint at the new location...

simple case

User adds a Breakpoint

addBreakpoint is called with an original location

generated location is calculated based on the source's sourcemap

a pending breakpoint is added with the generated location

User reloads page / User re-opens debugger

breakpoints reducer adds pending breakpoints

newSource is called and the debugger attempts to sync the breakpoint

checks if there is a pending breakpoint

uses the pending bps generated location to recalculate the original location

adds a new breakpoint to the store

syncing case

User adds a Breakpoint

addBreakpoint is called with an original location

generated location is calculated based on the source's sourcemap

a pending breakpoint is added with the generated location

User reloads page / User re-opens debugger

breakpoints reducer adds pending breakpoints

newSource is called and the debugger attempts to sync the breakpoint

checks if there is a pending breakpoint

uses the pending bps generated location to recalculate the original location

if the new original location is different from the pending breakpoint original location

removes the old breakpoint on the server

adds the new breakpoint to the server

adds the new breakpoint to the store

jasonLaster · 2017-05-31T14:19:39Z

+) {
+  console.log("IM CALLED NOW");
+  const location = breakpoint.location;
+  await client.removeBreakpoint(referenceBreakpoint.id);


how do we know we have an ID? the pending bp doesn't have an id

jasonLaster

This is really great work. When the dust settles we'll have a simpler system.

I left lots of comments to over share.

jasonLaster · 2017-06-01T21:50:05Z

+import { updateObj } from "../utils";
+// Return the first argument that is a string, or null if nothing is a
+// string.
+export function firstString(...args) {


this can be moved to utils/utils.js

jasonLaster · 2017-06-01T21:53:33Z

+    return location;
+  }
+  return Object.assign({}, location, { column: undefined });
+}


2 things:

I think we should do this in firefox/commands.js#onNewBreakpoint

onNewBreakpoint should be moved to firefox/create#createBreakpoint

The rationale for this is to give us consistent data from the server... we did the same thing for source URLs when we worked on pending bps

jasonLaster · 2017-06-01T21:54:15Z

  return await sourceMaps.getGeneratedLocation(location, source.toJS());
 }

+async function syncClientBreakpoint(


Lets add a function comment, we don't typically have comments but we should lol

minor style nit, but i move actiony things to options i.e.

syncClientBreakpoint(source, pendingBreakpoint, location, {client, sourceMaps})

we now have just 4 items, so this should be ok

jasonLaster · 2017-06-01T21:57:18Z

+
+  const originalLocation = await sourceMaps.getOriginalLocation(
+    Object.assign({}, generatedLocation, {
+      line: pendingBreakpoint.generatedLocation.line


this seems weird to me... can you explain the rationale

yes. and i agree that this needs to be expressed better. I didn't quite finish fixing this

when the source is reloaded, the source id changes, and i was confused by that. I wasn't sure what the source was so I quickly threw this in to see if it would work. Basically, there is a new connection. This is why we can't delete / find the line after the debuggee is reloaded.

for example,

pendingBreakpoint.generatedLocation /* { id: "server1.conn3.child1/34", line: 51234 } */ generatedLocation // /* { id: "server1.conn4.child1/34", line: 51235 } */

I didn't get around to doing this properly, but we now have the source available in this function, and this should actually be:

const originalLocation = await sourceMaps.getOriginalLocation({ id: source.id, line: pendingBreakpoint.generatedLocation.line });

Also, i don't think we need to save the generated location in full, just the line. will keep hacking away at it

jasonLaster · 2017-06-01T21:58:00Z

+  client,
+  sourceMaps,
+  pendingBreakpoint,
+  location


It's not really clear to me if you need location because it's a composite of many things

jasonLaster · 2017-06-01T21:59:45Z

+        }
+        return Object.assign({}, args, { status: "done" });
+      });
+    }


can we move this try/catch to the firefox/commands side?

jasonLaster · 2017-06-01T22:00:10Z

+
+    // If the breakpoint is already disabled, we need to communicate
+    // with the server, but we also have to remove the breakpoint immediately.
+    if (pendingBreakpoint.disabled) {


why do we need to talk to the server if it's disabled, i think that means that the server doesn't have a bp?

Good point here. Originally, my thinking is that we need to verify our breakpoints with the server, and the only way to do that (at the time at least) was to add a breakpoint to the server. If its disabled we need to then remove it. Otherwise we have the weird behavior of disabled breakpoints in places that don't make sense.

Better solution: change the client code so we can get a breakpoint location. I haven't had a chance yet to try to make this better but it feels like it should be possible

jasonLaster

here are some random thoughts

jasonLaster · 2017-06-02T17:02:45Z

+  prefs.debuggerVersion = DEBUGGER_VERSION;
+}
+
+module.exports = { prefs, setDebuggerVersion };


I think it might be nice to keep the schema version private so that we only manage it here...

if (prefs.debuggerPrefsSchemaVersion != prefsSchemaVersion) { prefs.clear() prefs.debuggerPrefsSchemaVersion = prefsSchemaVersion }

I think the best place to implement the clear function would be in devtool-module -- i added a todo note, shall i make a separate issue?

jasonLaster · 2017-06-02T17:06:00Z

 }

+// Syncing Methods
+function _optimisticlyAddBreakpoint(state, breakpoint) {


nit: not sure if we need the _. I think everything is assumed to be private

jasonLaster · 2017-06-02T17:08:43Z

+  },
+  "location": Object {
+    "column": undefined,
+    "line": 7,


it's weird that generatedLocation and location are different line numbers

good catch, that might have been a bug later on

jasonLaster · 2017-06-02T17:10:52Z

+    const endBps = selectors.getBreakpoints(getState());
+    const returnedBp = endBps.get(expectedId);
+    expect(returnedBp).not.toBe(bp);
+    expect(returnedBp.location).toEqual(correctedLocation);


might be nice to have a snapshot of returnedBp here as well

jasonLaster · 2017-06-02T17:11:18Z

+    delete bpClients[breakpointId];
+    return bpClient.remove();
+  } catch (_error) {
+    console.info("No breakpoint to delete on server");


maybe console.warn...

jasonLaster · 2017-06-02T17:17:09Z

+  const location = overrides.location || breakpoint.location;
+  const id = makeLocationId(location);
+  const updatedOpts = { ...overrides, loading: false };
+  const updatedBreakpoint = updateObj(breakpoint, updatedOpts);


I think updateObj has outlived it's purpose.
I'd also try to be more aggressive about what we want to do.

function updateBreakpoint(state, newBreakpoint, oldBreakpoint) { const id = makeLocationId(newBreakpoint.location) state.setIn(["breakpoints", id], { ...breakpoint, ...overrides, loading: false }); }

…n reload

jasonLaster · 2017-06-05T13:06:53Z

+      breakpoint,
+      [PROMISE]: promise
+    };
+


nit: now that we've simplified this I think we can inline everything

dispatch({ type: SYNC_BREAKPOINT, [PROMISE]: syncClientBreakpoint() })

jasonLaster · 2017-06-05T13:07:27Z

-        )
-      })
-    );
+    return dispatch(action);


we can also inline this action too i think

jasonLaster · 2017-06-05T13:11:19Z

+// TODO: replace this with an implementation from devtools-modules
+prefs.clear = () => {
+  prefs.pendingBreakpoints = getDefaultPref("pendingBreakpoints");
+};


function clearPrefs() { prefs.pendingBreakpoints = getDefaultPref("pendingBreakpoints"); }

jasonLaster · 2017-06-05T13:11:38Z

+  };
+
+  return defaults[prefName];
+}


I think you can get the default for a pref.

…sync-with-the-server-on-reload' of https://github.com/codehag/debugger.html into bug/2904-breakpoints-sourcemaps-breakpoints-are-out-of-sync-with-the-server-on-reload

codehag force-pushed the bug/2904-breakpoints-sourcemaps-breakpoints-are-out-of-sync-with-the-server-on-reload branch 2 times, most recently from 75ea4ed to 4853833 Compare May 30, 2017 17:41

jasonLaster reviewed May 30, 2017

View reviewed changes

jasonLaster reviewed May 31, 2017

View reviewed changes

jasonLaster reviewed Jun 1, 2017

View reviewed changes

codehag force-pushed the bug/2904-breakpoints-sourcemaps-breakpoints-are-out-of-sync-with-the-server-on-reload branch 2 times, most recently from c84a0c8 to 6729db6 Compare June 2, 2017 16:41

codehag changed the title ~~[WIP] [Breakpoints] + [SourceMaps] breakpoints are out of sync with the server on reload~~ [Breakpoints] + [SourceMaps] breakpoints are out of sync with the server on reload Jun 2, 2017

jasonLaster reviewed Jun 2, 2017

View reviewed changes

codehag added 2 commits June 5, 2017 11:02

Breakpoints +SourceMaps breakpoints are out of sync with the server o…

d61a17b

…n reload

address pr comments

bc23496

codehag force-pushed the bug/2904-breakpoints-sourcemaps-breakpoints-are-out-of-sync-with-the-server-on-reload branch from 6729db6 to bc23496 Compare June 5, 2017 09:55

jasonLaster reviewed Jun 5, 2017

View reviewed changes

jasonLaster and others added 7 commits June 5, 2017 09:22

fix mochitests

3c8e4f7

update according to suggestions on prefs, add more comments to code

19dcf86

Merge branch 'bug/2904-breakpoints-sourcemaps-breakpoints-are-out-of-…

efb3d89

…sync-with-the-server-on-reload' of https://github.com/codehag/debugger.html into bug/2904-breakpoints-sourcemaps-breakpoints-are-out-of-sync-with-the-server-on-reload

inline actions

bd04a4a

Update prefs.js

98d042c

use the simplist solution

ad3595a

Merge branch 'bug/2904-breakpoints-sourcemaps-breakpoints-are-out-of-…

219286c

…sync-with-the-server-on-reload' of https://github.com/codehag/debugger.html into bug/2904-breakpoints-sourcemaps-breakpoints-are-out-of-sync-with-the-server-on-reload

jasonLaster merged commit 0329e1a into firefox-devtools:master Jun 5, 2017


		const breakpoint = _createBreakpoint(location, referenceBreakpoint);

		let promise = addClientBreakpoint(

Conversation

codehag commented May 30, 2017

Uh oh!

codecov Bot commented May 30, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Bug

simple case

syncing case

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codehag Jun 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jasonLaster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

codecov Bot commented May 30, 2017 •

edited

Loading

codehag Jun 2, 2017 •

edited

Loading