1611096 - Blank inspector when using target-switching and fission

Status: RESOLVED FIXED

(DevTools :: Inspector, task, P1)

Description

[Julian Descottes [:jdescottes]]

What were you doing?

1. set fission.autostart=true
2. set devtools.target-switching.enabled=true
3. restart Firefox
4. open google.com, open the inspector
5. navigate to mozilla.org

What happened?

Inspector will be blank with several errors in the stdout, mostly related to inspector-utils APIs called for an element that has apparently no ownerGlobal.

Spotted while reviewing https://bugzilla.mozilla.org/show_bug.cgi?id=1568874. It could be nice to fix the inspector a little bit (even if it's not perfect) it would make testing such patches easier. If the inspector is broken it's a bit hard to say if the sidebar panels support target-switching correctly or not.

For instance

"Error while calling actor 'domnode's method 'getAllSelectors'" "can't access property \"CSS\", ele.ownerGlobal is null"

What should have happened?

Target-switching for the inspector, with fission, never fully worked, even right after landing the initial support for it in Bug 1578242, but I think it's because the focus was rather on using targetlist apis, and maybe supporting the non-fission target switching (parent process <-> content process).

But most of the errors seem to come from the fact that we are trying to trigger the "onNewRoot" too soon on the inspector side. Looking at _onTargetAvailable, the method ends with

    if (this.isReady) {
      this.onNewRoot();
    }

If instead, we replace it with

    if (this.isReady) {
      this.walker.on("new-root", this.onNewRoot);
    }

The inspector seems to work better, at least when testing locally. Ideally, the walker would provide a way to know if the initial load is already done so that we can either call newRoot immediately or wait for the "new-root" event.

Anything else we should know?

The current comment above the onNewRoot call in _onTargetAvailable already mentions something similar, suggesting to share the code called in deferredOpen: https://searchfox.org/mozilla-central/rev/e878e5b81bb319c141900ce9cfcde732df5c8449/devtools/client/inspector/inspector.js#244-256

Comment 1

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Remove leftover usage of this._target in inspector.js

This was making the inspector destroy crash, if a target switch had occured before.

Comment 2

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Wait for new-root event in inspector for target switching

Depends on D60830

Comment 3

[Chris Peterson [:cpeterson]]

Tracking DevTools bugs for Fission Nightly (M6) milestone

Comment 4

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Remove deprecated trait retrieveNodeFromContentDomReference (Fx71)

Depends on D60831
Adding a new trait in the previous changeset so let's remove this one.

Comment 5

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Add test for inspector target switching

Depends on D60964

Comment 6

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Delete reflow front on target destroyed

This is often breaking target switching tests for the inspector. Depends on D62214

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Add watch-like API for new root event

Depends on D62216

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Remove new-root event from walker

Depends on D62217

Comment 9

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Merge inspector deferredOpen and onNewRoot initializers

Depends on D62218

Comment 10

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Do not fire new-root and reloaded events for the first root node

Depends on D62219

Comment 11

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Fix browser_toolbox_window_reload_target.js relying on newRoot mutation

Depends on D62220

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Stop waiting for markup loaded in iframe load test

Depends on D62221

Comment 13

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Add watch-like API for new root event

Depends on D62216

Comment 14

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Merge inspector deferredOpen and onNewRoot initializers

Depends on D62229

Comment 15

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Fix tests relying on newRoot mutation

Depends on D62230

Comment 16

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Add watch-like API for new root event (mutation version)

Depends on D62216
Same as D62229, but instead of introducing a new event, this preserves the new-root mutation approach

Comment 17

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Check webprogresslistener as a fallback for non loaded documents

Depends on D62229

Comment 18

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Remove leftover usage of this._target in inspector.js

Depends on D62576

Comment 19

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Remove deprecated trait retrieveNodeFromContentDomReference (Fx71)

Depends on D62616

Comment 20

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Delete reflow front on target destroyed

This is often breaking target switching tests for the inspector
Depends on D62617

Comment 21

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Add watch-like API for new root event

Depends on D62618

Comment 22

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Add test for inspector target switching

Depends on D62622

Comment 23

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Fix tests relying on newRoot mutations

Depends on D62623

Comment 24

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Check webprogresslistener as a fallback for non loaded documents

Depends on D62624

Comment 26

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Use walker actor events to watchRootNode/unwatchRootNode

Depends on D62625

This is a WIP patch. It uses walker actor events directly instead of forwarding in the walker front.
It also simplifies the API around the RootNode exposed by the WalkerFront (mostly removing the internal rootNode promise)

Comment 27

[Chris Peterson [:cpeterson]]

Tentatively moving P1 Fission M6 bugs to M6a.

Comment 28

[Phabricator Automation]

Comment on attachment 9126165 [details]
Bug 1611096 - Remove leftover usage of this._target in inspector.js

Revision D62616 was moved to bug 1628691. Setting attachment 9126165 [details] to obsolete.

Comment 29

[Phabricator Automation]

Comment on attachment 9126166 [details]
Bug 1611096 - Remove deprecated trait retrieveNodeFromContentDomReference (Fx71)

Revision D62617 was moved to bug 1628691. Setting attachment 9126166 [details] to obsolete.

Comment 30

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Always watch root node in walker actor

Depends on D62625

Comment 31

[Julian Descottes [:jdescottes]]

Attachment: Bug 1611096 - Call watchRootNode from getRootNode

Depends on D62625

Alternative to D73969

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3703fbfe567a
Add watch-like API for new root event r=ochameau,rcaliman
https://hg.mozilla.org/integration/autoland/rev/a7242f506aa6
Add test for inspector target switching r=ochameau,rcaliman
https://hg.mozilla.org/integration/autoland/rev/c86433fe8cd0
Fix tests relying on newRoot mutations r=rcaliman
https://hg.mozilla.org/integration/autoland/rev/0fe3613ee36b
Check webprogresslistener as a fallback for non loaded documents r=rcaliman,ochameau

Comment 33

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/3703fbfe567a
https://hg.mozilla.org/mozilla-central/rev/a7242f506aa6
https://hg.mozilla.org/mozilla-central/rev/c86433fe8cd0
https://hg.mozilla.org/mozilla-central/rev/0fe3613ee36b

Comment 34

[Narcis Beleuzu [:NarcisB]]

Backed out for dt failures on browser_animation_inspector_exists.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/bd777a58de7e9ee66da8b5fa3ee7a1f4921c63e8
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=301403561&repo=autoland&lineNumber=37513

Comment 35

[Julian Descottes [:jdescottes]]

I messed up the patch queue and missed a patch when landing...

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/0ba3aba8133f
Add watch-like API for new root event r=ochameau,rcaliman
https://hg.mozilla.org/integration/autoland/rev/690cc322f8be
Add test for inspector target switching r=ochameau,rcaliman
https://hg.mozilla.org/integration/autoland/rev/c67d4436a61e
Fix tests relying on newRoot mutations r=rcaliman
https://hg.mozilla.org/integration/autoland/rev/cc9959150347
Check webprogresslistener as a fallback for non loaded documents r=rcaliman,ochameau

Comment 37

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/0ba3aba8133f
https://hg.mozilla.org/mozilla-central/rev/690cc322f8be
https://hg.mozilla.org/mozilla-central/rev/c67d4436a61e
https://hg.mozilla.org/mozilla-central/rev/cc9959150347