canjs / devtools

Chrome DevTools for CanJS.
https://chrome.google.com/webstore/detail/canjs-devtools/hhdfadlgplkpapjfehnjhcebebgmibcb
MIT License
5 stars 0 forks source link

Restore breakpoints with a dedicated message type instead of using addBreakpoint #93

Closed bmomberger-bitovi closed 4 years ago

bmomberger-bitovi commented 4 years ago

Check out this GIF: devtools-breakpoints-true-async

What's happening here is that there are two chrome breakpoints that are set here in addBreakpoint: one stops execution while the other just console.logs the breakpoint expression and continues. Even though execution is stopped at the breakpoint once, all three breakpoints are logged.

This is because message handling between the devtools contexts and the page context in Chrome happens in parallel. For most asynchronous code in a page context, the UI thread has script executions and painting scheduled on it, and except for workers, no other execution happens. Here, though the breakpoint stops the UI thread, messaging is processed anyway (probably a separate system thread is running the message handler and sharing context with the UI thread).

This is extremely confusing to devs who expect this breakpoint to block the next message from being handled. It would help to dump all breakpoints into the injected script in a single message when they need to be restored for a particular frame.

phillipskevin commented 4 years ago

So to do this, we need to create another function in the injected script that restores all breakpoints at once instead of calling addBreakpoint for each individual breakpoint.

phillipskevin commented 4 years ago

Also, we should be able to remove this code which is working around this behavior: https://github.com/canjs/devtools/blob/87a185f29408bbf02b0bc944f304f85280cd7c46/canjs-devtools-injected-script.js#L304-L310

phillipskevin commented 4 years ago

Posting some notes from a conversation here.

There are two ways breakpoints get added:

  1. when you add a new breakpoint: https://github.com/canjs/devtools/blob/87a185f29408bbf02b0bc944f304f85280cd7c46/panel/panel.mjs#L138-L143
  2. when you load the page, breakpoints added previously are restored: https://github.com/canjs/devtools/blob/87a185f29408bbf02b0bc944f304f85280cd7c46/panel/panel.mjs#L266-L277

This calls the getBreakpointEvalString helper: https://github.com/canjs/devtools/blob/87a185f29408bbf02b0bc944f304f85280cd7c46/canjs-devtools-helpers.mjs#L170-L222 which creates a string that generates an observation (and some other things).

...and then calls addBreakpoint: https://github.com/canjs/devtools/blob/87a185f29408bbf02b0bc944f304f85280cd7c46/canjs-devtools-injected-script.js#L282-L331

which calls canReflect.onValue for the observation to actually make the breakpoint work.

The goal of this story is to change 2 above to restore all of the breakpoints at once instead of calling addBreakpoint individually for each.