Closed Bug 1620280 Opened 5 years ago Closed 4 years ago

Use the ResourceWatcher API to fetch Sources

Categories

(DevTools :: Debugger, enhancement, P2)

enhancement

Tracking

(Fission Milestone:M7, firefox83 fixed)

RESOLVED FIXED
83 Branch
Fission Milestone M7
Tracking Status
firefox83 --- fixed

People

(Reporter: ochameau, Assigned: ochameau)

References

(Blocks 3 open bugs)

Details

(Whiteboard: dt-fission-m2-mvp)

Attachments

(10 files)

(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details
(deleted), text/x-phabricator-request
Details

Bug 157662 introduced the ResourceWatcher API, accessible via toolbox.resourceWatcher. This API will help listen to data that is being created early, when the document just started loading.

We should migrate the whole DevTools codebase to this API for any data that:

  • DevTools frontend listen to, or care about,
    and,
  • may be created or be notified early, when the document just starts being loaded.
    This data will typically be: console messages, errors, warnings, sources, Root element NodeFront, storage values, network events, stylesheets, ...

We are typically not going to use this API for:

  • data being fetched on-demand, from user's input. For ex: webconsole evaluation input, or, DOM element expands from the Markup view.
  • events which we only want to record when the user cares about them. For ex: animation events.

For some more high level context, please have a look at Migration to Fission-compatible APIs, which describes all Fission-related refactorings.

The typical task for this bug will be about migrating code that:

  • start listening and register a RDP event listener,
  • retrieve already existings data,
    from panel's codebase, to the ResourceWatcher module, in the LegacyListener object.
    And then, the panel should use the ResourceWatcher instead.

Bug 1620234 is a good example of such migration, applied to Console Messages.
Bug 1623699 is also useful example as it demonstrates how to write tests for such migration.

This bug is about focusing on only one usecase in the debugger: the JS sources.

JS Sources are currently being listened via newSource RDP event from here:
https://searchfox.org/mozilla-central/rev/9c6e7500c0015a2c60be7b1b888261d95095ce27/devtools/client/debugger/src/client/firefox/events.js#146
https://searchfox.org/mozilla-central/rev/9c6e7500c0015a2c60be7b1b888261d95095ce27/devtools/client/debugger/src/client/firefox/events.js#42
and already existing ones fetch via ThreadFront.sources(), from here:
https://searchfox.org/mozilla-central/rev/9c6e7500c0015a2c60be7b1b888261d95095ce27/devtools/client/debugger/src/client/firefox/commands.js#399

This work depends on bug 1626655 as ResourceWatcher depends on the TargetList and would only expose resource from targets being provided by the TargetList.

Blocks: 1626645
No longer blocks: 1576624
Depends on: 1626655
Priority: -- → P2
Whiteboard: dt-fission-m2-mvp
Summary: Use the Resources API to fetch sources → Use the ResourceWatcher API to fetch Sources

Tracking Fission DevTools bugs for Fission Nightly (M6) milestone

Fission Milestone: --- → M6

Tracking dt-fission-m2-mvp bugs for Fission Nightly milestone (M6c)

Fission Milestone: M6 → M6c
Blocks: 1644188

There should be no more blocker for working on this.

The big shift here is to stop using ThreadFront.getSources() + listening to ThreadFront.newSource, that, for each individual Target,
and instead listen from one single place for SOURCES resource.

So switch from:
https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/devtools/client/debugger/src/client/firefox/commands.js#430-437

async function fetchSources(): Promise<Array<GeneratedSourceData>> {
  let sources = [];
  for (const threadFront of getAllThreadFronts()) {
    sources = sources.concat(await getSources(threadFront));
  }
  return sources;
}

,and,
https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/devtools/client/debugger/src/client/firefox/commands.js#408-414

async function getSources(
  client: ThreadFront
): Promise<Array<GeneratedSourceData>> {
  const { sources }: SourcesPacket = await client.getSources();

  return sources.map(source => prepareSourcePayload(client, source));
}

,and,
https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/devtools/client/debugger/src/client/firefox/events.js#31-40

// This is called for each individual Target
function addThreadEventListeners(thread: ThreadFront): void {
  const removeListeners = [];
  Object.keys(clientEvents).forEach(eventName => {
    // EventEmitter.on returns a function that removes the event listener.
    removeListeners.push(
      thread.on(eventName, clientEvents[eventName].bind(null, thread))
    );
  });
  threadFrontListeners.set(thread, removeListeners);
}

,and,
https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/devtools/client/debugger/src/client/firefox/events.js#105-110

function newSource(threadFront: ThreadFront, { source }: SourcePacket): void {
  sourceQueue.queue({
    type: "generated",
    data: prepareSourcePayload(threadFront, source),
  });
}

to:

resourceWatcher.watchResource([SOURCE], {
  onAvailable({ resourceType, targetFront, resource }) => {
    // Process this new source which is into `resource` object, or "is" `resource`, 
    // depending on legacy listener implementation
  }
});

This should probably be done from where we currently watch the targets:
https://searchfox.org/mozilla-central/rev/31d8600b73dc85b4cdbabf45ac3f1a9c11700d8e/devtools/client/debugger/src/client/firefox.js#32-36

Assignee: nobody → poirot.alex
Status: NEW → ASSIGNED

I was expecting more trouble doing that. But it highlighted various issues with the current code.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d32a9a9d67990e08a039fb2846aa8324fa8cecee

I should probably write a test to assert the behaviour of sources.
I've seen some weird behavior between existing and new sources. I suspect we have bugs in productions related to that.
I also suspect we have tons of workaround in the debugger frontend to mitigate them.
The role of SourceQueue isn't clear for me.

I'll let bug 1657310 land first and rebase on top of it.

Depends on: 1657310
Depends on: 1660268
Depends on: 1663614

This was relevant before as we were calling "ThreadActor.getSources"/"StyleSheets.getStyleSheets",
and by the time these requests resolved, we would have navigated to a new URL and need to ignore their response.
I think it is safe to drop this now as these requests may still be pending if we use legacy listeners,
but the call to unwatchResources should immediately unregister _onResourceAvailable and ignore any pending data
coming late.

Some data coming from DAMP.

  • all patches but the last, removing SourceQueue
    • compare against m-c so this include all throttling patches and console perf fixes...
    • compare on top of throttling patches
      => improvements: 70% responsePanel, 4% bulklog, 29% stepIn, 5% exportHAR
      => regressions: 15-20% debugger.open, 2-5% debugger.reload, 2% inspector.reload, 7% console.reload
  • all patches, including the SourceQueue removal
    • compare against m-c
      ** this is the one I would like to highlight as it contain all my recent work **
      => improvements: 70% responsePanel, 4% bulklog
      => regressions: only on simple.* tests as well as close, and some very short irrelevant tests like collapseall.manychildren
      => So not very important regressions here.
    • compare on top of throttling patches
      => improvements: 70% responsePanel, 4% bulklog, 2% custom.debugger.reload
      => regressions: 2-4% inspector.reload, console.reload 8%, many regression of simple test, but that must be related to duplicated throttling
  • SourceQueue patch compared to all but this one patch
    => improvements: debugger 11-17% on open and 5-19% reload
    => regressions: 33% stepIn, 2% pause, 3% inspector.reload
Blocks: 1665712
Blocks: 1665716
Blocks: 1667026

With bug 1620280's patches, browser_dbg-windowless-workers-early-breakpoint.js started failing
because of a removed breakpoint being re-added after being removed!
This relates to the usage of PROMISE in the action object,
which triggers the promise middleware, itself making the action being emitted twice.
One time immediately, and another time after the promise is resolved.
So that the reducer was adding the breakpoint twice.

Green and greener try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=810b99cfd045bf7b9e205b0cba93870e4e32c31b
Still two suspicious failure, which sounds very unrelated:

  • devtools/client/inspector/animation/test/browser_animation_animation-target_select.js
  • devtools/client/inspector/rules/test/browser_rules_edit-selector-click.js
Attachment #9169775 - Attachment description: Bug 1620280 - Implement and use a SOURCE resource. → Bug 1620280 - [devtools] Implement and use a SOURCE resource.
Attachment #9172192 - Attachment description: Bug 1620280 - Convert SourceMapService to use SOURCE resource. → Bug 1620280 - [devtools] Convert SourceMapService to use SOURCE resource.
Attachment #9176069 - Attachment description: Bug 1620280 - Ensure calling unwatchResources. → Bug 1620280 - [devtools] Ensure calling unwatchResources.
Attachment #9176070 - Attachment description: Bug 1620280 - Simplify things around sourceLoading promise. → Bug 1620280 - [devtools] Simplify things around sourceLoading promise.
Attachment #9176071 - Attachment description: Bug 1620280 - Wait for source actor when fetching frames. → Bug 1620280 - [devtools] Wait for source actor when fetching frames.
Attachment #9176072 - Attachment description: Bug 1620280 - Bypass SourceQueue as it should be made redundant with ResourceWatcher throttling. → Bug 1620280 - [devtools] Bypass SourceQueue as it should be made redundant with ResourceWatcher throttling.
Attachment #9178424 - Attachment description: Bug 1620280 - Fix duplicated SET_BREAKPOINT action handling. → Bug 1620280 - [devtools] Fix duplicated SET_BREAKPOINT action handling.
Attachment #9178662 - Attachment description: Bug 1620280 - Prevent showing/hiding highlighters when the panels are already destroyed. → Bug 1620280 - [devtools] Prevent showing/hiding highlighters when the panels are already destroyed.

Without that, we are closing the toolbox by destroying its tab.
This prevent correctly waiting for toolbox full destruction and was causing leaks
in debug builds.

Attachment #9178704 - Attachment description: Bug 1620280 - [devtools] Explicitely wait for toolbox destruction in about:debugging test. → Bug 1620280 - [devtools] Explicitely wait for toolbox destruction in a few tests.
Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/9cffe1c5fb6d [devtools] Implement and use a SOURCE resource. r=jdescottes,bomsy,nchevobbe https://hg.mozilla.org/integration/autoland/rev/488f645884ba [devtools] Convert SourceMapService to use SOURCE resource. r=nchevobbe,jdescottes https://hg.mozilla.org/integration/autoland/rev/526c067da0c1 [devtools] Ensure calling unwatchResources. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/47a2d6b77270 [devtools] Simplify things around sourceLoading promise. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/6a8126ded6ec [devtools] Wait for source actor when fetching frames. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/74aba83895ce [devtools] Bypass SourceQueue as it should be made redundant with ResourceWatcher throttling. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/78a06cd336c6 [devtools] Fix duplicated SET_BREAKPOINT action handling. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/309c0be48745 [devtools] Prevent showing/hiding highlighters when the panels are already destroyed. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/8dda1f048067 [devtools] Explicitely wait for toolbox destruction in a few tests. r=jdescottes

Bulk move of all m2-mvp devtools bugs to Fission M7.

Fission Milestone: M6c → M7

browser_ext_devtools_inspectedWindow_targetSwitch.js has top level targets that are destroyed while
the test runs. This makes the sources request to fail with an exception because when a target
is destroyed, we "purge" any pending request and make them throw/reject them.

This isn't super elegant, but the idea is that the server side implementation won't have this issue.

Pushed by apoirot@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/96b97978b8b3 [devtools] Implement and use a SOURCE resource. r=jdescottes,bomsy,nchevobbe https://hg.mozilla.org/integration/autoland/rev/89df82dd69d3 [devtools] Convert SourceMapService to use SOURCE resource. r=nchevobbe,jdescottes https://hg.mozilla.org/integration/autoland/rev/6b01b539bc4f [devtools] Ensure calling unwatchResources. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/d15cdba181c9 [devtools] Simplify things around sourceLoading promise. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/ef7c45b795d5 [devtools] Wait for source actor when fetching frames. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/9aa803fd8968 [devtools] Bypass SourceQueue as it should be made redundant with ResourceWatcher throttling. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/b6c0dde619bc [devtools] Fix duplicated SET_BREAKPOINT action handling. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/077867b8be26 [devtools] Prevent showing/hiding highlighters when the panels are already destroyed. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/638f910d98ae [devtools] Explicitely wait for toolbox destruction in a few tests. r=jdescottes https://hg.mozilla.org/integration/autoland/rev/a1cb07833e35 [devtools] Workaround browser_ext_devtools_inspectedWindow_targetSwitch.js intermittent failure. r=jdescottes
Regressions: 1678254
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: