1580255 - Update Search panel results when filtering happens

Status: NEW

(DevTools :: Netmonitor, enhancement, P3)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

The search panel content should be automatically synced/updated as the main list of requests is filtered.

This should include both:

• Filter buttons available on Net panel's toolbar
• Free text filter also available on the toolbar

Honza

Comment 1

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: search-update-prototype.diff

Attaching a quick prototype showing the direction when fixing this bug.

But, there is an issue to solve:
The WorkerDispatcher(); ctor is executed twice since the module is loaded twice - using two different loaders

1. BrowserLoader: https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/devtools/client/netmonitor/initializer.js#16
2. DevToolsLoader is used when opening the Network panel: https://searchfox.org/mozilla-central/rev/a777ff11b6d700a698c61e5bd17e73b044304494/devtools/client/framework/toolbox.js#2328

This means that we have two instances of the worker dispatcher, but only one is started here:
https://searchfox.org/mozilla-central/rev/588814f2edddf0e132d77d326ddae50911e8bad1/devtools/client/netmonitor/src/app.js#58

STR:

1. Apply the patch run Firefox
2. Open the Network panel
3. See the Browser console for captured stack traces.

So, when the search action is triggered from within the searchMiddleware (to update the search results when request list filtering happens) second loader scope that doesn't started the worker is used - and the WorkerDispatcher doesn't have valid this.worker instance.

STR:

1. Load a page
2. Perform search using the Search panel
3. Filter the request list
4. Watch the Browser console for console.error("No worker!"); (coming from WorkerDispatcher)

Honza

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Julian, do you know more about the two-loaders issue described in the previous comment and how to solve that?

Basically there are modules in the Network monitor code base loaded twice since we are using two different loaders.

Honza

Comment 3

[Julian Descottes [:jdescottes]]

Thanks for the ping, this looks like an interesting loader issue!

tldr: move the worker outside of netmonitor for now so that we always use the DevTools loader and only have one copy of the module. But we need to fix the duplicated loads in a follow up, this is pretty bad!

Each loader has its own sandbox, so if you load moduleA with Loader1 and Loader2, the module will be loaded twice.

Note that BrowserLoader(s) are special because if the required path is not under baseURI then it will load the module using the global devtools loader. For the netmonitor, the baseURI is "devtools/client/netmonitor/". So when loading a module from DevTools Loader and from netmonitor BrowserLoader:

• if you load "devtools/shared/some-shared-module" -> the module will only be loaded once, you will get the same copy in both cases
• if you load "devtools/client/netmonitor/some-netmonitor-module" -> the module will be loaded twice

Ideally panels should stick to only one loader. As you pointed out, the netmonitor initialization sequence mixes two loaders:

• devtools/client/netmonitor/src/api is loaded from DevTools Loader (by the toolbox)
• devtools/client/netmonitor/src/app is loaded from the BrowserLoader (by the initializer)

The worker is started from app.js (BrowserLoader version), but we will use it from the search action. But the search action is also loaded twice. Once from a component (BrowserLoader loads app.js loads react loads components etc...), once from the search middleware (DevTools loader loads api.js creates the store creates the middlewares etc...). And that's the case for many modules. I quickly tried to stop loading api.js from the DevTools loader, and there is a big impact: we go from loading 102 modules to 58 modules. This means 44 modules get loaded twice because api.js is loaded via the DevTools loader instead of the BrowserLoader.

I think we didn't have the metrics tests when https://bugzilla.mozilla.org/show_bug.cgi?id=1436665 landed, otherwise we would have spotted such a regression.

Because there are so many modules duplicated I don't think there is a good "simple" fix. The simplest solution for the worker, will be to move its module outside of the netmonitor folder, so that we always use the DevTools Loader to load the module. The BrowserLoader will fallback on the DevTools loader since the module is outside of its baseURI. We can do this as a temporary fix for this bug.

But in the long run we really need to stop loading everything twice. api.js does many things which are only for the panel: creates the store, creates middlewares, loads actions etc... From it we should extract a module that only exposes the external API. It shouldn't care about anything else. It should have as little dependencies as possible. And this file should be the only thing loaded by the toolbox, from the DevTools loader. All the rest should remain under the BrowserLoader. We should file a follow up for that.

I think we should file another followup to have tests that check if we have duplicated modules between BrowserLoaders and the devtools Loader. The metrics tests should be a good entry point for that.

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Attachment: Force DevTools loader for specific dirs.

Thanks for detailed description of the issue Julian!

One comment related to the workaround for this bug. I'd like to avoid moving the search worker outside of the netmonitor dir (smells like a mess). I don't have perfect solution, but what about the following?

https://bugzilla.mozilla.org/attachment.cgi?id=9092678

Comment 5

[Jan Honza Odvarko [:Honza] (always need-info? me)]

NI

Comment 6

[Julian Descottes [:jdescottes]]

As a temporary fix, I think your suggestion is ok as well!