From 73923d06c0cd63addf6f902f15da376adbb4f851 Mon Sep 17 00:00:00 2001 From: Agi Sferro Date: Tue, 27 Oct 2020 18:41:58 +0000 Subject: [PATCH] Bug 1673316 - Ensure docShellIsActive is preserved when switching process. r=snorp I'm not a fan of accessing private bits like `docShellIsActive` in tests like that but it's both: 1) very important, if we don't correctly activate docShell performance will be very poor 2) stable, this API is not likely to change, and if it changes it should be easy to get whatever the replacement is Differential Revision: https://phabricator.services.mozilla.com/D94874 --- mobile/android/chrome/geckoview/geckoview.js | 9 +++++++++ .../assets/web_extensions/test-support/background.js | 3 +++ .../assets/web_extensions/test-support/test-api.js | 5 +++++ .../web_extensions/test-support/test-schema.json | 12 ++++++++++++ .../org/mozilla/geckoview/test/BaseSessionTest.kt | 4 ++++ .../mozilla/geckoview/test/NavigationDelegateTest.kt | 12 ++++++++++++ .../mozilla/geckoview/test/SessionLifecycleTest.kt | 6 ++++++ .../geckoview/test/rule/GeckoSessionTestRule.java | 6 ++++++ 8 files changed, 57 insertions(+) diff --git a/mobile/android/chrome/geckoview/geckoview.js b/mobile/android/chrome/geckoview/geckoview.js index 7e9a0c360f86..40a7eafc715a 100644 --- a/mobile/android/chrome/geckoview/geckoview.js +++ b/mobile/android/chrome/geckoview/geckoview.js @@ -186,9 +186,18 @@ var ModuleManager = { this.forEach(module => { module.onDestroyBrowser(); }); + + // TODO: Bug 1673683: `docShellIsActive` is sometimes not preserved when + // switching process. + this.docShellIsActiveWhileSwitchingProcess = this.browser.docShellIsActive; }, didChangeBrowserRemoteness() { + debug`DidChangeBrowserRemoteness`; + + this.browser.docShellIsActive = this.docShellIsActiveWhileSwitchingProcess; + this.docShellIsActiveWhileSwitchingProcess = undefined; + this.forEach(module => { if (module.impl) { module.impl.onInitBrowser(); diff --git a/mobile/android/geckoview/src/androidTest/assets/web_extensions/test-support/background.js b/mobile/android/geckoview/src/androidTest/assets/web_extensions/test-support/background.js index 3fa907c95c09..3317e996173b 100644 --- a/mobile/android/geckoview/src/androidTest/assets/web_extensions/test-support/background.js +++ b/mobile/android/geckoview/src/androidTest/assets/web_extensions/test-support/background.js @@ -23,6 +23,9 @@ const APIS = { GetPrefs({ prefs }) { return browser.test.getPrefs(prefs); }, + GetActive({ tab }) { + return browser.test.getActive(tab.id); + }, RemoveCertOverride({ host, port }) { browser.test.removeCertOverride(host, port); }, diff --git a/mobile/android/geckoview/src/androidTest/assets/web_extensions/test-support/test-api.js b/mobile/android/geckoview/src/androidTest/assets/web_extensions/test-support/test-api.js index ad4c8d6168f8..1ed6c4055415 100644 --- a/mobile/android/geckoview/src/androidTest/assets/web_extensions/test-support/test-api.js +++ b/mobile/android/geckoview/src/androidTest/assets/web_extensions/test-support/test-api.js @@ -180,6 +180,11 @@ this.test = class extends ExtensionAPI { }); }, + async getActive(tabId) { + const tab = context.extension.tabManager.get(tabId); + return tab.browser.docShellIsActive; + }, + async flushApzRepaints(tabId) { const tab = context.extension.tabManager.get(tabId); const { browsingContext } = tab.browser; diff --git a/mobile/android/geckoview/src/androidTest/assets/web_extensions/test-support/test-schema.json b/mobile/android/geckoview/src/androidTest/assets/web_extensions/test-support/test-schema.json index 4e09a1a35dff..65ab7d48487f 100644 --- a/mobile/android/geckoview/src/androidTest/assets/web_extensions/test-support/test-schema.json +++ b/mobile/android/geckoview/src/androidTest/assets/web_extensions/test-support/test-schema.json @@ -133,6 +133,18 @@ } ] }, + { + "name": "getActive", + "type": "function", + "async": true, + "description": "Returns true if the docShell is active for given tab.", + "parameters": [ + { + "type": "number", + "name": "tabId" + } + ] + }, { "name": "getPidForTab", "type": "function", diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/BaseSessionTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/BaseSessionTest.kt index c5108e14694d..cd4f506e1ea5 100644 --- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/BaseSessionTest.kt +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/BaseSessionTest.kt @@ -191,6 +191,10 @@ open class BaseSessionTest(noErrorCollector: Boolean = false) { fun GeckoSession.flushApzRepaints() = sessionRule.flushApzRepaints(this) + var GeckoSession.active: Boolean + get() = sessionRule.getActive(this) + set(value) = setActive(value) + @Suppress("UNCHECKED_CAST") fun Any?.asJsonArray(): JSONArray = this as JSONArray diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt index 68714d59db46..24f20d5b9f2d 100644 --- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/NavigationDelegateTest.kt @@ -1695,6 +1695,9 @@ class NavigationDelegateTest : BaseSessionTest() { mainSession.loadTestPath(HELLO2_HTML_PATH) sessionRule.waitForPageStop() + assertThat("docShell should start out active", mainSession.active, + equalTo(true)) + // This loads in the parent process mainSession.loadUri(url) sessionRule.waitForPageStop() @@ -1706,6 +1709,9 @@ class NavigationDelegateTest : BaseSessionTest() { sessionRule.waitForPageStop() assertThat("URL should match", currentUrl!!, endsWith(HELLO_HTML_PATH)) + assertThat("docShell should be active after switching process", + mainSession.active, + equalTo(true)) mainSession.loadUri(url) sessionRule.waitForPageStop() @@ -1716,6 +1722,9 @@ class NavigationDelegateTest : BaseSessionTest() { sessionRule.waitForPageStop() assertThat("URL should match", currentUrl!!, endsWith(HELLO_HTML_PATH)) + assertThat("docShell should be active after switching process", + mainSession.active, + equalTo(true)) sessionRule.session.goBack() sessionRule.waitForPageStop() @@ -1726,6 +1735,9 @@ class NavigationDelegateTest : BaseSessionTest() { sessionRule.waitForPageStop() assertThat("URL should match", currentUrl!!, endsWith(HELLO2_HTML_PATH)) + assertThat("docShell should be active after switching process", + mainSession.active, + equalTo(true)) settings.aboutConfigEnabled = aboutConfigEnabled } diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/SessionLifecycleTest.kt b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/SessionLifecycleTest.kt index 509fdf64bc9d..5ea0be06fef8 100644 --- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/SessionLifecycleTest.kt +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/SessionLifecycleTest.kt @@ -127,6 +127,8 @@ class SessionLifecycleTest : BaseSessionTest() { mainSession.loadTestPath(HELLO_HTML_PATH) mainSession.waitForPageStop() + assertThat("docShell should start active", mainSession.active, equalTo(true)) + // Deactivate the GeckoSession and confirm that rAF/setTimeout/etc callbacks do not run mainSession.setActive(false) mainSession.evaluateJS( @@ -139,9 +141,13 @@ class SessionLifecycleTest : BaseSessionTest() { mainSession.waitForJS("new Promise(resolve => { resolve() })") val isNotGreen = mainSession.evaluateJS("document.documentElement.style.backgroundColor !== 'green'") as Boolean assertThat("requestAnimationFrame has not run yet", isNotGreen, equalTo(true)) + assertThat("docShell shouldn't be active after calling setActive", + mainSession.active, equalTo(false)) // Reactivate the GeckoSession and confirm that rAF/setTimeout/etc callbacks now run mainSession.setActive(true) + assertThat("docShell should be active after calling setActive(true)", + mainSession.active, equalTo(true)) mainSession.waitForJS("new Promise(resolve => requestAnimationFrame(() => { resolve(); }))"); var isGreen = mainSession.evaluateJS("document.documentElement.style.backgroundColor === 'green'") as Boolean assertThat("requestAnimationFrame has run", isGreen, equalTo(true)) diff --git a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java index 4bb271dbb94f..62f9b71226e4 100644 --- a/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java +++ b/mobile/android/geckoview/src/androidTest/java/org/mozilla/geckoview/test/rule/GeckoSessionTestRule.java @@ -1911,6 +1911,12 @@ public class GeckoSessionTestRule implements TestRule { return dblPid.intValue(); } + public boolean getActive(final @NonNull GeckoSession session) { + final Boolean isActive = (Boolean) + webExtensionApiCall(session, "GetActive", null); + return isActive; + } + private Object waitForMessage(String id) { UiThreadUtils.waitForCondition(() -> mPendingMessages.containsKey(id), mTimeoutMillis);