Bug 1824726 - [devtools] Throttle all resources from the server side. r=devtools-reviewers,nchevobbe,perftest-reviewers,sparky

This helps accumulate RDP packets related to resources being available/updated/destroyed
and emit less and larger RDP packets instead or more and smaller.
This appears to reduce the overhead of DevTools transport and JSWindowActor layers.

The issue in browser_markup_events_toggle.js is interesting.
It highlights that a resource notified before a call to WatcherActor.watchResources,
may be emitted during the call to watchResources.
This makes ignoreExistingResources flag a bit brittle as that resource should be
considered as an existing/past one.
We should probably flush past resource and probably introduce a more explicit
way of handling "existing" resources on the server side.

The fix in document-events.js relates to failures in browser_net_offline_mode.js.
This test was passing thanks to late dom-complete event emitted on the previous WindowGlobal.
Surprisingly, when reloading the page in offline mode, the previous, already existing WindowGlobal
triggered the WebProgressListener and we were emitting dom-complete event.
Because of throttling, this resource is no longer transfered to the client as the related target
actor is already destroyed on reload.
But at the end, the issue was the missing dom-interactive and dom-complete events for the error page.

Regarding browser_net_ws-sse-persist-columns.js, it looks like this test was properly waiting
for the WebSocket table to update. We were really lucky it was working without frequent intermittent!

Similarly to the client side, DOCUMENT_EVENT's will-navigate is special
and has to be emitted synchronously in order to prevent clearing things out of order.
This is also true for NETWORK_EVENT_STACKTRACE which is expected to be received
*before* related NETWORK_EVENT. NETWORK_EVENT_STACKTRACE is fired from the content
process while NETWORK_EVENT is fired from the parent process.
For now, it is easier to synchronously emit those resources rather than trying
to do cross process coordination.
We may revisit this choice once we start doing throttling from the parent process
and may be once D143409 lands.

About browser_resources_clear_resources.js, it is surprising it wasn't already failing.

Differential Revision: https://phabricator.services.mozilla.com/D197772
This commit is contained in:
Alexandre Poirot 2024-06-10 21:29:00 +00:00
parent eb0e441e9a
commit 5170312c97
21 changed files with 291 additions and 71 deletions

View File

