Prior to this patch, the code marked a Debugger if any of its debuggee globals
were marked, and it had breakpoints set in JSScripts/Instances that were marked.
It cleaned up breakpoints by scanning all JSScripts and wasm::Instances in the
zones being collected, looking for breakpoints set in them, and deleting them if
either the JSScript/Instance or the owning Debugger was about to be finalized.
That byzantine approach is equivalent to having owning edges from
JSScripts/Instances to the breakpoints set in them, and from those breakpoints
to the Debuggers they belong to. Proof: if a script is live, then its global
must be live. If the script has a breakpoint set in it, and its global is live,
then its Debugger will be marked, and hence neither the Debugger nor the script
will be finalized, and hence the breakpoints will be retained.
That second approach handles the situation with a much more traditional tracing
process, wherein tracing a script traces its breakpoints, which traces their
owning Debuggers. We can simply use cross-compartment wrappers for the edges
from the breakpoints to their handlers and Debuggers, which removes a bunch of
special cases for the GC to worry about.
This patch changes things to use the second approach.
- js::DebugScript and js::wasm::DebugState get `trace` and `finalize`/`delete_`
methods, which get called from the right places. DebugScript and DebugState
become the owners of their `BreakpointSite`s.
- `BreakpointSite`s get `trace` and `finalize` methods that visit their
`Breakpoint`s. `JSBreakpointSite`s also trace their scripts, and
`WasmBreakpointSite`s trace their wasm::Instances.
- `Breakpoint`s get a `trace` method that traces their owning `Debugger` and
their handler object. They hold these as cross-compartment references, since
the `Breakpoint` logically belongs to its code's compartment. Code that uses
these fields wraps/unwraps as appropriate.
- All the specialized breakpoint sweeping code is deleted:
- `Zone::sweepBreakpoints` and its supporting `DebugAPI` functions are
deleted.
- `Debugger::hasAnyLiveHooks` no longer considers breakpoints.
- `DebugAPI::markIteratively` no longer searches for breakpoints to mark.
- `DebugAPI::traceAllForMovingGC` no longer traverses breakpoints.
Differential Revision: https://phabricator.services.mozilla.com/D49858
--HG--
extra : moz-landing-system : lando
Separate `Breakpoint::destroy` into two functions, `Breakpoint::remove`, which
takes care of cleaning up empty sites, and `Breakpoint::delete_`, which only
removes the breakpoint from its linked lists and frees its storage. Call
`Breakpoint::remove` from sites that passed `True` for `destroy`'s
`mayDestroySite` argument, and `Breakpoint::delete_` for those that passed
`False`.
The prior code had a `Breakpoint::destroy` method that took a `mayDestroySite`
flag, which indicated whether the BreakpointSite should also be cleaned up, if
the outgoing breakpoint was the last one left. Given that we never intend to
leave empty BreakpointSites lying around, it was unclear why this option was
needed.
And indeed, it was passed as `True` (yes, please destroy the site if empty) from
all call sites except one, in `js::wasm::DebugState::clearBreakpointsIn`. This
function is used for both deleting individual breakpoints and removing all
breakpoints set in a particular `wasm::Instance`, so it has two nested loops,
the outer visiting all sites in the instance, and the inner visiting each site's
breakpoints. Since `DebugState` stores `WasmBreakpointSites` in a hash table,
allowing `Breakpoint::destroy` to remove empty `BreakpointSite`s would
invalidate the outer loop's iteration, a classic bug. Instead, the inner loop
forbade `Breakpoint::destroy` from removing the site, and cleaned it up itself,
if empty, at the bottom of the outer loop body.
Later patches in this series want a cleaner separation between the high-level
operation of removing a single breakpoint, with whatever followup that entails,
versus the low-level of operation of simply freeing the structure at hand.
Differential Revision: https://phabricator.services.mozilla.com/D49857
--HG--
extra : moz-landing-system : lando
No effect on behavior.
It's SpiderMonkey's convention that the success path should be the top-level
path through a function, and that failure should be handled by early returns.
This patch makes SetBreakpointMatcher's methods conform to that convention.
Differential Revision: https://phabricator.services.mozilla.com/D49856
--HG--
extra : moz-landing-system : lando
`Debugger` objects used to have an `enabled` property, which controlled (among
other things) whether breakpoints would fire. This meant that it was possible to
have a BreakpointSite that had breakpoints set in it, but yet should never
trigger. Thus, whether or not the JIT code itself should be patched for the
breakpoint depended on both 1) whether breakpoints were set, and 2) whether any
of those breakpoints' owning Debuggers were enabled.
To manage this, each BreakpointSite had not only a linked list of inserted
breakpoints, but an 'enabled count' of how many of those breakpoints were in
enabled Debuggers. Enabling/disabling a Debugger walked all its breakpoints and
adjusted their sites' counts. When the enabled count was non-zero, the JIT code
needed traps inserted; when it was zero, the trap sites could be filled with
no-ops.
The `enabled` property seemed like an innocent idea, but it turns out to have
introduced all sorts of hair like this throughout the Debugger (for example,
also making it harder for the GC to figure out what could be removed without
observable effect), and then it turned out that the server never really wanted
to toggle them on and off anyway. So we removed 'enabled' in bug 1564168.
But this means that a BreakpointSite's enabled count should be non-zero if and
only if its breakpoint list is non-empty; the count is redundant and can be
removed.
Further, since an empty BreakpointSite should be removed altogether, any
inserted BreakpointSite can be assumed to have a non-zero enabled count. In
other words, JIT code should have traps inserted if and only if a BreakpointSite
is present for that code location.
This means we can tie JIT code trap insertion and removal to BreakpointSite
insertion and removal. Since BreakpointSite is an abstract base class, all the
code that inserts and removes sites knows which concrete class it's creating, so
we know statically how to go about patching the code. This means BreakpointSite
no longer needs a 'recompile' pure virtual method. Its implementations get merged
into the code that's inserting and removing BreakpointSite concrete instances.
Differential Revision: https://phabricator.services.mozilla.com/D49855
--HG--
extra : moz-landing-system : lando
The way a breakpoint site is removed depends on the concrete subclass, but
deciding whether the site is empty or not is common to all BreakpointSites.
There's no reason to repeat the condition in every implementation.
Differential Revision: https://phabricator.services.mozilla.com/D49854
--HG--
extra : moz-landing-system : lando
All the information we need specific to Wasm breakpoints is available in the
WasmBreakpointSite now, so there's no need for two different kinds of
breakpoints any more.
Code that used to obtain the wasm instance from the breakpoint now consults the
site instead.
BreakpointSite::allocSize confusingly provided the size of the site's
*breakpoints*, not the site itself; but now it's no longer needed at all, and
can be removed.
Differential Revision: https://phabricator.services.mozilla.com/D49853
--HG--
extra : moz-landing-system : lando
WasmInstanceObjects and wasm::Instances are 1:1, so this should have no effect.
But the next patch removes WasmBreakpoint altogether, and being able to get
exactly the same information from the site makes that patch more obviously correct.
Why not make them both hold wasm::Instance*? The IsMarkedUnbarriered checks in
DebugAPI::markIteratively and Debugger::traceForMovingGC would really like an
object pointer they can write to, which of course a wasm::Instance will not
provide. Even though using WasmInstanceObject* in both spots requires more
tracing calls, it's a shorter patch overall. (Those checks are deleted in the
end, anyway.)
Differential Revision: https://phabricator.services.mozilla.com/D49852
--HG--
extra : moz-landing-system : lando
As an abstract base class, BreakpointSite can't be constructed except as a base
class. And since BreakpointSites are owned by implementation-specific structures
like js::DebugScript (for JS breakpoints) or js::wasm::DebugState (for Wasm),
only implementation-specific code should be destructing them.
Differential Revision: https://phabricator.services.mozilla.com/D49851
--HG--
extra : moz-landing-system : lando
Much of the code in js/src/debugger/DebugScript.{cpp,h} deals only with
breakpoints set in JSScripts, which must be located at js::JSBreakpointSites,
not just any kind of js::BreakpointSite. Using the looser type seems to have
misled people into thinking they need to deal with more cases than they actually
do, resulting in unnecessary dynamic checks and confusing `if` conditions.
This patch changes js::DebugScript::breakpoints into an array of
js::JSBreakpointSite*, not just js::BreakpointSite*, and then propagates the
consequences of that through the code.
Differential Revision: https://phabricator.services.mozilla.com/D49850
--HG--
extra : moz-landing-system : lando
Although many Clients API usages are inherently exclusive (a specific claim
or control request), the execution-ready promise is shared by all requests
to get the state of a client that is not yet execution ready.
Differential Revision: https://phabricator.services.mozilla.com/D49976
--HG--
extra : moz-landing-system : lando
If a Debugger.Frame for a live stack frame has hooks set, it should not be
collected, since the failure to call the hooks would be visible to JS.
The prior code handles tracing Debugger.Frames entirely through an addition to
the iterative weak map marking process: each time through the cycle, we also
check all unmarked Debuggers of marked globals for Debugger.Frames with any
hooks set, and if any such Debugger.Frames exist, we mark the Debugger.
This is a circuitous way to get exactly the same effect as simply marking, once
at the start of GC (like any other root set), all Debugger.Frames for live stack
frames that have hooks set, as if there were an owning edge from each stack
frame to each Debugger.Frame with hooks set.
This patch removes the code from Debugger::hasAnyLiveHooks that checks
Debugger::frames, and adds a new function, DebugAPI::traceFramesWithLiveHooks,
that is called once from root marking.
Although this patch doesn't reduce the number of lines of code, I feel it is
still a simplification, because it replaces a case currently handled in
circuitous way (markIteratively, the markedAny flags, etc.) with a function that
just traces things.
This is one step towards our goal of eliminating (or at least trimming down)
DebugAPI::markIteratively.
Differential Revision: https://phabricator.services.mozilla.com/D48241
--HG--
extra : moz-landing-system : lando
Move odd circumlocution into its own function, to make it clearer what problem
it's trying to address. We'll add another use later in the patch series.
Differential Revision: https://phabricator.services.mozilla.com/D48415
--HG--
extra : moz-landing-system : lando
When suggest.history = true and suggest.bookmark = false, change the inclusion logic from `NOT bookmarked` to `visited OR NOT bookmarked`. That will include visited bookmarks above the autofill threshold but still exclude unvisited bookmarks.
This also renames the various SQL query consts to better reflect when they are used: `HISTORY_BOOKMARK` for when both suggest.history and suggest.bookmark = true, `HISTORY` for when only suggest.history = true, and `BOOKMARK` for when only suggest.bookmark = true.
Finally, it adds a bunch of test tasks consistent with the other existing autofill test tasks.
Differential Revision: https://phabricator.services.mozilla.com/D50146
--HG--
extra : moz-landing-system : lando
Currently when browser chrome tests are failing the open tabs, client,
and Remote Agent will never be closed, and as such each failing test
causes massive memory leaks.
Therefore the teardown logic needs to be moved out of the tests into
the "add_task()" function. Only that way we can make sure to run
all the clean-up steps independent of the test success state.
Differential Revision: https://phabricator.services.mozilla.com/D50233
--HG--
extra : moz-landing-system : lando
To ensure that the CDP server connection is always closed after a
test even when it is failing, its lifetime has to be handled inside
the "add_task" function.
Currently if a test fails all the registered events and observer
notifications are getting leaked. This patch ensures that all of
those events and notifications are getting unregistered.
Differential Revision: https://phabricator.services.mozilla.com/D50232
--HG--
extra : moz-landing-system : lando
Due to some obvious bugs in the code of TabObserver.jsm the registered
targets for each of the window's tabs haven't been unregistered when
the window has been closed.
It has the effect that when closing the Remote Agent the browsingContext
of the tab target, which has to be destroyed, cannot be retrieved.
Instead an error is raises, because the underlying frameLoader actually
doesn't exist anymore.
Given that "TabClose" events aren't fired when the window closes,
those have to be emulated.
Differential Revision: https://phabricator.services.mozilla.com/D50231
--HG--
extra : moz-landing-system : lando
When suggest.history = true and suggest.bookmark = false, change the inclusion logic from `NOT bookmarked` to `visited OR NOT bookmarked`. That will include visited bookmarks above the autofill threshold but still exclude unvisited bookmarks.
This also renames the various SQL query consts to better reflect when they are used: `HISTORY_BOOKMARK` for when both suggest.history and suggest.bookmark = true, `HISTORY` for when only suggest.history = true, and `BOOKMARK` for when only suggest.bookmark = true.
Finally, it adds a bunch of test tasks consistent with the other existing autofill test tasks.
Differential Revision: https://phabricator.services.mozilla.com/D50146
--HG--
extra : moz-landing-system : lando
This fixes the cases where we may end urlbar layout extension when the panel is open, causing it to appear as if it has a transparent background.
On dragstart the view is closed, but layout extension is no more ended, because there's no more risk of overlapping toolbar items.
Additionally it stops clearing results when ui.popup.disable_autohide is enabled, so they can be analyzed from the Browser Toolbox.
Differential Revision: https://phabricator.services.mozilla.com/D50077
--HG--
extra : moz-landing-system : lando
Jim Blandy
Marco Castelluccio
Thomas Nguyen
Chris Manchester
Cosmin Sabou
Jean-Yves Avenard
Anthony Ramine
Jeff Gilbert
Jeff Walden
Matt Brandt
Andrew Sutherland
Ehsan Akhgari
Gabriel Luong
Brian Hackett
Johann Hofmann
Kashav Madan
Mark Striemer
Christian Holler
Drew Willcoxon
Razvan Maries
Christoph Kerschbaumer
Andreea Pavel
Henrik Skupin
Marco Bonardo
Mike de Boer
Mike Conley
Belén Albeza