Closed Bug 1604126 Opened 5 years ago Closed 4 years ago

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

Categories

(DevTools :: Style Editor, enhancement, P3)

Product:
DevTools
Component:
Style Editor
Type:
enhancement

Tracking

(firefox74 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 74
Tracking Flags:
Tracking Status
firefox74 --- fixed

People

(Reporter: daisuke, Assigned: daisuke)

References

Details

Attachments

(2 files)

Bug 1604126: Remove code that checks whether the document is XUL or not. r?jdescottes!
47 bytes, text/x-phabricator-request
Details | Review
Bug 1604126: Enable a styleeditor test that has been blocked by this bug. r?jdescottes!
47 bytes, text/x-phabricator-request
Details | Review

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.

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.

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

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?

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)

Flags: needinfo?(bgrinstead)

(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).

Flags: needinfo?(bgrinstead)

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.

(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.

Blocks: 1568874
Assignee: nobody → daisuke
Status: NEW → ASSIGNED
Pushed by dakatsuka.birchill@mozilla.com:
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
Pushed by dakatsuka.birchill@mozilla.com:
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

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

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
status-firefox74: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 74
QA Whiteboard: [qa-74b-p2]
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: