1620280 - Use the ResourceWatcher API to fetch Sources

Status: RESOLVED FIXED

(DevTools :: Debugger, enhancement, P2)

Description

[Alexandre Poirot [:ochameau]]

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

Comment 1

[Alexandre Poirot [:ochameau]]

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

Comment 2

[Chris Peterson [:cpeterson]]

Tracking Fission DevTools bugs for Fission Nightly (M6) milestone

Comment 3

[Chris Peterson [:cpeterson]]

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

Comment 4

[Alexandre Poirot [:ochameau]]

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

Comment 5

[Alexandre Poirot [:ochameau]]

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.

Comment 6

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620280 - [devtools] Implement and use a SOURCE resource.

Comment 7

[Alexandre Poirot [:ochameau]]

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

Comment 8

[Alexandre Poirot [:ochameau]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2938114183d050163413b50af522bd8c9ecf8ad8

Comment 9

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620280 - [devtools] Convert SourceMapService to use SOURCE resource.

Comment 10

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620280 - [devtools] Ensure calling unwatchResources.

Comment 11

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620280 - [devtools] Simplify things around sourceLoading promise.

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.

Comment 12

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620280 - [devtools] Wait for source actor when fetching frames.

Comment 13

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620280 - [devtools] Bypass SourceQueue as it should be made redundant with ResourceWatcher throttling.

Comment 14

[Alexandre Poirot [:ochameau]]

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

Comment 15

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620280 - [devtools] Fix duplicated SET_BREAKPOINT action handling.

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.

Comment 16

[Alexandre Poirot [:ochameau]]

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

Comment 17

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620280 - [devtools] Prevent showing/hiding highlighters when the panels are already destroyed.

Comment 18

[Alexandre Poirot [:ochameau]]

I think this one will be green with the last patch:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=0d50878b3ccc35975f7fedc1d81bfa79365f2c91

Comment 19

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620280 - [devtools] Explicitely wait for toolbox destruction in a few tests.

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.

Comment 20

[Pulsebot]

Pushed by [e-mail]:
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

Comment 21

[Neha Kochar [:neha]]

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

Comment 22

[Atila Butkovits]

Backed outfor failure at browser_ext_devtools_inspectedWindow_targetSwitch.js.

backout link: https://hg.mozilla.org/integration/autoland/rev/298e7e8aa7b77bba617a69ea81bd43b898b953e7

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&searchStr=linux%2C18.04%2Cx64%2Cdebug%2Cmochitests%2Ctest-linux1804-64%2Fdebug-mochitest-browser-chrome-e10s%2Cbc15&revision=8dda1f0480677499129a9dce312d6e13a966de62&selectedTaskRun=M0AgHgNXQaidAbqeZCeOtA.0

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=317309705&repo=autoland&lineNumber=7742

Comment 23

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1620280 - [devtools] Workaround browser_ext_devtools_inspectedWindow_targetSwitch.js intermittent failure.

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.

Comment 24

[Alexandre Poirot [:ochameau]]

New try run with the latest patch:
https://treeherder.mozilla.org/#/jobs?repo=try&test_paths=browser%2Fcomponents%2Fextensions%2Ftest%2Fbrowser%2F&revision=e4e4ed45258b18e02a4f1f0ed3b886e675f8a226

Comment 25

[Pulsebot]

Pushed by [e-mail]:
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

Comment 26

[Narcis Beleuzu [:NarcisB]]

https://hg.mozilla.org/mozilla-central/rev/96b97978b8b3
https://hg.mozilla.org/mozilla-central/rev/89df82dd69d3
https://hg.mozilla.org/mozilla-central/rev/6b01b539bc4f
https://hg.mozilla.org/mozilla-central/rev/d15cdba181c9
https://hg.mozilla.org/mozilla-central/rev/ef7c45b795d5
https://hg.mozilla.org/mozilla-central/rev/9aa803fd8968
https://hg.mozilla.org/mozilla-central/rev/b6c0dde619bc
https://hg.mozilla.org/mozilla-central/rev/077867b8be26
https://hg.mozilla.org/mozilla-central/rev/638f910d98ae
https://hg.mozilla.org/mozilla-central/rev/a1cb07833e35