1581093 - Empty DevTools window in private browsing mode when toggling the docking position

Status: RESOLVED FIXED

(DevTools :: General, defect, P2)

Description

[Igor]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/76.0.3809.132 Safari/537.36

Steps to reproduce:

Macos 10.14.6, FF DevEdition 70b5
Short: In private mode trying open devtools in separate window.
Watch the video https://www.youtube.com/watch?v=LXQZ2SkQ93

Actual results:

Empty window opened.

Expected results:

Devtools should open in separate window

Comment 1

[Igor]

UPD: lost last char in video URL
correct link https://www.youtube.com/watch?v=LXQZ2SkQ93k

Comment 2

[Patrick Brosset <:pbro>]

Thanks for filing. I can reproduce this too on macOS, both on DevEdition and on Nightly.
The new window appears, but is empty, and the toolbox that was previously docked in the browser window is still visible.
So the switching between the 2 hosts is stopped in mid-air for some reason.
The browser console shows: NS_ERROR_NOT_IMPLEMENTED toolbox-host-manager.js:239 which points to this line of code: newIframe.swapFrameLoaders(iframe);.

Comment 3

[Patrick Brosset <:pbro>]

This is an important issue we should fix quickly. I have made it a P2 instead of a P1 for the following reasons:

• it only occurs in private mode
• you can recover from it (the toolbox is still visible in the tab and you can still use it)

Comment 11

[Patrick Brosset <:pbro>]

Running mozregression at the moment to find the regressing bug.

Comment 12

[Patrick Brosset <:pbro>]

I found the regressing bug to be 1539979.

Last good revision: 91f7769b3d7ec17aa11f7a8ada7104f5b4cec38d
First bad revision: 39e19afc797d310f568447eae7054789e641b591
Pushlog: https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=91f7769b3d7ec17aa11f7a8ada7104f5b4cec38d&tochange=39e19afc797d310f568447eae7054789e641b591

Comment 13

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

smaug: We could use help with this frameloader issue!

With the STRs:

• open private window
• open devtools in private window
• switch DevTools to window host
  we get an error when calling swapFrameLoaders->

JavaScript error: resource://devtools/client/framework/toolbox-host-manager.js, line 239: NS_ERROR_NOT_IMPLEMENTED

The method is called from https://searchfox.org/mozilla-central/source/devtools/client/framework/toolbox-host-manager.js#239

    const iframe = this.host.frame;
    const newHost = this.createHost(hostType);
    const newIframe = await newHost.create();
    // change toolbox document's parent to the new host
    newIframe.swapFrameLoaders(iframe);

Note that the iframes in question here are actually <browser> elements, created via
https://searchfox.org/mozilla-central/rev/23d4bffcad365e68d2d45776017056b76ca9a968/devtools/client/framework/toolbox-hosts.js#432-445

function createDevToolsFrame(doc, className) {
  let frame;
  if (Services.prefs.getBoolPref("devtools.toolbox.content-frame", false)) {
    frame = doc.createXULElement("browser");
    frame.setAttribute("type", "content");
  } else {
    frame = doc.createXULElement("iframe");
  }
  // ...
  return frame;
}

The element used (browser or iframe) doesn't seem to matter, but setAttribute("type", "content"); seems mandatory to trigger the issue.

In C++ I tracked it down to the following check inside swapFrameLoaders:
https://searchfox.org/mozilla-central/rev/23d4bffcad365e68d2d45776017056b76ca9a968/dom/base/nsFrameLoader.cpp#1583-1585

  if (ourOriginAttributes != otherOriginAttributes) {
    return NS_ERROR_NOT_IMPLEMENTED;
  }

Removing the check "fixes" the issue, but I guess it's here for a good reason. Do you have a suggestion to move forward here?

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

(I don't know what "switch DevTools to window host" means.)

So some iframe moves, or its contents to be precise, from document A to document B? I assume only one of them is in private browsing mode, when they both should be.

Comment 15

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

Attachment: choose_devtools_host.png

switch devtools to window host

Means chosing "separate window" in the menu (see picture).

So some iframe moves, or its contents to be precise, from document A to document B?
I assume only one of them is in private browsing mode, when they both should be.

I guess the iframe we create inside of the private window is in private browsing mode, while the new one is not?

:ochameau found that we could copy attributes using

      let attrs = iframe.docShell.getOriginAttributes();
      newIframe.docShell.setOriginAttributes(attrs); 

I'll move forward with that (and using getOriginAttributes, I'll try to confirm which attribute is different between the two docshells).

Comment 16

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

After checking, the discrepancy in docshell attributes is indeed about "privateBrowsingId".

Comment 17

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

Attachment: Bug 1581093 - Fix blank DevTools window in private browsing, set private flag on host window

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

But is the whole top level window initially in pb mode, but the new window isn't?

Comment 19

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

