Bug 195248

<rdar://problem/48538465>

Created attachment 363433 [details] Patch

Comment on attachment 363433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363433&action=review r- for some multi-target logic. I didn't go in depth after finding a few issues. The Multimap code looks good, you may want to separate that out into its own patch. > Source/WebInspectorUI/UserInterface/Base/Multimap.js:65 > + let deleted = valueSet.delete(value); > + > + // Allow an entire key to be removed by not passing a value. > + if (arguments.length === 1 || !valueSet.size) > + deleted = this._map.delete(key); I'd go with: // Allow an entire key to be removed by not passing a value. if (arguments.length === 1) return this._map.delete(key); let deleted = valueSet.delete(value); if (deleted && !valueSet.size) this._map.delete(key); return deleted; Which: 1. Avoids a `set.delete(undefined)` when arguments.length is 1 2. Avoids a `map.delete(key)` when valueSet shouldn't be empty 3. Avoids a duplicate unnecessary assignment to `deleted` when valueSet is empty > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:180 > + for (let target of WI.targets) > + this._updateDOMBreakpoint(breakpoint, target); This is another where maybe we use WI.assumingMainTarget(). > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:211 > + for (let target of WI.targets) > + target.DOMDebuggerAgent.removeDOMBreakpoint(nodeIdentifier, breakpoint.type); It is not safe to assume the target has a DOMDebuggerAgent (a ServiceWorker target will not). Typically the first thing we *should* do inside of WI.target loops is check for the relevant agent the loop will make use of. Currently many call sites don't because they deal with agents that are guaranteed to exist in all targets (RuntimeAgent, DebuggerAgent, and ConsoleAgent). But like the loop you just added for CanvasAgent and another for HeapAgent checking is the right thing to do. The pattern I'd suggest is: for (let target of WI.targets) { if (target.DOMDebuggerAgent) { ... } } Note that some of your other code gets around this by burying the agent check inside functions, which is fine but tricky to review. We could add a utility such as: for (let target of WI.targetsWith("DOMDebugger")) { ... } Which might reduce complexity at sites like this. Then this entire file could follow that pattern. --- In any case, this specific code (DOM Breakpoints / Node breakpoint) will likely be incorrect for the coming future anyways. A DOMNode / nodeIdentifier will only be valid in a single Target (eventually a Frame target). We probably don't want to tell all targets about a nodeIdentifier that doesn't correspond to them. Ideally in this case the breakpoint would know the target the nodeIdentifier is associated with, so you'd end up with: breakpoint.target.removeDOMBreakpoint(nodeIdentifier, breakpoint.type); So for now maybe convert this one to just: // We should get the target associated with the nodeIdentifier in this breakpoint. let target = WI.assumingMainTarget(); target.DOMDebuggerAgent.removeDOMBreakpoint(nodeIdentifier, breakpoint.type); Put simply: • Some DOMDebugger breakpoint types (Event, XHR, URL) are universal, and make sense to broadcast messages to all targets with a DOMDebuggerAgent. • Some DOMDebugger breakpoint types (Node, EventListener) are context specific, and don't make sense to broadcast, but instead should be associated with a single target. Let me know if that all made sense or if you view this differently. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:285 > + for (let target of WI.targets) { Check target.DOMDebuggerAgent. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:355 > + for (let target of WI.targets) { Check target.DOMDebuggerAgent. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:449 > - this._updateDOMBreakpoint(breakpoint); > + this._updateDOMBreakpoint(breakpoint, WI.assumingMainTarget()); Lets add a similar comment that ideally we would find the target associated with this target specific DOM breakpoint. I like the idea of adding comments next to assumingMainTarget so when we come across it later we know why! > LayoutTests/inspector/unit-tests/multimap.html:86 > + suite.addTestCase({ > + name: "Multimap.prototype.delete", > + test() { > + let multimap = new Multimap; > + > + multimap.add(0, 1); > + multimap.add(2, 3); > + multimap.add(2, 3); > + > + InspectorTest.log(multimap); > + > + InspectorTest.expectThat(multimap.delete(0, 1), "The key and the value were successfully deleted."); > + > + InspectorTest.log(multimap); > + > + InspectorTest.expectThat(multimap.delete(2, 3), "The key and the value were successfully deleted."); > + > + InspectorTest.log(multimap); > + }, > + }); I'd like to see this test deleting a value from a set that has multiple values. multimap.add(0, 1); multimap.add(2, 3); multimap.add(2, 4); log(); delete(0, 1); log(); // should see `0` keys go away entirely delete(2, 3); log(); // should see `[2,4]` still delete(2, 4); log(); // should see empty > LayoutTests/inspector/unit-tests/multimap.html:155 > + multimap.add("badger", "raccoon"); Would be nice to give this more values.