@ -113,10 +113,22 @@ add_task(async function () {
eventTooltipBadge.classList.contains("has-disabled-events"),
"event badge still has the has-disabled-events class"
);
({ onResource: onConsoleInfoMessage } =
await resourceCommand.waitForNextResource(
resourceCommand.TYPES.CONSOLE_MESSAGE,
{
ignoreExistingResources: true,
predicate(resource) {
return resource.message.level == "info";
},
}
));
await safeSynthesizeMouseEventAtCenterInContentPage("#target");
data = await getTargetElementHandledEventData();
is(data.click, 2, `click event listener was disabled`);
is(data.mousedown, 1, `and mousedown still is disabled as well`);
await onConsoleInfoMessage;
ok(true, `the "click" event listener (console.info) was called`);
info("Uncheck the mouseup event checkbox");
await toggleEventListenerCheckbox(tooltip, mouseupHeader);

View File

@ -36,6 +36,7 @@ add_task(async function () {
const requestsListStatus = requestItem.querySelector(".status-code");
EventUtils.sendMouseEvent({ type: "mouseover" }, requestsListStatus);
await waitUntil(() => requestsListStatus.title);
await waitForDOMIfNeeded(requestItem, ".requests-list-timings-total");
}
verifyRequestItemTarget(

View File

@ -83,11 +83,15 @@ add_task(async function () {
clickOnSidebarTab(document, "response");
// Get all messages present in the "Response" panel
const frames = document.querySelectorAll(
"#messages-view .message-list-table .message-list-item"
);
let frames = await waitFor(() => {
const nodeList = document.querySelectorAll(
"#messages-view .message-list-table .message-list-item"
);
return nodeList.length === 2 ? nodeList : null;
});
// Check expected results
is(frames.length, 2, "There should be two frames");
let columnHeaders = Array.prototype.map.call(
@ -139,6 +143,16 @@ add_task(async function () {
store.dispatch(Actions.toggleMessageColumn("eventName"));
store.dispatch(Actions.toggleMessageColumn("retry"));
await waitFor(
() =>
document.querySelectorAll(
"#messages-view .message-list-headers .button-text"
).length === 5
);
frames = document.querySelectorAll(
"#messages-view .message-list-table .message-list-item"
);
columnHeaders = Array.prototype.map.call(
document.querySelectorAll(
"#messages-view .message-list-headers .button-text"

View File

@ -1626,6 +1626,11 @@ export class StyleEditorUI extends EventEmitter {
};
#onResourceUpdated = async updates => {
// The editors are instantiated asynchronously from onResourceAvailable,
// but we may receive updates right after due to throttling.
// Ensure waiting for this async work before trying to update the related editors.
await this.#waitForLoadingStyleSheets();
for (const { resource, update } of updates) {
if (
update.resourceType === this.#toolbox.resourceCommand.TYPES.STYLESHEET

View File

@ -24,6 +24,32 @@ async function closeAndReopenToolbox() {
return newui;
}
/**
* In this test, we are modifying rules via the inspector, which doesn't use STYLESHEET's onResourceUpdated event,
* but the style editor do listen for these events. The issue is that the style editor triggers asynchronous updates
* and new RDP requests which only happens randomly when the update notification happens late.
* In order to prevent that, ensure waiting for all the updates notifications while doing the action in the inspector,
* so that the style editor only gets STYLESHEET's available notification while opening.
*/
async function waitForNextStyleSheetResourceUpdate(inspector) {
const { resourceCommand } = inspector.commands;
const { promise, resolve } = Promise.withResolvers();
const onAvailable = () => {};
const onUpdated = () => {
resourceCommand.unwatchResources([resourceCommand.TYPES.STYLESHEET], {
onAvailable,
onUpdated,
});
resolve();
};
await resourceCommand.watchResources([resourceCommand.TYPES.STYLESHEET], {
onAvailable,
onUpdated,
});
return { onResourceUpdated: promise };
}
add_task(async function () {
await addTab(TESTCASE_URI);
const { inspector, view } = await openRuleView();
@ -31,20 +57,28 @@ add_task(async function () {
let ruleEditor = getRuleViewRuleEditor(view, 1);
// Disable the "font-size" property.
let { onResourceUpdated } = await waitForNextStyleSheetResourceUpdate(
inspector
);
let propEditor = ruleEditor.rule.textProps[0].editor;
let onModification = view.once("ruleview-changed");
propEditor.enable.click();
await onModification;
await onResourceUpdated;
// Disable the "color" property. Note that this property is in a
// rule that also contains a non-inherited property -- so this test
// is also testing that property editing works properly in this
// situation.
({ onResourceUpdated } = await waitForNextStyleSheetResourceUpdate(
inspector
));
ruleEditor = getRuleViewRuleEditor(view, 3);
propEditor = ruleEditor.rule.textProps[1].editor;
onModification = view.once("ruleview-changed");
propEditor.enable.click();
await onModification;
await onResourceUpdated;
let { ui } = await openStyleEditor();

View File

@ -357,6 +357,15 @@ async function watchResources(rootOrWatcherOrTargetActor, resourceTypes) {
watchers.set(rootOrWatcherOrTargetActor, watcher);
}
await Promise.all(promises);
// Force sending resources to the client before we resolve.
// So that resources are received by the client
// before WatcherActor.watchResources resolves.
// This is important when ResourceCommand.watchResources's `ignoreExistingResources` flag is set to false (default behavior).
// The client code expects all resources to be emitted when this server method resolves.
if (rootOrWatcherOrTargetActor.emitResources) {
rootOrWatcherOrTargetActor.emitResources();
}
}
exports.watchResources = watchResources;

View File

@ -102,7 +102,7 @@ class StyleSheetWatcher {
})
);
await this._onAvailable(resources);
this._onAvailable(resources);
}
_notifyResourceUpdated(

View File

@ -135,6 +135,8 @@ class RootActor extends Actor {
"dom.worker.console.dispatch_events_to_main_thread"
)
: true,
// @backward-compat { version 129 } The server started throttling ressource emission
throttledResources: true,
};
}

View File

@ -6,11 +6,14 @@
const { Actor } = require("resource://devtools/shared/protocol.js");
const {
TYPES,
TYPES: { DOCUMENT_EVENT, NETWORK_EVENT_STACKTRACE, CONSOLE_MESSAGE },
getResourceWatcher,
} = require("resource://devtools/server/actors/resources/index.js");
const Targets = require("devtools/server/actors/targets/index");
const { throttle } = require("resource://devtools/shared/throttle.js");
const RESOURCES_THROTTLING_DELAY = 100;
loader.lazyRequireGetter(
this,
"SessionDataProcessors",
@ -27,8 +30,24 @@ class BaseTargetActor extends Actor {
* @return {string}
*/
this.targetType = targetType;
// Lists of resources available/updated/destroyed RDP packet
// currently queued which will be emitted after a throttle delay.
this.#throttledResources = {
available: [],
updated: [],
destroyed: [],
};
this.#throttledEmitResources = throttle(
this.emitResources.bind(this),
RESOURCES_THROTTLING_DELAY
);
}
#throttledResources;
#throttledEmitResources;
/**
* Process a new data entry, which can be watched resources, breakpoints, ...
*
@ -74,6 +93,8 @@ class BaseTargetActor extends Actor {
/**
* Called by Resource Watchers, when new resources are available, updated or destroyed.
* This will only accumulate resource update packets into throttledResources object.
* The actualy sending of resources will happen from emitResources.
*
* @param String updateType
* Can be "available", "updated" or "destroyed"
@ -92,7 +113,48 @@ class BaseTargetActor extends Actor {
this.overrideResourceBrowsingContextForWebExtension(resources);
}
this.emit(`resource-${updateType}-form`, resources);
const shouldEmitSynchronously = resources.some(
resource =>
(resource.resourceType == DOCUMENT_EVENT &&
resource.name == "will-navigate") ||
resource.resourceType == NETWORK_EVENT_STACKTRACE
);
this.#throttledResources[updateType].push.apply(
this.#throttledResources[updateType],
resources
);
// Force firing resources immediately when:
// * we receive DOCUMENT_EVENT's will-navigate
// This will force clearing resources on the client side ASAP.
// Otherwise we might emit some other RDP event (outside of resources),
// which will be cleared by the throttled/delayed will-navigate.
// * we receive NETWOR_EVENT_STACKTRACE which are meant to be dispatched *before*
// the related NETWORK_EVENT fired from the parent process. (we aren't throttling
// resources from the parent process, so it is even more likely to be dispatched
// in the wrong order)
if (shouldEmitSynchronously) {
this.emitResources();
} else {
this.#throttledEmitResources();
}
}
/**
* Flush resources to DevTools transport layer, actually sending all resource update packets
*/
emitResources() {
if (this.isDestroyed()) {
return;
}
for (const updateType of ["available", "updated", "destroyed"]) {
const resources = this.#throttledResources[updateType];
if (!resources.length) {
continue;
}
this.#throttledResources[updateType] = [];
this.emit(`resource-${updateType}-form`, resources);
}
}
/**
@ -172,7 +234,7 @@ class BaseTargetActor extends Actor {
if (this.isTopLevelTarget) {
const consoleMessageWatcher = getResourceWatcher(
this,
TYPES.CONSOLE_MESSAGE
CONSOLE_MESSAGE
);
if (consoleMessageWatcher) {
consoleMessageWatcher.emitMessages([

View File

@ -743,6 +743,12 @@ class WindowGlobalTargetActor extends BaseTargetActor {
}
this.destroying = true;
// Force flushing pending resources if the actor isn't already destroyed.
// This helps notify the client about pending resources on navigation.
if (!this.isDestroyed()) {
this.emitResources();
}
// Tell the thread actor that the window global is closed, so that it may terminate
// instead of resuming the debuggee script.
// TODO: Bug 997119: Remove this coupling with thread actor

View File

@ -116,6 +116,16 @@ DocumentEventsListener.prototype = {
isFrameSwitching,
});
// Error pages, like the Offline page, i.e. about:neterror?...
// are special and the WebProgress listener doesn't trigger any notification for them.
// Also they are stuck on "interactive" state and never reach the "complete" state.
// So fake the two missing events.
if (window.docShell.currentDocumentChannel?.loadInfo.loadErrorPage) {
this.onContentLoaded({ target: window.document }, isFrameSwitching);
this.onLoad({ target: window.document }, isFrameSwitching);
return;
}
const { readyState } = window.document;
if (readyState != "interactive" && readyState != "complete") {
// When EFT is enabled, we track this event via the WebProgressListener interface.

View File

@ -103,6 +103,16 @@ export const ContentProcessWatcherRegistry = {
return this._getAllWatchersDataMap().values();
},
/**
* Similar to `getAllWatcherDataObjects`, but will only return the already existing registered watchers in this process.
*/
getAllExistingWatchersDataObjects() {
if (!gAllWatcherData) {
return [];
}
return gAllWatcherData.values();
},
/**
* Get the watcher data object for a given watcher actor.
*

View File

@ -498,7 +498,7 @@ export class DevToolsProcessChild extends JSProcessActorChild {
didDestroy() {
// Unregister all the active watchers.
// This will destroy all the active target actors and unregister the target observers.
for (const watcherDataObject of ContentProcessWatcherRegistry.getAllWatchersDataObjects()) {
for (const watcherDataObject of ContentProcessWatcherRegistry.getAllExistingWatchersDataObjects()) {
this.#destroyWatcher(watcherDataObject);
}

View File

@ -64,8 +64,23 @@ class ResourceCommand {
// we don't have listeners registered twice.
this._offTargetFrontListeners = new Map();
// @backward-compat { version 124 } The server started throttling ressource emission
// Once 124 is released, we can always throttle to 0.
// We might also remove the throttle usage entirely as it may not have a significant impact anymore.
const throttleDelay =
commands.client.mainRoot.traits.throttledResources &&
!Services.prefs.getBoolPref(
"devtools.client-side-throttling.enable",
false
)
? 0
: 100;
this._notifyWatchers = this._notifyWatchers.bind(this);
this._throttledNotifyWatchers = throttle(this._notifyWatchers, 100);
this._throttledNotifyWatchers = throttle(
this._notifyWatchers,
throttleDelay
);
}
get watcherFront() {

View File

@ -35,6 +35,19 @@ add_task(async () => {
`await fetch("${EXAMPLE_DOMAIN}/request2.html", { method: "GET" });`,
]);
info("Wait for initial message resources");
await waitFor(
() =>
resourceCommand.getAllResources(resourceCommand.TYPES.CONSOLE_MESSAGE)
.length == 3
);
info("Wait for initial network resources");
await waitFor(
() =>
resourceCommand.getAllResources(resourceCommand.TYPES.NETWORK_EVENT)
.length == 2
);
assertNoOfResources(resourceCommand, 3, 2);
info("Clear the network event resources");

View File

@ -39,6 +39,12 @@ add_task(async function () {
await resourceCommand.watchResources([resourceCommand.TYPES.CSS_CHANGE], {
onAvailable: resources => availableResources.push(...resources),
});
// There is no guarantee that the CSS change will be already available when calling watchResources,
// so wait for it to be received.
info("Wait for CSS change to be received");
await waitFor(() => availableResources.length == 1);
assertResource(
availableResources[0],
{ index: 0, property: "color", value: "black" },

View File

@ -13,6 +13,7 @@ add_task(async function () {
await testBfCacheNavigation();
await testDomCompleteWithWindowStop();
await testCrossOriginNavigation();
await testDomCompleteWithOfflineDocument();
});
async function testDocumentEventResources() {
@ -574,6 +575,45 @@ async function testDomCompleteWithWindowStop() {
await client.close();
}
async function testDomCompleteWithOfflineDocument() {
info("Test dom-complete with an offline page");
const tab = await addTab(`${URL_ROOT_SSL}empty.html`);
const { commands, client, resourceCommand, targetCommand } =
await initResourceCommand(tab);
info("Check that all DOCUMENT_EVENTS are fired for the already loaded page");
let documentEvents = [];
await resourceCommand.watchResources([resourceCommand.TYPES.DOCUMENT_EVENT], {
onAvailable: resources => documentEvents.push(...resources),
});
is(documentEvents.length, 3, "Existing document events are fired");
documentEvents = [];
const targetBeforeNavigation = commands.targetCommand.targetFront;
tab.linkedBrowser.browsingContext.forceOffline = true;
gBrowser.reloadTab(tab);
// The offline mode may break Document Event Watcher and we would miss some of the expected events
info(
"Wait for will-navigate, dom-loading, dom-interactive and dom-complete events"
);
await waitFor(() => documentEvents.length === 4);
// Only will-navigate will have a valid timestamp, as the page is failing loading
// and we get the offline notice page, the other events will be set to 0
assertEvents({
commands,
targetBeforeNavigation,
documentEvents,
ignoreAllTimestamps: true,
});
targetCommand.destroy();
await client.close();
}
async function assertPromises(
commands,
targetBeforeNavigation,
@ -605,6 +645,7 @@ function assertEvents({
expectedTargetFront = commands.targetCommand.targetFront,
expectedNewURI = gBrowser.selectedBrowser.currentURI.spec,
ignoreWillNavigateTimestamp = false,
ignoreAllTimestamps = false,
}) {
const [willNavigateEvent, loadingEvent, interactiveEvent, completeEvent] =
documentEvents;
@ -651,23 +692,26 @@ function assertEvents({
`Type of time attribute for complete event is correct (${completeEvent.time})`
);
if (willNavigateEvent && !ignoreWillNavigateTimestamp) {
// In case of errors the timestamps may be set to 0.
if (!ignoreAllTimestamps) {
if (willNavigateEvent && !ignoreWillNavigateTimestamp) {
Assert.lessOrEqual(
willNavigateEvent.time,
loadingEvent.time,
`Timestamp for dom-loading event is greater than will-navigate event (${willNavigateEvent.time} <= ${loadingEvent.time})`
);
}
Assert.lessOrEqual(
willNavigateEvent.time,
loadingEvent.time,
`Timestamp for dom-loading event is greater than will-navigate event (${willNavigateEvent.time} <= ${loadingEvent.time})`
interactiveEvent.time,
`Timestamp for interactive event is greater than loading event (${loadingEvent.time} <= ${interactiveEvent.time})`
);
Assert.lessOrEqual(
interactiveEvent.time,
completeEvent.time,
`Timestamp for complete event is greater than interactive event (${interactiveEvent.time} <= ${completeEvent.time}).`
);
}
Assert.lessOrEqual(
loadingEvent.time,
interactiveEvent.time,
`Timestamp for interactive event is greater than loading event (${loadingEvent.time} <= ${interactiveEvent.time})`
);
Assert.lessOrEqual(
interactiveEvent.time,
completeEvent.time,
`Timestamp for complete event is greater than interactive event (${interactiveEvent.time} <= ${completeEvent.time}).`
);
if (willNavigateEvent) {
// If we switched to a new target, this target will be different from currentTargetFront.

View File

@ -148,6 +148,10 @@ add_task(async function testParentProcessRequests() {
IMAGE_URI,
"The third resource is for the second image request"
);
await waitFor(
() => secondImageRequest.fromCache,
"Wait for fromCache attribute to be set asynchronously via a resource update"
);
ok(secondImageRequest.fromCache, "The second image request is cached");
ok(
secondImageRequest.chromeContext,

View File

@ -4,6 +4,9 @@
"use strict";
// setImmediate is only defined when running in the worker thread
/* globals setImmediate */
/**
* From underscore's `_.throttle`
* http://underscorejs.org
@ -39,20 +42,32 @@ function throttle(func, wait, scope) {
const remaining = wait - (now - previous);
args = arguments;
if (remaining <= 0) {
clearTimeout(timeout);
if (!isWorker) {
clearTimeout(timeout);
}
timeout = null;
previous = now;
result = func.apply(scope, args);
args = null;
} else if (!timeout) {
timeout = setTimeout(later, remaining);
// On worker thread, we don't have access to privileged setTimeout/clearTimeout
// API which wouldn't be frozen when the worker is paused. So rely on the privileged
// setImmediate function which executes on the next event loop.
if (isWorker) {
setImmediate(later);
timeout = true;
} else {
timeout = setTimeout(later, remaining);
}
}
return result;
};
function cancel() {
if (timeout) {
clearTimeout(timeout);
if (!isWorker) {
clearTimeout(timeout);
}
timeout = null;
}
previous = 0;

View File

@ -4051,6 +4051,10 @@ pref("devtools.errorconsole.deprecation_warnings", true);
// Disable service worker debugging on all channels (see Bug 1651605).
pref("devtools.debugger.features.windowless-service-workers", false);
// Bug 1824726 replaced client side throttling with server side throttling.
// Use a preference in order to rollback in case of trouble.
pref("devtools.client-side-throttling.enable", false);
// Disable remote debugging protocol logging.
pref("devtools.debugger.log", false);
pref("devtools.debugger.log.verbose", false);

View File

@ -27,7 +27,6 @@ const {
step,
waitForSource,
waitForText,
evalInFrame,
waitUntil,
addBreakpoint,
waitForPaused,
@ -60,10 +59,6 @@ module.exports = async function () {
dump("Creating context\n");
const dbg = await createContext(panel);
// Note that all sources added via eval, and all sources added by this function
// will be gone when reloading the page in the next step.
await testAddingSources(dbg, tab, toolbox);
// Reselect App.js as that's the source expected to be selected after page reload
await selectSource(dbg, EXPECTED.file);
@ -319,44 +314,3 @@ async function testPrettyPrint(dbg, toolbox) {
await garbageCollect();
}
async function testAddingSources(dbg, tab, toolbox) {
// Before running the test, select an existing source in the two folders
// where we add sources so that the added sources are made visible in the SourceTree.
await selectSource(dbg, "js/testfile.js?id=0");
await selectSource(dbg, "js/subfolder/testsubfolder.js");
// Disabled ResourceCommand throttling so that the source notified by the server
// is immediately processed by the client and we process each new source quickly.
// Otherwise each source processing is faster than the throttling and we would mostly measure the throttling.
toolbox.commands.resourceCommand.throttlingDisabled = true;
const test = runTest("custom.jsdebugger.adding-sources.DAMP");
for (let i = 0; i < 15; i++) {
// Load source from two distinct folders to extend coverage around the source tree
const sourceFilename =
(i % 2 == 0 ? "testfile.js" : "testsubfolder.js") + "?dynamic-" + i;
const sourcePath =
i % 2 == 0 ? sourceFilename : "subfolder/" + sourceFilename;
await evalInFrame(
tab,
`
const script = document.createElement("script");
script.src = "./js/${sourcePath}";
document.body.appendChild(script);
`
);
dump(`Wait for new source '${sourceFilename}'\n`);
// Wait for the source to be in the redux store to avoid executing expensive DOM selectors.
await waitUntil(() => findSource(dbg, sourceFilename));
await waitUntil(() => {
return Array.from(
dbg.win.document.querySelectorAll(".sources-list .tree-node")
).some(e => e.textContent.includes(sourceFilename));
});
}
test.done();
toolbox.commands.resourceCommand.throttlingDisabled = false;
}