1807896 - Reducer number of events between parent process code and soon-to-be privileged content proces toolbox

Status: RESOLVED FIXED

(DevTools :: Framework, task)

Description

[Alexandre Poirot [:ochameau]]

• "toolbox-unload"
  Is fired in middle of the destruction codepath, near the end:
  https://searchfox.org/mozilla-central/rev/8fd39dc89c68501a354446a8681ec8263d15e5aa/devtools/client/framework/toolbox.js#4171
  And listened by the ToolboxHostManager:
  https://searchfox.org/mozilla-central/rev/8fd39dc89c68501a354446a8681ec8263d15e5aa/devtools/client/framework/toolbox-host-manager.js#117
  to destroy itself, with a delay.
  Here it looks like we rather want to wait for full toolbox destruction (given the delay), so it makes sense to try to listen to "toolbox-destroyed". Which is fired the latest.

• "toolbox-created"
  Was added only for a test. It would most likely change drasticaly once we move the toolbox OOP. It's actual precise time of firing will most likely no longer be relevant for the test verifying a race condition in many toggling of a unique toolbox.

• "toolbox-destroy"
  Is only used from WebExt codebase over there:
  https://searchfox.org/mozilla-central/rev/8fd39dc89c68501a354446a8681ec8263d15e5aa/browser/components/extensions/parent/ext-devtools.js#377
  It can't easily move to toolbox-destruction as it queries toolbox.commands, which is already nullified when this event is fired.
  Toolbox.commands will most likely no longer exists once the toolbox runs in another process, so we might want to early revisit this implementation.

Comment 1

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1807896 - [devtools] Move manager destruction from toolbox-unload to destroyed.

This toolbox-unload was only fired for ToolboxHostManager own usage.
On top of that, ToolBoxHostManager was delaying its destruction by an additional
event loop. It indicates that this toolbox-unload event was too early.
So it rather makes sense to use the latest event in the destruction codepath.
i.e. toolbox's "destroyed" event.

I kept the event loop tick as toolbox-destroyed is still slightly too early.
The debugger is still going through pending async work in browser_dbg-breakpoints-in-evaled-sources.js.
This relates to the promise middleware which delay each promise action
by an event loop tick using setTimeout. setTimeout throws once we remove the host iframe.

I first tried to remove this event loop tick.
It highlighted two possible intermittents in two tests involving viewSourceInDebugger.
These tests weren't waiting for the full processing of opened sources by the Debugger frontend.
Using debugger shared-head helpers ensure waiting for full source selection.

Comment 2

[Alexandre Poirot [:ochameau]]

Attachment: Bug 1807896 - [devtools] Remove "toolbox-created" event.

This event was only used in this test.

Unfortunately, I wasn't able to keep the same logic in the test while keeping it reliable.
I prefer to remove this test-only event and simplify the test to only ensure
we safely ignore subsequent calls to toggle.

What is hard to know from the test, when calling toggle in some following event loops
is if this will be ignored -or- close/open a toolbox...

Comment 3

[Alexandre Poirot [:ochameau]]

Greenish try run:
https://treeherder.mozilla.org/jobs?repo=try&revision=d6c3c0850c972cb41cdb1caaba2424126d4c3910

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8ddfebd080fb
[devtools] Move manager destruction from toolbox-unload to destroyed. r=devtools-reviewers,bomsy
https://hg.mozilla.org/integration/autoland/rev/9e752ce5baed
[devtools] Remove "toolbox-created" event. r=devtools-reviewers,perftest-reviewers,sparky,jdescottes

Comment 5

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/8ddfebd080fb
https://hg.mozilla.org/mozilla-central/rev/9e752ce5baed