(In reply to Olli Pettay [:smaug] from comment #18)

But is the whole top level window initially in pb mode, but the new window isn't?

That's correct, I suppose that it is because we always create the window using

      let flags = "chrome,centerscreen,resizable,dialog=no";
      const win = Services.ww.openWindow(
        null,
        this.WINDOW_URL,
        "_blank",
        flags,
        null
      );

We always pass the same flags, regardless of the window that we are trying to debug.

Based on your comment I tried an alternative approach, which is to set the "private" flag if the top window is private:

      let flags = "chrome,centerscreen,resizable,dialog=no";
      if (
        this.hostTab &&
        PrivateBrowsingUtils.isWindowPrivate(this.hostTab.ownerGlobal)
      ) {
        flags += ",private";
      }
      const win = Services.ww.openWindow(
        null,
        this.WINDOW_URL,
        "_blank",
        flags,
        null
      );

This also fixes the issue. Is this more adequate in your opinion?

Comment 20

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

Attachment: Bug 1581093 - Fix blank DevTools window in private browsing, set private flag on host window

Comment 21

[Alexandre Poirot [:ochameau]]

As I just said in https://phabricator.services.mozilla.com/D56636 review,
I'm not sure the DevTools iframe should ever run in private browsing mode.
i.e. that its docshell has private browsing origin attribute set to 1.

Given that, I don't have a big preference between the two attached patchs:

• The copy of origin attributes loos like a quite low level hack which should not be necessary. The environment and the attributes set on the iframe should define the right origin attributes.
• While, the tweak made to the devtools window attributes is higher level but is also a bit surprising. We are tweaking the environment instead of changing the iframe itself.

I'm wondering if we should force all the DevTools iframes created over here:
https://searchfox.org/mozilla-central/rev/690e903ef689a4eca335b96bd903580394864a1c/devtools/client/framework/toolbox-hosts.js#429-445
to never be running with private browsing flags.

One issue I see is, is that we may not be able to force disabling private browsing until the iframe has a DocShell and so is added to the Document.
But once this is done, we can do it like this:
https://searchfox.org/mozilla-central/source/browser/components/BrowserContentHandler.jsm#312-316
And so we may have to do that after we added the iframe to the DOM Tree over here:
https://searchfox.org/mozilla-central/rev/690e903ef689a4eca335b96bd903580394864a1c/devtools/client/framework/toolbox-hosts.js#75
https://searchfox.org/mozilla-central/rev/690e903ef689a4eca335b96bd903580394864a1c/devtools/client/framework/toolbox-hosts.js#165-171

Comment 22

[Olli Pettay [:smaug][[e-mail]]]

Yeah, I think it is less unexpected setup to have both top level windows using the same pb mode, in other words
docShell.usePrivateBrowsing on top level docshells should be the same.
I assume the separate pb-mode devtools window can't be attached back to some non-private browsing window.

Comment 23

[Olli Pettay [:smaug][[e-mail]]]

Hmm, never running in pb mode... maybe, if that doesn't lead to principal mismatches or such.

Comment 24

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

(In reply to Olli Pettay [:smaug] from comment #22)

Yeah, I think it is less unexpected setup to have both top level windows using the same pb mode, in other words
docShell.usePrivateBrowsing on top level docshells should be the same.

Thanks for the feedback!

In that regard I think the second patch is better. With the first patch I you first open the DevTools in window and then dock them, since the original window has privateBrowsing:0, the patch would just update the docShell of the docked devtools iframe to also have privateBrowsing:0 (even though it is nested in a private browsing window).

So the second patch will be more consistent, devtools docshells will always be in pb mode when debugging pb windows. We can the investigate later if it is possible to consistently run in non pb-mode.

I assume the separate pb-mode devtools window can't be attached back to some non-private browsing window.

That's correct. The window is linked to the tab we are currently debugging and won't be reused to debug a non-pb tab.

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

As I just said in https://phabricator.services.mozilla.com/D56636 review,
I'm not sure the DevTools iframe should ever run in private browsing mode.
i.e. that its docshell has private browsing origin attribute set to 1.

Given that, I don't have a big preference between the two attached patchs:

• The copy of origin attributes loos like a quite low level hack which should not be necessary. The environment and the attributes set on the iframe should define the right origin attributes.
• While, the tweak made to the devtools window attributes is higher level but is also a bit surprising. We are tweaking the environment instead of changing the iframe itself.

Then I'm moving forward with the second patch for the consistency issue I mentioned just above.

I'm wondering if we should force all the DevTools iframes created over here:
https://searchfox.org/mozilla-central/rev/690e903ef689a4eca335b96bd903580394864a1c/devtools/client/framework/toolbox-hosts.js#429-445
to never be running with private browsing flags.

One issue I see is, is that we may not be able to force disabling private browsing until the iframe has a DocShell and so is added to the Document.
But once this is done, we can do it like this:
https://searchfox.org/mozilla-central/source/browser/components/BrowserContentHandler.jsm#312-316
And so we may have to do that after we added the iframe to the DOM Tree over here:
https://searchfox.org/mozilla-central/rev/690e903ef689a4eca335b96bd903580394864a1c/devtools/client/framework/toolbox-hosts.js#75
https://searchfox.org/mozilla-central/rev/690e903ef689a4eca335b96bd903580394864a1c/devtools/client/framework/toolbox-hosts.js#165-171

Thanks for the pointers! I added them to the follow up https://bugzilla.mozilla.org/show_bug.cgi?id=1603091

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/699dc090b0bb
Fix blank DevTools window in private browsing, set private flag on host window r=ochameau

Comment 26

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/699dc090b0bb

Comment 28

[Julian Descottes [:jdescottes] (PTO -> Jan 5th)]

Comment on attachment 9115013 [details]
Bug 1581093 - Fix blank DevTools window in private browsing, set private flag on host window

Beta/Release Uplift Approval Request

• User impact if declined: When using DevTools in a private window, users cannot open devtools in a separate window.
  Private windows are often used by developers to get a cleaner environment because it disable addons.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): This is devtools only, covered by tests.
  When debugging private windows, the docked DevTools already run in private browsing mode. We are only extending this to also apply to DevTools running in a separate window.
  Therefore we don't expect new issues related to this setup, it should only make the behavior more consistent.
• String changes made/needed:

Comment 29

[Julien Cristau [:jcristau]]

Comment on attachment 9115013 [details]
Bug 1581093 - Fix blank DevTools window in private browsing, set private flag on host window

fix for a devtools regression, approved for 72.0b8

Comment 30

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/releases/mozilla-beta/rev/37fcce5aa2f9