browser(firefox): remove the hack around setting viewport size (#4010)

Juggler code had a bug where we subscribed to window and tab events, but did not iterate collections of current windows and tabs. As a result, we were sometimes failing to set viewport size for the initial window, and implemented an artificial promise to workaround the problem. This patch: - starts calling `onOpenWindow` and `onOpenTabListener` callbacks for *all* windows and tabs - current and future, eliminating the race condition. This worked too well and we started overriding window sizes that were set by users with `window.open(url, 'width=300;height=400')` (we have a test for this). To fix this, we now plumb `CHROME_WITH_SIZE` flag from appWindow and override viewport iff this flag is not set. After this patch, we will use the `onTabOpened` event to move user agent emulation to the browser-side. References #3995
microsoft · Sep 30, 2020 · 24bc0e3 · 24bc0e3
1 parent a20c0e0
commit 24bc0e3
Show file tree

Hide file tree

Showing 4 changed files with 86 additions and 52 deletions.
diff --git a/browser_patches/firefox/BUILD_NUMBER b/browser_patches/firefox/BUILD_NUMBER
@@ -1,2 +1,2 @@
-1174
-Changed: [email protected] Tue Sep 29 02:02:37 MDT 2020
+1175
+Changed: [email protected] Wed Sep 30 00:44:40 MDT 2020
diff --git a/browser_patches/firefox/juggler/Helper.js b/browser_patches/firefox/juggler/Helper.js
@@ -22,6 +22,15 @@ class Helper {
     return () => receiver.removeEventListener(eventName, handler);
   }
 
+  awaitEvent(receiver, eventName) {
+    return new Promise(resolve => {
+      receiver.addEventListener(eventName, function listener() {
+        receiver.removeEventListener(eventName, listener);
+        resolve();
+      });
+    });
+  }
+
   on(receiver, eventName, handler) {
     // The toolkit/modules/EventEmitter.jsm dispatches event name as a first argument.
     // Fire event listeners without it for convenience.

diff --git a/browser_patches/firefox/juggler/TargetRegistry.js b/browser_patches/firefox/juggler/TargetRegistry.js
@@ -190,11 +190,12 @@ class TargetRegistry {
       },
     });
 
-    const onTabOpenListener = (window, event) => {
+    const onTabOpenListener = (appWindow, window, event) => {
       const tab = event.target;
       const userContextId = tab.userContextId;
       const browserContext = this._userContextIdToBrowserContext.get(userContextId);
-      if (browserContext && browserContext.defaultViewportSize)
+      const hasExplicitSize = (appWindow.chromeFlags & Ci.nsIWebBrowserChrome.JUGGLER_WINDOW_EXPLICIT_SIZE) !== 0;
+      if (!hasExplicitSize && browserContext && browserContext.defaultViewportSize)
         setViewportSizeForBrowser(browserContext.defaultViewportSize, tab.linkedBrowser, window);
     };
 
@@ -206,36 +207,46 @@ class TargetRegistry {
           target.dispose();
     };
 
-    Services.wm.addListener({
-      onOpenWindow: async window => {
-        const domWindow = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowInternal || Ci.nsIDOMWindow);
-        if (!(domWindow instanceof Ci.nsIDOMChromeWindow))
-          return;
-        if (domWindow.document.readyState !== 'uninitialized')
-          throw new Error('DOMWindow should not be loaded yet');
-        await new Promise(fulfill => {
-          domWindow.addEventListener('DOMContentLoaded', function listener() {
-            domWindow.removeEventListener('DOMContentLoaded', listener);
-            fulfill();
-          });
-        });
-        if (!domWindow.gBrowser)
-          return;
-        domWindow.gBrowser.tabContainer.addEventListener('TabOpen', event => onTabOpenListener(domWindow, event));
-        domWindow.gBrowser.tabContainer.addEventListener('TabClose', onTabCloseListener);
-      },
-      onCloseWindow: window => {
-        const domWindow = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowInternal || Ci.nsIDOMWindow);
-        if (!(domWindow instanceof Ci.nsIDOMChromeWindow))
-          return;
-        if (!domWindow.gBrowser)
-          return;
-        domWindow.gBrowser.tabContainer.removeEventListener('TabOpen', onTabOpenListener);
-        domWindow.gBrowser.tabContainer.removeEventListener('TabClose', onTabCloseListener);
-        for (const tab of domWindow.gBrowser.tabs)
-          onTabCloseListener({ target: tab });
-      },
-    });
+    const domWindowTabListeners = new Map();
+
+    const onOpenWindow = async (appWindow) => {
+      if (!(appWindow instanceof Ci.nsIAppWindow))
+        return;
+      const domWindow = appWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowInternal || Ci.nsIDOMWindow);
+      if (!(domWindow instanceof Ci.nsIDOMChromeWindow))
+        return;
+      if (domWindow.document.readyState !== 'uninitialized')
+        throw new Error('DOMWindow should not be loaded yet');
+      await helper.awaitEvent(domWindow, 'DOMContentLoaded');
+
+      if (!domWindow.gBrowser)
+        return;
+      const tabContainer = domWindow.gBrowser.tabContainer;
+      domWindowTabListeners.set(domWindow, [
+        helper.addEventListener(tabContainer, 'TabOpen', event => onTabOpenListener(appWindow, domWindow, event)),
+        helper.addEventListener(tabContainer, 'TabClose', onTabCloseListener),
+      ]);
+      for (const tab of domWindow.gBrowser.tabs)
+        onTabOpenListener(appWindow, domWindow, { target: tab });
+    };
+
+    const onCloseWindow = window => {
+      const domWindow = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowInternal || Ci.nsIDOMWindow);
+      if (!(domWindow instanceof Ci.nsIDOMChromeWindow))
+        return;
+      if (!domWindow.gBrowser)
+        return;
+
+      const listeners = domWindowTabListeners.get(domWindow) || [];
+      domWindowTabListeners.delete(domWindow);
+      helper.removeListeners(listeners);
+      for (const tab of domWindow.gBrowser.tabs)
+        onTabCloseListener({ target: tab });
+    };
+
+    Services.wm.addListener({ onOpenWindow, onCloseWindow });
+    for (const win of Services.wm.getEnumerator(null))
+      onOpenWindow(win);
 
     const extHelperAppSvc = Cc["@mozilla.org/uriloader/external-helper-app-service;1"].getService(Ci.nsIExternalHelperAppService);
     extHelperAppSvc.setDownloadInterceptor(new DownloadInterceptor(this));
