1604126 - getWalker of InspectorFront should wait until the target document was finished to load

Status: RESOLVED FIXED

(DevTools :: Style Editor, enhancement, P3)

Description

[Daisuke Akatsuka (:daisuke)]

This is a follow-up of bug 1578753.
Now, we face an error (almost all the time) as like follows if we do following STRs.

 0:13.84 GECKO(15206) console.error: "Error while calling actor 'inspector's method 'getWalker'" "this
.targetActor.window.document.documentElement is null"                                                
 0:13.84 GECKO(15206) console.error: "getWalker@resource://devtools/server/actors/inspector/inspector.
js:150

STRs:

1. Turn devtools.target-switching.enabled on
2. Open about:robots (that runs on parent process)
3. Open DevTools
4. Select StyleEditor
5. Open any page that runs on the child process. (e.g. https://exmaple.com)

The reason for this failure is a place where we check whether the document inspected is XUL document or not. Now, we try to check the flag with the namespace of documentElement, but the object will be null while loading the document.
Actually, we add a handler to wait to load the document here, but we need to check the readyState of document before touching the nullable object documentElement.

Comment 1

[Alexandre Poirot [:ochameau]]

I think it relates to bug 1277348 and making the inspector be able to inspect the page before DOMContentLoaded, i.e. before the page is almost loaded. In that other bug, I had to make documentElement be nullable. See attachment 8800771 [details].

I don't think we should do anything around targets / TargetList, but rather fix the inspector to support non-fully-loaded documents.

Comment 2

[Julian Descottes [:jdescottes]]

Quick summary of the issues that we spotted while looking at this:

• getWalker will crash at https://searchfox.org/mozilla-central/rev/6bceafe72cad5d5752667b4b6bd595d3a4047ca3/devtools/server/actors/inspector/inspector.js#149-151 . This crash breaks the styleeditor
• getHighlighterByType will crash in the onXUL helper called at https://searchfox.org/mozilla-central/rev/6bceafe72cad5d5752667b4b6bd595d3a4047ca3/devtools/server/actors/highlighters.js#567 . This does not lead to a blank styleeditor, it is simply visible in the logs.

It seems getWalker comes from StyleEditorUI's initializeHighlighter which retrieves the inspector front immediately :

  async _onTargetAvailable({ targetFront, isTopLevel }) {
    if (isTopLevel) {
      await this.initializeHighlighter(targetFront);
      // ...
    }
  },
  async initializeHighlighter(targetFront) {
    const inspectorFront = await targetFront.getFront("inspector");
    // ...
    }
  },

And the inspector front initialize method does:

  // async initialization
  async initialize() {
    await Promise.all([
      this._getWalker(),
      this._getHighlighter(),
      this._getPageStyle(),
      this._startChangesFront(),
    ]);
  }

