1716960 - browser_toolbox_window_reload_target.js fails with server side target switching enabled

Status: RESOLVED FIXED

(DevTools :: Framework, defect)

Description

[Alexandre Poirot [:ochameau]]

This test fails with at least two distinct failures:

./mach mochitest devtools/client/framework/test/browser_toolbox_window_reload_target.js --setpref devtools.target-switching.server.enabled=true

 2:23.47 INFO Select tool inspector
 2:23.54 GECKO(81974) console.error: "Error when attaching target:" (new Error("Connection closed, pending request to server0.conn1.windowGlobal2147483658/consoleActor3, type startListeners failed\n\nRequest stack:\nrequest@resource://devtools/shared/protocol/Front.js:292:14\ngenerateRequestMethods/</frontProto[name]@resource://devtools/shared/protocol/Front/FrontClassWithSpec.js:46:19\nstartListeners@resource://devtools/client/fronts/webconsole.js:310:34\nattachConsole@resource://devtools/client/fronts/targets/target-mixin.js:461:26\nasync*attach/this._attach<@resource://devtools/client/fronts/targets/browsing-context.js:133:20\nasync*attach@resource://devtools/client/fronts/targets/browsing-context.js:135:7\n_attachAndInitThread@resource://devtools/client/fronts/targets/target-mixin.js:506:20\nattachAndInitThread@resource://devtools/client/fronts/targets/target-mixin.js:485:40\n_onTargetAvailable@resource://devtools/shared/commands/target/target-command.js:198:25\n_emit@resource://devtools/shared/event-emitter.js:226:34\nemit@resource://devtools/shared/event-emitter.js:172:18\nemit@resource://devtools/shared/event-emitter.js:324:18\n_onTargetAvailable@resource://devtools/client/fronts/watcher.js:60:10\n_emit@resource://devtools/shared/event-emitter.js:226:34\nemit@resource://devtools/shared/event-emitter.js:172:18\nemit@resource://devtools/shared/event-emitter.js:324:18\nonPacket@resource://devtools/shared/protocol/Front.js:336:13\nonPacket@resource://devtools/client/devtools-client.js:482:13\nsend/<@resource://devtools/shared/transport/local-transport.js:68:25\nexports.makeInfallible/<@resource://devtools/shared/ThreadSafeDevToolsUtils.js:103:22\nDevToolsUtils.executeSoon*exports.executeSoon@resource://devtools/shared/DevToolsUtils.js:54:21\nsend@resource://devtools/shared/transport/local-transport.js:56:21\nsend@resource://devtools/server/devtools-server-connection.js:99:20\n_sendEvent@resource://devtools/shared/protocol/Actor.js:72:15\ninitialize/<@resource://devtools/shared/protocol/Actor.js:46:16\n_emit@resource://devtools/shared/event-emitter.js:226:34\nemit@resource://devtools/shared/event-emitter.js:172:18\nemit@resource://devtools/shared/event-emitter.js:324:18\nnotifyTargetAvailable@resource://devtools/server/actors/watcher.js:216:10\nconnectFromContent@resource://devtools/server/connectors/js-window-actor/DevToolsFrameParent.jsm:154:13\nreceiveMessage@resource://devtools/server/connectors/js-window-actor/DevToolsFrameParent.jsm:220:21\nJSActor query*_createTargetActor@resource://devtools/server/connectors/js-window-actor/DevToolsFrameChild.jsm:257:10\ninstantiate@resource://devtools/server/connectors/js-window-actor/DevToolsFrameChild.jsm:190:14\nhandleEvent@resource://devtools/server/connectors/js-window-actor/DevToolsFrameChild.jsm:505:12\n", "resource://devtools/shared/protocol/Front.js", 103))
 2:23.55 INFO Console message: [JavaScript Error: "InvalidStateError: JSWindowActorChild.sendAsyncMessage: JSWindowActorChild cannot send at the moment" {file: "resource://devtools/server/connectors/js-window-actor/DevToolsFrameChild.jsm" line: 367}]
 2:23.61 GECKO(81974) console.warn: "Listener for event 'frame' did not return a promise."
 2:23.69 GECKO(81974) get cssProperties@resource://devtools/client/inspector/inspector.js:341:36
 2:23.69 GECKO(81974) CssRuleView@resource://devtools/client/inspector/rules/rules.js:136:3
 2:23.69 GECKO(81974) RuleViewTool@resource://devtools/client/inspector/rules/rules.js:2004:15
 2:23.69 GECKO(81974) getPanel@resource://devtools/client/inspector/inspector.js:988:17
 2:23.69 GECKO(81974) addRuleView@resource://devtools/client/inspector/inspector.js:867:12
 2:23.69 GECKO(81974) setupSidebar@resource://devtools/client/inspector/inspector.js:1040:10
 2:23.69 GECKO(81974) _deferredOpen@resource://devtools/client/inspector/inspector.js:379:10
 2:23.69 GECKO(81974) init@resource://devtools/client/inspector/inspector.js:226:17
 2:23.69 GECKO(81974) async*open@resource://devtools/client/inspector/panel.js:12:28
 2:23.69 GECKO(81974) onLoad@resource://devtools/client/framework/toolbox.js:2538:27
 2:23.69 GECKO(81974) console.error: (new TypeError("can't access property \"cssProperties\", this._cssProperties is undefined", "resource://devtools/client/inspector/inspector.js", 342))

 0:08.37 INFO Now test with the toolbox undocked
 0:08.37 INFO Select tool options
 0:08.41 INFO Reload with toolbox.reload.key
 0:08.41 INFO Focus the toolbox window and emit the reload shortcut: CmdOrCtrl+R
 0:08.41 INFO Synthesizing key shortcut: CmdOrCtrl+R
 0:08.41 INFO Wait for page and toolbox reload promises: 1
 0:08.56 GECKO(82389)  => load
 0:08.56 INFO Reload with toolbox.reload2.key
 0:08.56 INFO Focus the toolbox window and emit the reload shortcut: F5
 0:08.57 INFO Synthesizing key shortcut: F5
 0:08.57 INFO Wait for page and toolbox reload promises
 0:08.74 GECKO(82389) console.warn: "Listener for event 'frame' did not return a promise."

