Closes mozilla-mobile#8431: Notify all new observers about

change on enableTrackingProtection
Amejia481 · Sep 17, 2020 · 63e8206 · 63e8206
1 parent a1a5c58
commit 63e8206
Show file tree

Hide file tree

Showing 8 changed files with 173 additions and 26 deletions.
diff --git a/...ne-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt b/...ne-gecko-beta/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt
@@ -223,14 +223,31 @@ class GeckoEngineSession(
             policy.contains(TrackingProtectionPolicy.TrackingCategory.SCRIPTS_AND_SUB_RESOURCES)
 
         geckoSession.settings.useTrackingProtection = shouldBlockContent && enabled
-        notifyAtLeastOneObserver {
-            // We now register engine observers in a middleware using a dedicated
-            // store thread. Since this notification can be delayed until an observer
-            // is registered we switch to the main scope to make sure we're not notifying
-            // on the store thread.
-            MainScope().launch {
-                onTrackerBlockingEnabledChange(enabled)
-            }
+        etpEnabled = enabled
+        notifyObservers {
+            onTrackerBlockingEnabledChange(this, enabled)
+        }
+    }
+
+    // This is a temporary solution to address
+    // https://github.com/mozilla-mobile/android-components/issues/8431
+    // until we eventually delete [EngineObserver] then this will not be needed.
+    private var etpEnabled: Boolean? = null
+
+    override fun register(observer: Observer) {
+        super.register(observer)
+        etpEnabled?.let { enabled ->
+            onTrackerBlockingEnabledChange(observer, enabled)
+        }
+    }
+
+    private fun onTrackerBlockingEnabledChange(observer: Observer, enabled: Boolean) {
+        // We now register engine observers in a middleware using a dedicated
+        // store thread. Since this notification can be delayed until an observer
+        // is registered we switch to the main scope to make sure we're not notifying
+        // on the store thread.
+        MainScope().launch {
+            observer.onTrackerBlockingEnabledChange(enabled)
         }
     }
 

diff --git a/...ecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt b/...ecko-beta/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt
@@ -1151,6 +1151,37 @@ class GeckoEngineSessionTest {
         assertFalse(engineSession.geckoSession.settings.useTrackingProtection)
     }
 
+    @Test
+    fun `changes to enableTrackingProtection will be notified to all new observers`() {
+        whenever(runtime.settings).thenReturn(mock())
+        whenever(runtime.settings.contentBlocking).thenReturn(mock())
+        val session = GeckoEngineSession(runtime, geckoSessionProvider = geckoSessionProvider)
+        val observers = mutableListOf<EngineSession.Observer>()
+        val policy = TrackingProtectionPolicy.strict()
+
+        for (x in 1..5) {
+            observers.add(spy(object : EngineSession.Observer {}))
+        }
+
+        session.enableTrackingProtection(policy)
+
+        observers.forEach { session.register(it) }
+
+        observers.forEach {
+            verify(it).onTrackerBlockingEnabledChange(true)
+        }
+
+        observers.forEach { session.unregister(it) }
+
+        session.enableTrackingProtection(policy.forPrivateSessionsOnly())
+
+        observers.forEach { session.register(it) }
+
+        observers.forEach {
+            verify(it).onTrackerBlockingEnabledChange(false)
+        }
+    }
+
     @Test
     fun safeBrowsingCategoriesAreAligned() {
         assertEquals(GeckoSafeBrowsing.NONE, SafeBrowsingPolicy.NONE.id)

diff --git a/...gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt b/...gecko-nightly/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt
@@ -223,14 +223,31 @@ class GeckoEngineSession(
             policy.contains(TrackingProtectionPolicy.TrackingCategory.SCRIPTS_AND_SUB_RESOURCES)
 
         geckoSession.settings.useTrackingProtection = shouldBlockContent && enabled
-        notifyAtLeastOneObserver {
-            // We now register engine observers in a middleware using a dedicated
-            // store thread. Since this notification can be delayed until an observer
-            // is registered we switch to the main scope to make sure we're not notifying
-            // on the store thread.
-            MainScope().launch {
-                onTrackerBlockingEnabledChange(enabled)
-            }
+        etpEnabled = enabled
+        notifyObservers {
+            onTrackerBlockingEnabledChange(this, enabled)
+        }
+    }
+
+    // This is a temporary solution to address
+    // https://github.com/mozilla-mobile/android-components/issues/8431
+    // until we eventually delete [EngineObserver] then this will not be needed.
+    private var etpEnabled: Boolean? = null
+
+    override fun register(observer: Observer) {
+        super.register(observer)
+        etpEnabled?.let { enabled ->
+            onTrackerBlockingEnabledChange(observer, enabled)
+        }
+    }
+
+    private fun onTrackerBlockingEnabledChange(observer: Observer, enabled: Boolean) {
+        // We now register engine observers in a middleware using a dedicated
+        // store thread. Since this notification can be delayed until an observer
+        // is registered we switch to the main scope to make sure we're not notifying
+        // on the store thread.
+        MainScope().launch {
+            observer.onTrackerBlockingEnabledChange(enabled)
         }
     }
 

diff --git a/...o-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt b/...o-nightly/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt
@@ -1153,6 +1153,37 @@ class GeckoEngineSessionTest {
         assertFalse(engineSession.geckoSession.settings.useTrackingProtection)
     }
 
+    @Test
+    fun `changes to enableTrackingProtection will be notified to all new observers`() {
+        whenever(runtime.settings).thenReturn(mock())
+        whenever(runtime.settings.contentBlocking).thenReturn(mock())
+        val session = GeckoEngineSession(runtime, geckoSessionProvider = geckoSessionProvider)
+        val observers = mutableListOf<EngineSession.Observer>()
+        val policy = TrackingProtectionPolicy.strict()
+
+        for (x in 1..5) {
+            observers.add(spy(object : EngineSession.Observer {}))
+        }
+
+        session.enableTrackingProtection(policy)
+
+        observers.forEach { session.register(it) }
+
+        observers.forEach {
+            verify(it).onTrackerBlockingEnabledChange(true)
+        }
+
+        observers.forEach { session.unregister(it) }
+
+        session.enableTrackingProtection(policy.forPrivateSessionsOnly())
+
+        observers.forEach { session.register(it) }
+
+        observers.forEach {
+            verify(it).onTrackerBlockingEnabledChange(false)
+        }
+    }
+
     @Test
     fun safeBrowsingCategoriesAreAligned() {
         assertEquals(GeckoSafeBrowsing.NONE, SafeBrowsingPolicy.NONE.id)

diff --git a/.../engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt b/.../engine-gecko/src/main/java/mozilla/components/browser/engine/gecko/GeckoEngineSession.kt
@@ -223,14 +223,31 @@ class GeckoEngineSession(
             policy.contains(TrackingProtectionPolicy.TrackingCategory.SCRIPTS_AND_SUB_RESOURCES)
 
         geckoSession.settings.useTrackingProtection = shouldBlockContent && enabled
-        notifyAtLeastOneObserver {
-            // We now register engine observers in a middleware using a dedicated
-            // store thread. Since this notification can be delayed until an observer
-            // is registered we switch to the main scope to make sure we're not notifying
-            // on the store thread.
-            MainScope().launch {
-                onTrackerBlockingEnabledChange(enabled)
-            }
+        etpEnabled = enabled
+        notifyObservers {
+            onTrackerBlockingEnabledChange(this, enabled)
+        }
+    }
+
+    // This is a temporary solution to address
+    // https://github.com/mozilla-mobile/android-components/issues/8431
+    // until we eventually delete [EngineObserver] then this will not be needed.
+    private var etpEnabled: Boolean? = null
+
+    override fun register(observer: Observer) {
+        super.register(observer)
+        etpEnabled?.let { enabled ->
+            onTrackerBlockingEnabledChange(observer, enabled)
+        }
+    }
+
+    private fun onTrackerBlockingEnabledChange(observer: Observer, enabled: Boolean) {
+        // We now register engine observers in a middleware using a dedicated
+        // store thread. Since this notification can be delayed until an observer
+        // is registered we switch to the main scope to make sure we're not notifying
+        // on the store thread.
+        MainScope().launch {
+            observer.onTrackerBlockingEnabledChange(enabled)
         }
     }
 

diff --git a/...ine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt b/...ine-gecko/src/test/java/mozilla/components/browser/engine/gecko/GeckoEngineSessionTest.kt
@@ -1149,6 +1149,37 @@ class GeckoEngineSessionTest {
         assertFalse(engineSession.geckoSession.settings.useTrackingProtection)
     }
 
+    @Test
+    fun `changes to enableTrackingProtection will be notified to all new observers`() {
+        whenever(runtime.settings).thenReturn(mock())
+        whenever(runtime.settings.contentBlocking).thenReturn(mock())
+        val session = GeckoEngineSession(runtime, geckoSessionProvider = geckoSessionProvider)
+        val observers = mutableListOf<EngineSession.Observer>()
+        val policy = TrackingProtectionPolicy.strict()
+
+        for (x in 1..5) {
+            observers.add(Mockito.spy(object : EngineSession.Observer {}))
+        }
+
+        session.enableTrackingProtection(policy)
+
+        observers.forEach { session.register(it) }
+
+        observers.forEach {
+            verify(it).onTrackerBlockingEnabledChange(true)
+        }
+
+        observers.forEach { session.unregister(it) }
+
+        session.enableTrackingProtection(policy.forPrivateSessionsOnly())
+
+        observers.forEach { session.register(it) }
+
+        observers.forEach {
+            verify(it).onTrackerBlockingEnabledChange(false)
+        }
+    }
+
     @Test
     fun safeBrowsingCategoriesAreAligned() {
         assertEquals(GeckoSafeBrowsing.NONE, SafeBrowsingPolicy.NONE.id)

diff --git a/...s/support/base/src/main/java/mozilla/components/support/base/observer/ObserverRegistry.kt b/...s/support/base/src/main/java/mozilla/components/support/base/observer/ObserverRegistry.kt
@@ -25,7 +25,7 @@ import java.util.WeakHashMap
  * ObserverRegistry is thread-safe.
  */
 @Suppress("TooManyFunctions")
-class ObserverRegistry<T> : Observable<T> {
+open class ObserverRegistry<T> : Observable<T> {
     private val observers = mutableSetOf<T>()
     private val lifecycleObservers = WeakHashMap<T, LifecycleBoundObserver<T>>()
     private val viewObservers = WeakHashMap<T, ViewBoundObserver<T>>()
@@ -39,7 +39,7 @@ class ObserverRegistry<T> : Observable<T> {
      * @param observer the observer to register.
      */
     @Synchronized
-    override fun register(observer: T) {
+    open override fun register(observer: T) {
         observers.add(observer)
 
         while (!queuedNotifications.isEmpty()) {

diff --git a/docs/changelog.md b/docs/changelog.md
@@ -12,6 +12,9 @@ permalink: /changelog/
 * [Gecko](https://github.com/mozilla-mobile/android-components/blob/master/buildSrc/src/main/java/Gecko.kt)
 * [Configuration](https://github.com/mozilla-mobile/android-components/blob/master/.config.yml)
 
+* **browser-engine-gecko**, **browser-engine-gecko-beta**, **browser-engine-gecko-nightly**
+  * 🚒 Bug fixed [issue #8431](https://github.com/mozilla-mobile/android-components/issues/8431) SpeculativeSessionObserver is swallowing events meant to be for EngineObserver.
+
 # 59.0.0
 
 * [Commits](https://github.com/mozilla-mobile/android-components/compare/v58.0.0...v59.0.0)