1934520 - Inspector breaks when opened after an XML file with an invalid embedded XSLT has been shown

Status: RESOLVED FIXED

(DevTools :: Inspector, defect, P2)

Description

[Pier Angelo Vendrame]

Attachment: A file that allows to see the problem

What were you doing?

1. Try to open the attached test.xml
2. Open the inspector (e.g., F12)

What happened?

The inspector does not open (see screenshot).

I get these errors on the browser console:

 TypeError: window.performance is null
    onWindowReady resource://devtools/server/actors/webconsole/listeners/document-events.js:112
    listen resource://devtools/server/actors/webconsole/listeners/document-events.js:87
    watch resource://devtools/server/actors/resources/document-event.js:97
    watchResources resource://devtools/server/actors/resources/index.js:342
    addOrSetSessionDataEntry resource://devtools/server/actors/targets/session-data-processors/resources.js:19
    addOrSetSessionDataEntry resource://devtools/server/actors/targets/base-target-actor.js:86
    #addOrSetSessionDataEntry resource://devtools/server/connectors/js-process-actor/DevToolsProcessChild.sys.mjs:389
    receiveMessage resource://devtools/server/connectors/js-process-actor/DevToolsProcessChild.sys.mjs:314
Exception while opening the toolbox Error: Protocol error (TypeError): window.performance is null from: server0.conn0.watcher2 (resource://devtools/server/actors/webconsole/listeners/document-events.js:112:18) Error: Protocol error (TypeError): window.performance is null from: server0.conn0.watcher2 (resource://devtools/server/actors/webconsole/listeners/document-events.js:112:18)
    onPacket resource://devtools/shared/protocol/Front.js:382
    DevTools RDP*request resource://devtools/shared/protocol/Front.js:299
    name resource://devtools/shared/protocol/Front/FrontClassWithSpec.js:47
    _startListening resource://devtools/shared/commands/resource/resource-command.js:1047

The browser console is also unusable if opened after the file.

What should have happened?

The inspector should have opened.

Anything else we should know?

Nope.

Comment 1

[Pier Angelo Vendrame]

Attachment: Screenshot with the problem. Notice that in the below area the inspector should be shown. Also, dragging to resize works.

Comment 2

[Nicolas Chevobbe [:nchevobbe]]

Thanks for the report, I can reproduce easily.
The following patch does fix the issue:

diff --git a/devtools/server/actors/webconsole/listeners/document-events.js b/devtools/server/actors/webconsole/listeners/document-events.js
--- a/devtools/server/actors/webconsole/listeners/document-events.js
+++ b/devtools/server/actors/webconsole/listeners/document-events.js
@@ -109,7 +109,7 @@ DocumentEventsListener.prototype = {
       return;
     }
 
-    const time = window.performance.timing.navigationStart;
+    const time = window.performance?.timing.navigationStart;
 
     this.emit("dom-loading", {
       time,
@@ -161,7 +161,7 @@ DocumentEventsListener.prototype = {
     // on the main document, that is when its Document.readyState changes to
     // 'interactive' and the corresponding readystatechange event is thrown
     const window = event.target.defaultView;
-    const time = window.performance.timing.domInteractive;
+    const time = window.performance?.timing.domInteractive;
     this.emit("dom-interactive", { time, isFrameSwitching });
   },
 
@@ -173,7 +173,7 @@ DocumentEventsListener.prototype = {
     // on the main document, that is when its Document.readyState changes to
     // 'complete' and the corresponding readystatechange event is thrown
     const window = event.target.defaultView;
-    const time = window.performance.timing.domComplete;
+    const time = window.performance?.timing.domComplete;
     this.emit("dom-complete", {
       time,
       isFrameSwitching,
@@ -193,10 +193,10 @@ DocumentEventsListener.prototype = {
     const isWindow = flag & Ci.nsIWebProgressListener.STATE_IS_WINDOW;
     const window = progress.DOMWindow;
     if (isDocument && isStop) {
-      const time = window.performance.timing.domInteractive;
+      const time = window.performance?.timing.domInteractive;
       this.emit("dom-interactive", { time });
     } else if (isWindow && isStop) {
-      const time = window.performance.timing.domComplete;
+      const time = window.performance?.timing.domComplete;
       this.emit("dom-complete", {
         time,
         hasNativeConsoleAPI: this.hasNativeConsoleAPI(window),

but then there are other errors

Pier Angelo, is there something you'd wanted to use the Inspector for in that state?

Comment 3

[Pier Angelo Vendrame]

I was developing a regression test for fingerprint vectors to be kept outside the source tree (I was trying to help the developer of a suite of similar tests).
The steps to reproduce I wrote above are a minimal example, but the same problem happens when you embed such a malformed XML file in an iframe, maybe I should've written that.
I was using an iframe with a malformed XML file as it's a known method to leak the app language into content (basically the PoC behind Bug 1900648).
So, I was using console.log to check if I got the message (or why I didn't get it).

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1866814

:ochameau, since you are the author of the regressor, bug 1866814, could you take a look? Also, could you set the severity field?

For more information, please visit BugBot documentation.

Comment 5

[Donal Meehan [:dmeehan]]

:jdescottes tagging as triage owner. Could you add a severity on this?
That will help to know if we should keep it tracked for Fx133.

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Donal Meehan [:dmeehan] from comment #5)

:jdescottes tagging as triage owner. Could you add a severity on this?
That will help to know if we should keep it tracked for Fx133.

I think it's S3, maybe we can simply hide the issue as suggested by Nicolas if it at least allows to use DevTools

Comment 7

[Julian Descottes [:jdescottes]]

So we had the same errors earlier, but they were caught in a try catch:

  async addOrSetSessionDataEntry({
    watcherActorID,
    sessionContext,
    type,
    entries,
    updateType,
  }) {
    try {
      await this.sendQuery("DevToolsFrameParent:addOrSetSessionDataEntry", {
        watcherActorID,
        sessionContext,
        type,
        entries,
        updateType,
      });
    } catch (e) {
      console.warn(
        "Failed to add session data entry for frame targets in browsing context",
        this.browsingContext.id
      );
      console.warn(e);
    }
  }

The code changed quite a bit after the switch to JSProcessActors, but at least I can't find a similar wrapper for calls to addOrSetSessionDataEntry, which makes setting up devtools more fragile than it used to be.

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1934520 - [devtools] Avoid errors in document-events webconsole listeners for broken XML docs

We used to try catch around calling DevToolsFrameParent.addOrSetSessionDataEntry but
when migrating to JSProcessActors, the try catch was removed.

In this case one of the console listeners fails to start on broken XML documents which prevents
from using devtools at all on a page which contains an iframe with such a document.

We should also fix the listener, but I think we should make the framework logic a bit safer first.

Test is added to cover toolbox opening in this case.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/810abd1d3a1b
[devtools] Avoid errors in document-events webconsole listeners for broken XML docs r=ochameau,devtools-reviewers

Comment 10

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/810abd1d3a1b

Comment 11

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jdescottes, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox135 to wontfix.

For more information, please visit BugBot documentation.

Comment 12

[Julian Descottes [:jdescottes]]

Attachment: Bug 1934520 - [devtools] Avoid errors in document-events webconsole listeners for broken XML docs

We used to try catch around calling DevToolsFrameParent.addOrSetSessionDataEntry but
when migrating to JSProcessActors, the try catch was removed.

One of the console listeners fails to start on broken XML documents which prevents
from using devtools at all on a page which contains an iframe with such a document.

The listener is updated to safely read the performance timing or use -1 if unavailable.

Test is added to cover toolbox opening in this case.

Original Revision: https://phabricator.services.mozilla.com/D232193

Comment 13

[Phabricator Automation]

beta Uplift Approval Request

• User impact if declined: DevTools inspector can be blank if any iframe contains an invalid XML document
• Code covered by automated testing: yes
• Fix verified in Nightly: yes
• Needs manual QE test: no
• Steps to reproduce for manual QE testing: N/A
• Risk associated with taking this patch: Low
• Explanation of risk level: Simple JS fix, DevTools only, covered by tests
• String changes made/needed: N/A
• Is Android affected?: no

Comment 14

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/9f4145da48e2