To fix getWalker, we first need to check if XUL documents still need to be handled (we announced their removal at cf https://groups.google.com/forum/#!msg/firefox-dev/WGcGYaQgu3g/-Z5jtoWdEAAJ but maybe the code needs to stay for some reason).
If we still need to handle this, then maybe we could simply listen to both "load" and "DOMContentLoaded" events, without checking the document type - since it might crash. After that getWalker is supposed to correctly wait for the document load before resolving, so we should be ok.

For the issue in getHighlighterByType, I thought it was also coming from the inspector front initialization. But that doesn't seem to be the case so we first need to check if it comes from initializeHighlighter in StyleEditorUI.jsm. The document.readyState was consistently "uninitialized" when I was seeing this failure, which is weird because in theory we should already have resolved getFront("inspector") which means the walker should be ready. Unless the call comes from another spot?

Comment 3

[Julian Descottes [:jdescottes]]

Brian, do we still need to handle XUL documents as in https://searchfox.org/mozilla-central/rev/6bceafe72cad5d5752667b4b6bd595d3a4047ca3/devtools/server/actors/inspector/inspector.js#149-151 ? Or can we skip this now that XUL documents are gone (I might be confused with the vocabulary)

Comment 4

[Brian Grinstead [:bgrins]]

(In reply to Julian Descottes [:jdescottes] from comment #3)

Brian, do we still need to handle XUL documents as in https://searchfox.org/mozilla-central/rev/6bceafe72cad5d5752667b4b6bd595d3a4047ca3/devtools/server/actors/inspector/inspector.js#149-151 ? Or can we skip this now that XUL documents are gone (I might be confused with the vocabulary)

The name of that variable isn't accurate anymore. There's no such thing as a "XUL document", but there are (1) "documents that have a root XUL element (<xul:window>)" and (2) "documents loaded with the prototype cache (any chrome xhtml)". The former case is still a relevant check in some cases (for instance the normal highlighter anonymous content thing won't work with a XUL root element) but I don't think it would be relevant in getWalker. The latter may be relevant in getWalker as far as readyState / DOMContentLoaded events potentially being treated differently) but the check would already be insufficient (for instance browser.xhtml uses an <html> root element).

So, I'd first try to remove that check and see if anything breaks. If so then possibly change the check to something like doc.nodePrincipal.isSystemPrincipal && doc.contentType == "application/xhtml+xml" to capture (2).

Comment 5

[Julian Descottes [:jdescottes]]

Thanks for the clarification Brian!

(In reply to Brian Grinstead [:bgrins] from comment #4)

(In reply to Julian Descottes [:jdescottes] from comment #3)

Brian, do we still need to handle XUL documents as in https://searchfox.org/mozilla-central/rev/6bceafe72cad5d5752667b4b6bd595d3a4047ca3/devtools/server/actors/inspector/inspector.js#149-151 ? Or can we skip this now that XUL documents are gone (I might be confused with the vocabulary)

[...]

So, I'd first try to remove that check and see if anything breaks. If so then possibly change the check to something like doc.nodePrincipal.isSystemPrincipal && doc.contentType == "application/xhtml+xml" to capture (2).

For the record I tried removing the check, and the test that was added with this check didn't fail. The test is https://searchfox.org/mozilla-central/source/devtools/client/inspector/test/browser_inspector_reload_xul.js , but at least locally it doesn't trigger the codepath that relies on load/DOMContentLoaded.

To test "(2) documents loaded with the prototype cache (any chrome xhtml)", I updated the test to load the xhtml page via chrome://, and I verified that it was matching the check you suggested doc.nodePrincipal.isSystemPrincipal && doc.contentType == "application/xhtml+xml".

But in the end, I lack a good test scenario to get such a document in a loading state. The current test browser_styleeditor_fission_switch_target.js often triggers this when moving from about:robots to a content page. But so far I haven't find a good way to trigger it when moving to a chrome://...something.xhtml page.

Comment 6

[Brian Grinstead [:bgrins]]

(In reply to Julian Descottes [:jdescottes] from comment #5)

Thanks for the clarification Brian!

(In reply to Brian Grinstead [:bgrins] from comment #4)

(In reply to Julian Descottes [:jdescottes] from comment #3)

Brian, do we still need to handle XUL documents as in https://searchfox.org/mozilla-central/rev/6bceafe72cad5d5752667b4b6bd595d3a4047ca3/devtools/server/actors/inspector/inspector.js#149-151 ? Or can we skip this now that XUL documents are gone (I might be confused with the vocabulary)

[...]

So, I'd first try to remove that check and see if anything breaks. If so then possibly change the check to something like doc.nodePrincipal.isSystemPrincipal && doc.contentType == "application/xhtml+xml" to capture (2).

For the record I tried removing the check, and the test that was added with this check didn't fail. The test is https://searchfox.org/mozilla-central/source/devtools/client/inspector/test/browser_inspector_reload_xul.js , but at least locally it doesn't trigger the codepath that relies on load/DOMContentLoaded.

To test "(2) documents loaded with the prototype cache (any chrome xhtml)", I updated the test to load the xhtml page via chrome://, and I verified that it was matching the check you suggested doc.nodePrincipal.isSystemPrincipal && doc.contentType == "application/xhtml+xml".

But in the end, I lack a good test scenario to get such a document in a loading state. The current test browser_styleeditor_fission_switch_target.js often triggers this when moving from about:robots to a content page. But so far I haven't find a good way to trigger it when moving to a chrome://...something.xhtml page.

When you say "lack a good test scenario to get such a document in a loading state" are you referring to the reported this .targetActor.window.document.documentElement is null exception, or a state that requires the DOMContentLoaded event? FWIW I think just removing the check is fine.

Comment 7

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1604126: Remove code that checks whether the document is XUL or not. r?jdescottes!

Comment 8

[Daisuke Akatsuka (:daisuke)]

Attachment: Bug 1604126: Enable a styleeditor test that has been blocked by this bug. r?jdescottes!

Depends on D60794

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/94392af1b184
Remove code that checks whether the document is XUL or not. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/2840946503b2
Enable a styleeditor test that has been blocked by this bug. r=jdescottes

Comment 10

[Alexandru Michis [:malexandru]]

Backed out 2 changesets for causing devtool failures in browser_styleeditor_fission_switch_target.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/1744d189c437cc3e4ff3c9d4ed5ad6b208a327d1

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=2840946503b2b78ee175d121371991a3ef234df2&searchStr=devtools&selectedJob=286112879

Failure logs:

• https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=286112879&repo=autoland&lineNumber=25782
• leaks: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=286114698&repo=autoland&lineNumber=13585

Comment 11

[Daisuke Akatsuka (:daisuke)]

I changed a bit and looks like this is working.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=583498e0fc30ef9706cf7287cdcb1952a628e1c0&selectedJob=286141850

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f015da156476
Remove code that checks whether the document is XUL or not. r=jdescottes
https://hg.mozilla.org/integration/autoland/rev/a2b1309c16ab
Enable a styleeditor test that has been blocked by this bug. r=jdescottes

Comment 13

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/f015da156476
https://hg.mozilla.org/mozilla-central/rev/a2b1309c16ab