This second one seems to be the most frequent one.

Comment 1

[Alexandre Poirot [:ochameau]]

This first issue about undefined _cssProperties comes from this code in target command:
https://searchfox.org/mozilla-central/rev/b172dd415c475e8b2899560e6005b3a953bead2a/devtools/shared/commands/target/target-command.js#598-618

      // It can happen that onAvailable was already called with this targetFront at
      // this time (via _onTargetAvailable). If that's the case, we don't want to call
      // onAvailable a second time.
      if (
        this._pendingWatchTargetInitialization &&
        this._pendingWatchTargetInitialization.has(onAvailable) &&
        !this._pendingWatchTargetInitialization
          .get(onAvailable)
          .has(targetFront)
      ) {
        return;
      }

      try {
        // Ensure waiting for eventual async create listeners
        // which may setup things regarding the existing targets
        // and listen callsite may care about the full initialization
        await onAvailable({
          targetFront,
          isTargetSwitching: false,
        });
      } catch (e)

The top level target is being created or is still being processed by TargetCommand.onTargetAvailable while we are calling TargetCommand.watchTargets.
Because of that, the onAvailable callback passed by the inspector UI:
https://searchfox.org/mozilla-central/source/devtools/client/inspector/inspector.js#205
is called by TargetCommand.onTargetAvailable and not by the code I just linked in TargetCommand.watchTargets. This is because of the _pendingWatchTargetInitialization logic.
Because of that, watchTargets resolves before the onAvailable callback completes. And this is why we have css properties not initialized when calling deferredOpen.

Comment 2

[Alexandre Poirot [:ochameau]]

