This CL removes a race that may happen if PersistenceImpl tries to
move a breakpoint from one UISourceCode to another UISourceCode by
explicitly awaiting the calls to remove/setBreakpoint in
PersistenceImpl.
The race happens in the following case (affecting snippets):
* A snippet is created, and the user sets a breakpoint and reloads
the page.
* PersistenceImpl.ts will try to move the breakpoint via
1. removing the breakpoint from the old UISourceCode, and
2. re-setting the breakpoint on the new UISourceCode.
* Simultaneously, the BreakpointManager tries to set a breakpoint
on the new UISourceCode
* For snippets, the url stays the same after reloading, and in some
cases the `setBreakpointByURL` of the PersistenceImpl and the
BreakpointManager will be sent to v8 one after another, such that
v8 returns a ServerError, as it has already set the breakpoint
previously
This race causes the breakpoint to completely disappear, since
DevTools front-end removes the breakpoint if it sees a ServerError.
Drive-by: Added additional clean up step to tests.
Bug: 1280621
Change-Id: I7c9cd94c02d80661100cc5c5141e7d0dbb027f6d
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3629862
Reviewed-by: Simon Zünd <szuend@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Kim-Anh Tran <kimanh@chromium.org>
This is a reland of commit 1b596f3b4b.
Changes:
1. Gated behind an experiment
2. Removed changes that are currently not used since it's gated
(i.e. test changes)
Original change's description:
> Enable synchronization on instrumentation breakpoints
>
> This CL introduces the synchronization point for setting
> breakpoints on new scripts using instrumentation breakpoints.
>
> Instrumentation breakpoints will trigger at the first statement
> of a newly run script. Afterwards we wait for all resources
> that we need to set a breakpoint (if one is cached for that
> particular script), set it, and resume.
>
> Note: this will not correctly synchronize the case where we
> just opened DevTools or just created a new target, as at this
> point some scripts might have started running. For this, we
> need to synchronize the Debugger.enable call, which is something
> that is left to take care of.
>
> Bug: chromium:1229541, chromium:1133307, chromium:1300509
> Change-Id: I44e9b053a7cf64cc1f477a68ce9389cd92a1d05d
> Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3470237
> Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
> Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
> Commit-Queue: Kim-Anh Tran <kimanh@chromium.org>
Bug: chromium:1229541, chromium:1133307, chromium:1300509
Change-Id: I19553ddac9530fa3ccac6b245a3dd89da88c6d51
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3542248
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Kim-Anh Tran <kimanh@chromium.org>
This reverts commit 1b596f3b4b.
Reason for revert: closed the tree, more blink tests to adapt
Original change's description:
> Enable synchronization on instrumentation breakpoints
>
> This CL introduces the synchronization point for setting
> breakpoints on new scripts using instrumentation breakpoints.
>
> Instrumentation breakpoints will trigger at the first statement
> of a newly run script. Afterwards we wait for all resources
> that we need to set a breakpoint (if one is cached for that
> particular script), set it, and resume.
>
> Note: this will not correctly synchronize the case where we
> just opened DevTools or just created a new target, as at this
> point some scripts might have started running. For this, we
> need to synchronize the Debugger.enable call, which is something
> that is left to take care of.
>
> Bug: chromium:1229541, chromium:1133307, chromium:1300509
> Change-Id: I44e9b053a7cf64cc1f477a68ce9389cd92a1d05d
> Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3470237
> Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
> Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
> Commit-Queue: Kim-Anh Tran <kimanh@chromium.org>
Bug: chromium:1229541, chromium:1133307, chromium:1300509
Change-Id: I9eccdf065820854746ec7ad4643362859c9960e4
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3506653
Auto-Submit: Kim-Anh Tran <kimanh@chromium.org>
Reviewed-by: Johan Bay <jobay@chromium.org>
Commit-Queue: Kim-Anh Tran <kimanh@chromium.org>
Owners-Override: Kim-Anh Tran <kimanh@chromium.org>
This CL introduces the synchronization point for setting
breakpoints on new scripts using instrumentation breakpoints.
Instrumentation breakpoints will trigger at the first statement
of a newly run script. Afterwards we wait for all resources
that we need to set a breakpoint (if one is cached for that
particular script), set it, and resume.
Note: this will not correctly synchronize the case where we
just opened DevTools or just created a new target, as at this
point some scripts might have started running. For this, we
need to synchronize the Debugger.enable call, which is something
that is left to take care of.
Bug: chromium:1229541, chromium:1133307, chromium:1300509
Change-Id: I44e9b053a7cf64cc1f477a68ce9389cd92a1d05d
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3470237
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Kim-Anh Tran <kimanh@chromium.org>
Since we record changed line numbers from formatted sources, we should
always compare formatted current line number against the changed line
numbers. This CL also retrieves the formatted mapping from the diff call
site and stores it for said comparison.
Bug: chromium:1268754
Change-Id: I9f0bdd7f081b17d15a1f80ce788de54defb6717b
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3466037
Reviewed-by: Mathias Bynens <mathias@chromium.org>
Commit-Queue: Changhao Han <changhaohan@chromium.org>
This fixes a bug where SourcePanel.executionLineChanged can revive stale
source code if `await liveLocation.uiLocation()` gets pre-empted by
deleting the source code.
This can happen on a reload of a page with a source mapped JavaScript
file while the execution is stopped in that JavaScript file. When
resetting the DebuggerModel, we first detach source maps. The detach
handler will schedule execution line update (among other things). Then
the scripts are removed. Once the scheduled execution line update runs,
it will revive the source code with the execution line in
TabbedEditorContainer. When/if the script is actually loaded by the VM,
the front-end will then incorrectly skip loading the script contents
from the VM because its content is already present in
TabbedEditorContainer. As a result, if the newly run script had
different content it won't be reflected in the source panel.
This patch solves the problem by skipping the execution line update
logic if the execution line location got discarded in the meantime.
Bug: chromium:508270
Change-Id: I338e5fda0b43edf8537d7eceea14940c9fa33238
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3365271
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Kim-Anh Tran <kimanh@chromium.org>
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
This CL restructures scheduleUpdateInDebugger to allow callers
to await the update. In follow up changes we will
need to properly await scheduleUpdateInDebugger to make sure
that an update (setting the breakpoint) has happened before
we can continue execution. Specifically, this is relevant
in scenarios in which we stop execution (e.g. through
instrumentation breakpoints) to make sure that all breakpoints
are set, before allowing the execution to continue.
Bug: chromium:1133307, chromium:1229541
Change-Id: I3ef2103ff03aaa02503a680b190c2d46148448de
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3361035
Reviewed-by: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Kim-Anh Tran <kimanh@chromium.org>
Following up on https://crrev.com/c/3069289 the back-end reports
positions (raw locations) for inline scripts with `//# sourceURL`
annotations relative to the start of the <script> tag content, rather
than relative to the surrounding document.
The DevTools front-end however was still trying to make up for the
wrong positions reported by the back-end, leading to negative line
numbers being reported and sometimes jumping to the wrong line when
using inline scripts with `//# sourceURL` annotations.
This change removes the hacks from the front-end and simply uses the
positions from the back-end, which are now correct.
Fixed: chromium:1270227
Bug: chromium:578269, chromium:1183990
Change-Id: I65a59f64f42230179f56218518e727116329ad5d
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3291151
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Commit-Queue: Kim-Anh Tran <kimanh@chromium.org>
Reviewed-by: Kim-Anh Tran <kimanh@chromium.org>
With https://crrev.com/c/3172834 we previously aligned the stepping
behavior of sourcemaps (both JavaScript and WebAssembly) with that
of DWARF (WebAssembly only), but we still gated that adjustment
behind the 'Empty sourcemap auto-stepping' experiment.
This enables the aligned behavior by default and removes the (confusing)
and according to usage metrics almost completely unused experiment.
Bug: chromium:1232347
Change-Id: Idfa62a2cfa337d4ced3486baa3249cb95a8d8c16
Fixed: chromium:1018234
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3197720
Auto-Submit: Benedikt Meurer <bmeurer@chromium.org>
Reviewed-by: Jaroslav Sevcik <jarin@chromium.org>
Commit-Queue: Benedikt Meurer <bmeurer@google.com>
This reverts commit a09263601b.
This change did not cause the original breakage, so we are relanding
the original CL as is.
Original change's description:
> Improve stepping with source maps
>
> This aligns stepping through source maps with stepping through
> Wasm+Dwarf: when stepping from a location, we lookup the corresponding
> source location and then make sure we skip all the raw/compiled
> locations that correspond to that source location.
>
> To implement the skipping, we reuse the skipList mechanism that is
> already used for stepping through Wasm+Dwarf.
>
> Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3093161
Bug: crbug.com/1232347
Change-Id: Ibe83e035aea73f070949902051dd8f100dde6f32
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3172834
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Reviewed-by: Philip Pfaffe <pfaffe@chromium.org>
This aligns stepping through source maps with stepping through
Wasm+Dwarf: when stepping from a location, we lookup the corresponding
source location and then make sure we skip all the raw/compiled
locations that correspond to that source location.
To implement the skipping, we reuse the skipList mechanism that is
already used for stepping through Wasm+Dwarf.
Bug: crbug.com/1232347
Change-Id: Ib19695b2078256282189df5b10d9165633849799
Reviewed-on: https://chromium-review.googlesource.com/c/devtools/devtools-frontend/+/3093161
Commit-Queue: Jaroslav Sevcik <jarin@chromium.org>
Reviewed-by: Philip Pfaffe <pfaffe@chromium.org>
Reviewed-by: Kim-Anh Tran <kimanh@chromium.org>
Kateryna Prokopenko