1676769 - Opening browser toolbox with the print dialog open with OOP iframes fails (Mismatch between DevToolsFrameParent and DevToolsFrameChild DevToolsFrameParent).

Status: RESOLVED FIXED

(DevTools :: General, defect, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

Thank you for helping make Firefox better. If you are reporting a defect, please complete the following:

What were you doing?

• Fission enabled (probably necessary, but not sure).
• Open https://bug1676721.bmoattachments.org/attachment.cgi?id=9187223
• Pres Print, or Ctrl+P.
• Try to open browser toolbox.

What happened?

Browser toolbox is all broken:

JavaScript error: resource://devtools/server/connectors/js-window-actor/DevToolsFrameChild.jsm, line 322: Error: Mismatch between DevToolsFrameParent and DevToolsFrameChild DevToolsFrameParent:destroy expected browsing context with browserId undefined, but got 2147483649

What should have happened?

I can debug stuff :)

Comment 1

[Julian Descottes [:jdescottes]]

Thanks for filing!

Fission enabled (probably necessary, but not sure).

Just checked, only happens with fission enabled.
Edit: can also repro without Fission, must have used wrong STRs

Comment 2

[Julian Descottes [:jdescottes]]

Note that if you open the Browser Toolbox before clicking on print, you can inspect the content of the print window.
It seems we try to create FrameTargets for a WatcherActor linked to ProcessDescriptor. This WatcherActor has no browserId, and we can't instantiate FrameTargets from it.

Comment 3

[Julian Descottes [:jdescottes]]

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

Note that if you open the Browser Toolbox before clicking on print, you can inspect the content of the print window.
It seems we try to create FrameTargets for a WatcherActor linked to ProcessDescriptor. This WatcherActor has no browserId, and we can't instantiate FrameTargets from it.

Not directly related to that. We always use a process descriptor for the Multiprocess Browser Toolbox.

The issue is that we are hitting the following check:

  // For some unknown reason, the print preview of PDFs generates an about:blank document,
  // which, on the parent process, has windowGlobal.documentURI.spec set to the pdf's URL.
  // So that Frame target helper accept this WindowGlobal and instantiate a target for it.
  // Which is great as this is a valuable document to debug.
  // But at the end of the day, in the content process, this ends up being an about:blank document,
  // and hasLoadedNonBlankURI is always false.
  // As print preview uses a dialog, check for `opener` being set. It is hard to find
  // any very good differenciator...
  if (!window.docShell.hasLoadedNonBlankURI && !browsingContext.opener) {
    return false;
  }

Maybe browsingContext.opener is non-null without Fission and null with Fission?
Edit: Actually I also reproduce without Fission, looks like I tested incorrectly in comment 1

Comment 4

[Alexandre Poirot [:ochameau]]

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

The issue is that we are hitting the following check:

  // For some unknown reason, the print preview of PDFs generates an about:blank document,
  // which, on the parent process, has windowGlobal.documentURI.spec set to the pdf's URL.
  // So that Frame target helper accept this WindowGlobal and instantiate a target for it.
  // Which is great as this is a valuable document to debug.
  // But at the end of the day, in the content process, this ends up being an about:blank document,
  // and hasLoadedNonBlankURI is always false.
  // As print preview uses a dialog, check for `opener` being set. It is hard to find
  // any very good differenciator...
  if (!window.docShell.hasLoadedNonBlankURI && !browsingContext.opener) {
    return false;
  }

Sorry about this workaround and convoluted check here.
We might have to tune in the platform code to figure out what is so special with this document.

Comment 5

[Julian Descottes [:jdescottes]]

There are several components to this bug.

1/ Using sendQuery for instanciateTarget / addWatcherDataEntry
frame-helper and worker-helper loop on all the normally valid browsing contexts and call instanciateTarget/addWatcherDataEntry. Both APIs rely on JSWindowActor's sendQuery. This means that if the child actor throws, the parent process createTargets or addWatcherDataEntry call will throw as well.

See for instance https://searchfox.org/mozilla-central/rev/c37038c592a352eda0f5e77dfb58c4929bf8bcd3/devtools/server/actors/watcher/target-helpers/frame-helper.js#43

We could try catch around those errors to avoid having one problematic target making the whole toolbox blank.

2/ Check the documentURI instead of browsingContext.opener

browsingContext.opener doesn't seem to work for me here. Maybe it did initially but I always get false for this in my tests. It seems instead we could check the documentURI. Since the print preview is still linked to the original document, checking window.document.documentURI will return the real document's URL (whereas window.location.href returns about:blank). This is probably still very implementation dependent, and a bit fragile but it should fix the issue for now, and we can try to add a test against this.

Comment 6

[Julian Descottes [:jdescottes]]

(In reply to Alexandre Poirot [:ochameau] from comment #4)

Sorry about this workaround and convoluted check here.
We might have to tune in the platform code to figure out what is so special with this document.

Yes definitely.

Comment 7

[Julian Descottes [:jdescottes]]

Attachment: Bug 1676769 - [devtools] Use document.documentURI to detect print preview documents

DevToolsFrameChild actor excludes about:blank windows to avoid creating unnecessary targets.
In the parent process we are supposed to also exclude about:blank windows when fetching the browsing-contexts that should be debugged.
However the parent-process logic relies on checking the documentURI to exclude about:blank. The actor relies on a docShell flag called hasLoadedNonBlankURI.

The printpreview browser element uses an about:blank window, but still displays a document different from about:blank.
Consequently the parent process check considers this document as valid, because of its documentURI.
But the child actor throws because of the hasLoadedNonBlankURI flag.

This changeset adds an additional check for window.document.documentURI in the child actor.
Also adds a new mochitest to check that we can open the BrowserToolbox and inspect the print preview

Comment 8

[Julian Descottes [:jdescottes]]

Attachment: Bug 1676769 - [devtools] Try catch around instantiateTarget/addWatcherDataEntry in DevTools parent actors

Depends on D97153

DevTools window actors rely on sendQuery for 2 APIs: instantiateTarget and addWatcherDataEntry.
If an exception occurs in the child actor, it will be propagated back to the parent, which will reject the sendQuery.
This can make the code revolving around those APIs very fragile. Since this code is involved in setting up the initial targets, it could be nice to catch errors there to avoid too many potential blank toolbox situations.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9b286f0c5c7f
[devtools] Use document.documentURI to detect print preview documents r=ochameau
https://hg.mozilla.org/integration/autoland/rev/cf1a7a72636f
[devtools] Try catch around instantiateTarget/addWatcherDataEntry in DevTools parent actors r=ochameau,nchevobbe

Comment 10

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/9b286f0c5c7f
https://hg.mozilla.org/mozilla-central/rev/cf1a7a72636f