1737832 - Simplify chrome vs content handling in nsWindowWatcher::OpenWindowInternal

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, task)

Description

[Tooru Fujisawa [:arai]]

nsWindowWatcher::OpenWindowInternal calculates chrome flag for 3 cases:

• isCallerChrome && XRE_IsParentProcess() && hasChromeParent
  • the caller has enough privilege
  • it has full control
• isCallerChrome && XRE_IsParentProcess() && !hasChromeParent
  • special case (see below)
• !(isCallerChrome && XRE_IsParentProcess())
  • the caller is web content
  • it has control over whether to open minimal-popup or not

https://searchfox.org/mozilla-central/rev/3b86063e6d46b2f130513c499343cd47773062b1/toolkit/components/windowwatcher/nsWindowWatcher.cpp#704-716

nsresult nsWindowWatcher::OpenWindowInternal(
    mozIDOMWindowProxy* aParent, const nsACString& aUrl,
    const nsACString& aName, const nsACString& aFeatures, bool aCalledFromJS,
    bool aDialog, bool aNavigate, nsIArray* aArgv, bool aIsPopupSpam,
    bool aForceNoOpener, bool aForceNoReferrer, PrintKind aPrintKind,
    nsDocShellLoadState* aLoadState, BrowsingContext** aResult) {
...
  if (isCallerChrome && XRE_IsParentProcess()) {
    chromeFlags = CalculateChromeFlagsForSystem(
        features, sizeSpec, aDialog, uriToLoadIsChrome, hasChromeParent);
  } else {
    MOZ_DIAGNOSTIC_ASSERT(parentBC && parentBC->IsContent(),
                          "content caller must provide content parent");
    chromeFlags = CalculateChromeFlagsForContent(features, sizeSpec);

    if (aDialog) {
      MOZ_ASSERT(XRE_IsParentProcess());
      chromeFlags |= nsIWebBrowserChrome::CHROME_OPENAS_DIALOG;
    }
  }

isCallerChrome && XRE_IsParentProcess() && !hasChromeParent happens in the following 2 cases:

(a) nsGlobalWindowOuter::Print
https://searchfox.org/mozilla-central/rev/3b86063e6d46b2f130513c499343cd47773062b1/dom/base/nsGlobalWindowOuter.cpp#5337-5346

• URL = empty
• features = empty
• aDialog = false
• parentBC = non-null

(b) nsDocShell::PerformRetargeting
https://searchfox.org/mozilla-central/rev/3b86063e6d46b2f130513c499343cd47773062b1/docshell/base/nsDocShell.cpp#8489-8502

• URL = not empty
• features = empty
• aDialog = false
• parentBC = non-null

those are covered by the following 3 tests:

• https://searchfox.org/mozilla-central/source/toolkit/components/extensions/test/mochitest/test_ext_new_tab_processType.html
• https://searchfox.org/mozilla-central/source/dom/base/test/test_anchor_target_blank_referrer.html
• https://searchfox.org/mozilla-central/source/layout/base/tests/chrome/test_printpreview_bug482976.xhtml

Then, if features is empty and aDialog is false, the only differences in the resulting chrome flag between CalculateChromeFlagsForSystem and CalculateChromeFlagsForContent are the following,
otherwise the flag is set to nsIWebBrowserChrome::CHROME_ALL:

https://searchfox.org/mozilla-central/rev/3b86063e6d46b2f130513c499343cd47773062b1/toolkit/components/windowwatcher/nsWindowWatcher.cpp#1889-1918

  // Determine whether the window should have remote tabs.
  bool remote = BrowserTabsRemoteAutostart();

...

  if (remote) {
    chromeFlags |= nsIWebBrowserChrome::CHROME_REMOTE_WINDOW;
  }

  // Determine whether the window should have remote subframes
  bool fission = FissionAutostart();

...

  if (fission) {
    chromeFlags |= nsIWebBrowserChrome::CHROME_FISSION_WINDOW;
  }

And those 2 flags are overridden immediately after that, because parentBC is non-null and windowTypeIsChrome == false (CHROME_OPENAS_CHROME isn't set):

https://searchfox.org/mozilla-central/rev/3b86063e6d46b2f130513c499343cd47773062b1/toolkit/components/windowwatcher/nsWindowWatcher.cpp#734-746

  // If we're opening a content window from a content window, we need to exactly
  // inherit fission and e10s status flags from parentBC. Only new toplevel
  // windows may change these options.
  if (parentBC && parentBC->IsContent() && !windowTypeIsChrome) {
    chromeFlags &= ~(nsIWebBrowserChrome::CHROME_REMOTE_WINDOW |
                     nsIWebBrowserChrome::CHROME_FISSION_WINDOW);
    if (parentBC->UseRemoteTabs()) {
      chromeFlags |= nsIWebBrowserChrome::CHROME_REMOTE_WINDOW;
    }
    if (parentBC->UseRemoteSubframes()) {
      chromeFlags |= nsIWebBrowserChrome::CHROME_FISSION_WINDOW;
    }
  }

So, there's no differences, and we can use CalculateChromeFlagsForContent for isCallerChrome && XRE_IsParentProcess() && !hasChromeParent case, for existing consumer.

Comment 1

[Tooru Fujisawa [:arai]]

Attachment: Bug 1737832 - Simplify the chromeFlag calculation in window.open for nsGlobalWindowOuter::Print and nsDocShell::PerformRetargeting. r?smaug!

This simplifies the chromeFlag calculation for both chrome-priv case and
content case by removing the special case for
nsGlobalWindowOuter::Print and nsDocShell::PerformRetargeting,
that has chrome caller but no chrome parent.

Code path for nsGlobalWindowOuter::Print and nsDocShell::PerformRetargeting
now uses the content case instead of the chrome-priv case, but the resulting
flag doesn't change (CHROME_ALL).

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/712b897cbc76
Simplify the chromeFlag calculation in window.open for nsGlobalWindowOuter::Print and nsDocShell::PerformRetargeting. r=smaug

Comment 3

[Marian-Vasile Laza]

Backed out 4 changesets (Bug 1737832, Bug 1701001) for causing geckoview failures on noopener-noreferrer-BarProp.window.html.
Backout link
Push with failures
Failure Log

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/47df3d211f15
Simplify the chromeFlag calculation in window.open for nsGlobalWindowOuter::Print and nsDocShell::PerformRetargeting. r=smaug

Comment 5

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/47df3d211f15