@@ -308,7 +319,7 @@ class TargetRegistry {
     const window = Services.ww.openWindow(null, AppConstants.BROWSER_CHROME_URL, '_blank', features, args);
     await waitForWindowReady(window);
     if (window.gBrowser.browsers.length !== 1)
-      throw new Error(`Unexpcted number of tabs in the new window: ${window.gBrowser.browsers.length}`);
+      throw new Error(`Unexpected number of tabs in the new window: ${window.gBrowser.browsers.length}`);
     const browser = window.gBrowser.browsers[0];
     const target = this._browserToTarget.get(browser) || await new Promise(fulfill => {
       const listener = helper.on(this, TargetRegistry.Events.TargetCreated, ({target}) => {
@@ -318,8 +329,6 @@ class TargetRegistry {
         }
       });
     });
-    if (browserContext && browserContext.defaultViewportSize)
-      setViewportSizeForBrowser(browserContext.defaultViewportSize, browser, window);
     browser.focus();
     if (browserContext.settings.timezoneId) {
       if (await target.hasFailedToOverrideTimezone())
@@ -373,7 +382,6 @@ class PageTarget {
 
     this._disposed = false;
     browserContext.pages.add(this);
-    browserContext._firstPageCallback();
     this._registry._browserToTarget.set(this._linkedBrowser, this);
     this._registry._browserBrowsingContextToTarget.set(this._linkedBrowser.browsingContext, this);
   }
@@ -501,7 +509,6 @@ class BrowserContext {
     this.bindings = [];
     this.settings = {};
     this.pages = new Set();
-    this._firstPagePromise = new Promise(f => this._firstPageCallback = f);
   }
 
   async destroy() {
@@ -545,11 +552,6 @@ class BrowserContext {
 
   async setDefaultViewport(viewport) {
     this.defaultViewportSize = viewport ? viewport.viewportSize : undefined;
-    if (!this.userContextId) {
-      // First page in the default context comes before onTabOpenListener
-      // so we don't set default viewport. Wait for it here and ensure the viewport.
-      await this._firstPagePromise;
-    }
     const promises = Array.from(this.pages).map(async page => {
       // Resize to new default, unless the page has a custom viewport.
       if (!page._viewportSize)
@@ -707,14 +709,8 @@ async function waitForWindowReady(window) {
       }, "browser-delayed-startup-finished");
     }));
   }
-  if (window.document.readyState !== 'complete') {
-    await new Promise(fulfill => {
-      window.addEventListener('load', function listener() {
-        window.removeEventListener('load', listener);
-        fulfill();
-      });
-    });
-  }
+  if (window.document.readyState !== 'complete')
+    await helper.awaitEvent(window, 'load');
 }
 
 function setViewportSizeForBrowser(viewportSize, browser, window) {

diff --git a/browser_patches/firefox/patches/bootstrap.diff b/browser_patches/firefox/patches/bootstrap.diff
@@ -1547,6 +1547,20 @@ index 66df8509044600a0d71eb36bb838f96087a53ef1..e4558874434e3aa57bc26344f0ca89b3
        ? "https://firefox.settings.services.mozilla.com/v1"
        : gServerURL;
    },
+diff --git a/toolkit/components/browser/nsIWebBrowserChrome.idl b/toolkit/components/browser/nsIWebBrowserChrome.idl
+index 1e9bea1655af731fc003f8d0cab3ad4d2ad29f5d..5081c0e1ee0c41c6a79bd2ed358a57442e3baa6b 100644
+--- a/toolkit/components/browser/nsIWebBrowserChrome.idl
++++ b/toolkit/components/browser/nsIWebBrowserChrome.idl
+@@ -70,6 +70,9 @@ interface nsIWebBrowserChrome : nsISupports
+     // Whether this window should use out-of-process cross-origin subframes.
+     const unsigned long CHROME_FISSION_WINDOW         = 0x00200000;
+
++    // Whether this window has "width" or "height" defined in features
++    const unsigned long JUGGLER_WINDOW_EXPLICIT_SIZE  = 0x00400000;
++
+     // Prevents new window animations on MacOS and Windows. Currently
+     // ignored for Linux.
+     const unsigned long CHROME_SUPPRESS_ANIMATION     = 0x01000000;
 diff --git a/toolkit/components/startup/nsAppStartup.cpp b/toolkit/components/startup/nsAppStartup.cpp
 index 4eebb0e55ab3622e8f1f55655ef096d919e0ecb1..fb0e29281393f9c68d14a6549d3a9988569b0366 100644
 --- a/toolkit/components/startup/nsAppStartup.cpp
@@ -1583,6 +1597,21 @@ index 318037b12e9ea7b8bad92498950ac48ff936fb3c..44db941025a5253da38572600cd0fc57
                                          int32_t aCurSelfProgress,
                                          int32_t aMaxSelfProgress,
                                          int32_t aCurTotalProgress,
+diff --git a/toolkit/components/windowwatcher/nsWindowWatcher.cpp b/toolkit/components/windowwatcher/nsWindowWatcher.cpp
+index 876b1ae05bbc2bdf725b749687b1c86e63b1f225..be0aee2bcc5cf0a0576d953b31e85d1d84bfd245 100644
+--- a/toolkit/components/windowwatcher/nsWindowWatcher.cpp
++++ b/toolkit/components/windowwatcher/nsWindowWatcher.cpp
+@@ -1796,6 +1796,10 @@ uint32_t nsWindowWatcher::CalculateChromeFlagsForContent(
+   uint32_t chromeFlags = CalculateChromeFlagsHelper(
+       nsIWebBrowserChrome::CHROME_WINDOW_BORDERS, aFeatures, aSizeSpec);
+
++  if (aFeatures.Exists("width") || aFeatures.Exists("height")) {
++    chromeFlags |= nsIWebBrowserChrome::JUGGLER_WINDOW_EXPLICIT_SIZE;
++  }
++
+   return EnsureFlagsSafeForContent(chromeFlags);
+ }
+
 diff --git a/toolkit/mozapps/update/UpdateService.jsm b/toolkit/mozapps/update/UpdateService.jsm
 index 32a9ac1478a20ecfcf5c5fa1cefe8468b122b895..3aaa78c4558019c9d1d76b3c4204beae45f7b70c 100644
 --- a/toolkit/mozapps/update/UpdateService.jsm
Original file line number	Diff line number	Diff line change
		@@ -1,2 +1,2 @@
		1174
		Changed: [email protected] Tue Sep 29 02:02:37 MDT 2020
		1175
		Changed: [email protected] Wed Sep 30 00:44:40 MDT 2020