Regarding the second failure, it comes from the reload request being done from the toolbox:
https://searchfox.org/mozilla-central/rev/b172dd415c475e8b2899560e6005b3a953bead2a/devtools/client/framework/toolbox.js#1109
Which fails around here:
https://searchfox.org/mozilla-central/rev/b172dd415c475e8b2899560e6005b3a953bead2a/devtools/shared/commands/target/target-command.js#771-781
The issue is that we try to reload while the current top level target is being destroyed or is already destroyed.
So either the targetFront here is already destroyed, or is destroyed while calling reload.
And the issue is that when it is destroyed while calling reload it did not always reached the target actor and we did not reload at all!

May be we should have an actor in the parent process to have a reliable reload method??

Comment 3

[Julian Descottes [:jdescottes]]

Would like to try this one.

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1716960 - [devtools] Move BrowsingContextTargetActor reload to descriptor actors

Depends on D118471

Move the BrowsingContextTargetActor::reload method to the TabDescriptorActor.
With this, calls to reload should no longer throw simply due to the Actor being destroyed while the request is processed, because descriptor actors stay alive across reloads and navigation.

The method is only added on the Tab descriptor as reload most likely doesn't make sense for parent process targets. WebExtension targets on the other hand already have a reload method available on the corresponding descriptor.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1716960 - [devtools] Reorder inspector initialization methods

Depends on D118473

This changeset is purely moving code around. The various inspector initialization methods are spread all over inspector.js, which makes it difficult to follow.
Here we try to sort them more or less in the order in which they should be invoked.

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1716960 - [devtools] Initialize the inspector UI only after handling the first root node

Depends on D118474

This patch improves the inspector initialization in order to make it more resilient to race issues which can occur with watchTargets.
watchTargets might resolve before all targets have been processed, in case some targets become available after the watchTargets call already started.

This should be fixed, either by removing the async nature of target initialization or by fully waiting on all potential targets discovered during a call to watchTargets.
The first option is currently blocked on other issues, and the second option might result in very complex logic.

Instead, this patch updates the inspector initialization so that we no longer attempt to call deferredOpen() right after calling watchTargets/watchResources.
The inspector will initialize as much UI as possible before getting target/root-node information, and when retrieving the first root-node it will then proceed to finalize the UI initialization. A lot of inspector UI (panels etc...) need to have fronts available to be initialized.

Note that this patch only waits on root-node. It might be better to wait both on a first target & on a first root-node.

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1716960 - [devtools] Wait for correct fontinspector-update event in font inspector tests

Depends on D118475

Unsurprisingly, updating the inspector initialization sequence makes a few tests intermittent.
font panel tests in particular seem to get a late event intermittently, and consume it incorrectly.
This patch updates one of the font inspector events to also emit the current node handled by the font inspector.

This way tests can now clearly wait for a specific font inspector event. The event is also emitted with emitForTests now as it is not used outside of our mochitests.

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1716960 - [devtools] Fix typo preventing inspector to unwatch reflows

Depends on D118476

Comment 9

[Phabricator Automation]

Comment on attachment 9228323 [details]
Bug 1716960 - [devtools] Move BrowsingContextTargetActor reload to descriptor actors

Revision D118473 was moved to bug 1718236. Setting attachment 9228323 [details] to obsolete.

Comment 10

[Phabricator Automation]

Comment on attachment 9228327 [details]
Bug 1716960 - [devtools] Wait for correct fontinspector-update event in font inspector tests

Revision D118476 was moved to bug 1718239. Setting attachment 9228327 [details] to obsolete.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6856487bb1fc
[devtools] Reorder inspector initialization methods r=nchevobbe
https://hg.mozilla.org/integration/autoland/rev/bde0b6ffd161
[devtools] Initialize the inspector UI only after handling the first root node r=ochameau,nchevobbe

Comment 12

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/6856487bb1fc
https://hg.mozilla.org/mozilla-central/rev/bde0b6ffd161