1410921 - All BrowserLoader instances in panels should be given a commonLibRequire argument

Status: NEW

(DevTools :: General, defect, P3)

Description

[Alexandre Poirot [:ochameau]]

This was regressed by bug 1350215 (netmonitor) and bug 1402262 (console).

See bug 1333131 comment 3 which highlights the cost of dupliated React instances.

Toolbox already need to load React and various other significant 3rd party libraries.

In order to reuse these instances, BrowserLoader supports "commonLibRequire" argument which allow to share some modules.
As of today, it is only used by old Console frontend:
http://searchfox.org/mozilla-central/rev/8a6a6bef7c54425970aa4fb039cc6463a19c0b7f/devtools/client/webconsole/webconsole.js#232
But it should be used in every BrowserLoader instance. Except the toolbox one and code that lives outside of a toolbox like about:debugging, RDM or panels loaded in a tab.

This is missing in many places:
http://searchfox.org/mozilla-central/search?q=BrowserLoader(&case=false&regexp=false&path=
* animation inspector
* debugger
* dom
* memory
* netmonitor
* performance
* new webconsole frontend

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1410921 - Fix netmonitor and webconsole URLs in react-dom monkey patch. ?

Review commit: https://reviewboard.mozilla.org/r/192070/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192070/

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1410921 - Reuse toolbox modules from the new console frontend.

Review commit: https://reviewboard.mozilla.org/r/192072/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192072/

Comment 3

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1410921 - Reuse toolbox modules from netmonitor panel.

Review commit: https://reviewboard.mozilla.org/r/192074/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/192074/

Comment 4

[Alexandre Poirot [:ochameau]]

Comment on attachment 8921106 [details]
Bug 1410921 - Fix netmonitor and webconsole URLs in react-dom monkey patch. ?

The URLs were outdated in react-dom patch. This reintroduces issues with DOM events. For example the filter bar doesn't toggle correctly.

Comment 5

[Alexandre Poirot [:ochameau]]

Let's see what DAMP says about this.

Just pushed to try, so still waiting for try color:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a333d65451d01212490ae3d4354a3782253de45d
and DAMP results:
https://treeherder.mozilla.org/perf.html#/comparechooser?newProject=try&newRevision=a333d65451d01212490ae3d4354a3782253de45d

Comment 6

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

We may want to add a test to ensure this behavior is preserved, so we can avoid regressing it (again) in the future.

Comment 7

[Alexandre Poirot [:ochameau]]

Ah. This becomes interesting.
DAMP reports significant regression.
I'm wondering if it is:
* cross compartment wrappers between toolbox and panel documents?
  We end up using toolbox's React which comes from another document and so I imagine there may be wrappers here.
* cost of the monkey patch of ReactDOM?
  This patch involves Proxy, if this is in a React hot codepath, it must be slow:
  http://searchfox.org/mozilla-central/source/devtools/client/shared/vendor/REACT_UPGRADING.md#patching-buildreact-dom.js

Comment 8

[Alexandre Poirot [:ochameau]]

It may be interesting to revisit this as the monkey patch has been removed.

Also, Julian recently highlighted that the inspector is doing something special, that can be replaced with a dedicated BrowserLoader using `commonLibRequire`.
Instead of that, it requires all react and component modules via toolbox's BrowserLoader instance, via toolbox.browserRequire/inspectorPanel.browserRequire:
  https://searchfox.org/mozilla-central/search?q=browserRequire(&case=false&regexp=false&path=%2Finspector%2F

(The toolbox has its own BrowserLoader instanciated here:
 https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/devtools/client/framework/toolbox.js#431)

Comment 9

[Alexandre Poirot [:ochameau]]

As of today, `commonLibRequire` is no longer used anywhere.

It has been removed from console without clear reason in bug 1402262.

The inspector still uses toolbox loader to share react instances as described in comment 8.

Comment 10

[Patrick Brosset <:pbro>]

Moving this inactive P2 to the backlog (P3)