Comment on attachment 363433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363433&action=review >> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:211 >> + target.DOMDebuggerAgent.removeDOMBreakpoint(nodeIdentifier, breakpoint.type); > > It is not safe to assume the target has a DOMDebuggerAgent (a ServiceWorker target will not). Typically the first thing we *should* do inside of WI.target loops is check for the relevant agent the loop will make use of. Currently many call sites don't because they deal with agents that are guaranteed to exist in all targets (RuntimeAgent, DebuggerAgent, and ConsoleAgent). But like the loop you just added for CanvasAgent and another for HeapAgent checking is the right thing to do. > > The pattern I'd suggest is: > > for (let target of WI.targets) { > if (target.DOMDebuggerAgent) { > ... > } > } > > Note that some of your other code gets around this by burying the agent check inside functions, which is fine but tricky to review. > > We could add a utility such as: > > for (let target of WI.targetsWith("DOMDebugger")) { > ... > } > > Which might reduce complexity at sites like this. Then this entire file could follow that pattern. > > --- > > In any case, this specific code (DOM Breakpoints / Node breakpoint) will likely be incorrect for the coming future anyways. A DOMNode / nodeIdentifier will only be valid in a single Target (eventually a Frame target). We probably don't want to tell all targets about a nodeIdentifier that doesn't correspond to them. Ideally in this case the breakpoint would know the target the nodeIdentifier is associated with, so you'd end up with: > > breakpoint.target.removeDOMBreakpoint(nodeIdentifier, breakpoint.type); > > So for now maybe convert this one to just: > > // We should get the target associated with the nodeIdentifier in this breakpoint. > let target = WI.assumingMainTarget(); > target.DOMDebuggerAgent.removeDOMBreakpoint(nodeIdentifier, breakpoint.type); > > Put simply: > • Some DOMDebugger breakpoint types (Event, XHR, URL) are universal, and make sense to broadcast messages to all targets with a DOMDebuggerAgent. > • Some DOMDebugger breakpoint types (Node, EventListener) are context specific, and don't make sense to broadcast, but instead should be associated with a single target. > > Let me know if that all made sense or if you view this differently. Ah oops! I was in a bit of a rush/anger to rewrite this, as I had done it once and then accidentally `git checkout`d my repo, which killed all my work :/ Looks like I missed a few things when redoing it :(

Created attachment 363858 [details] Patch

Created attachment 363859 [details] Patch Oops. Copypasta :P

Comment on attachment 363859 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=363859&action=review r=me > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:180 > + if (!breakpoint.disabled) { > + this._updateDOMBreakpoint(breakpoint, WI.assumingMainTarget()); > + } Style: No braces > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:256 > + for (let target of WI.targets) > + this._updateEventBreakpoint(breakpoint, target); It would make more sense to me to put the if (target.DOMDebuggerAgent) here in this loop then buried in _updateEventBreakpoint. It could be an assert there. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:328 > + for (let target of WI.targets) > + this._updateURLBreakpoint(breakpoint, target); It would make more sense to me to put the if (target.DOMDebuggerAgent) here in this loop then buried in _updateURLBreakpoint. It could be an assert there. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:542 > + for (let target of WI.targets) > + this._updateDOMBreakpoint(breakpoint, target); It would make more sense to me to put the if (target.DOMDebuggerAgent) here in this loop then buried in _updateDOMBreakpoint. It could be an assert there. > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:549 > + for (let target of WI.targets) Ditto > Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:561 > + for (let target of WI.targets) Ditto

Created attachment 364288 [details] Patch

Comment on attachment 364288 [details] Patch Clearing flags on attachment: 364288 Committed r242743: <https://trac.webkit.org/changeset/242743>

All reviewed patches have been landed. Closing bug.

It appears that the changes in https://trac.webkit.org/changeset/242743/webkit has caused the test inspector/dom-debugger/dom-breakpoints.html to become a constant failure. History: https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fdom-debugger%2Fdom-breakpoints.html Diff: --- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/dom-debugger/dom-breakpoints-expected.txt +++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/dom-debugger/dom-breakpoints-actual.txt @@ -59,8 +59,6 @@ PASS: Added 'subtree-modified' breakpoint. PASS: Added 'attribute-modified' breakpoint. PASS: Added 'node-removed' breakpoint. -PASS: Removed 3 breakpoints. -PASS: DOM node should have no breakpoints. -- Running test teardown. -- Running test case: SetBreakpointWithInvalidNodeId

(In reply to Truitt Savell from comment #11) > It appears that the changes in > https://trac.webkit.org/changeset/242743/webkit > > has caused the test inspector/dom-debugger/dom-breakpoints.html > > to become a constant failure. > > History: > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard. > html#showAllRuns=true&tests=inspector%2Fdom-debugger%2Fdom-breakpoints.html > > Diff: > --- > /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/ > inspector/dom-debugger/dom-breakpoints-expected.txt > +++ > /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/ > inspector/dom-debugger/dom-breakpoints-actual.txt > @@ -59,8 +59,6 @@ > PASS: Added 'subtree-modified' breakpoint. > PASS: Added 'attribute-modified' breakpoint. > PASS: Added 'node-removed' breakpoint. > -PASS: Removed 3 breakpoints. > -PASS: DOM node should have no breakpoints. > -- Running test teardown. > > -- Running test case: SetBreakpointWithInvalidNodeId Landed a followup fix <https://trac.webkit.org/changeset/242765/webkit>.