Skip to content
This repository has been archived by the owner on Jun 17, 2024. It is now read-only.

Commit

Permalink
Bug 1880476 — Remove redundant caching and destroy jexl helper after use
Browse files Browse the repository at this point in the history
  • Loading branch information
@jhugman @mergify
jhugman authored and mergify[bot] committed Feb 20, 2024
1 parent ff739ce commit 0d8d7a0
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 46 deletions.
74 changes: 42 additions & 32 deletions ...imbus/src/main/java/mozilla/components/service/nimbus/messaging/NimbusMessagingStorage.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -52,8 +52,15 @@ class NimbusMessagingStorage(
private val customAttributes: JSONObject
get() = attributeProvider?.getCustomAttributes(context) ?: JSONObject()

val helper: NimbusMessagingHelperInterface
get() = gleanPlumb.createMessageHelper(customAttributes)
/**
* Returns a Nimbus message helper, for evaluating JEXL.
*
* The JEXL context is time-sensitive, so this should be created new for each set of evaluations.
*
* Since it has a native peer, it should be [destroy]ed after finishing the set of evaluations.
*/
fun createMessagingHelper(): NimbusMessagingHelperInterface =
gleanPlumb.createMessageHelper(customAttributes)

/**
* Returns the [Message] for the given [key] or returns null if none found.
Expand Down Expand Up @@ -113,26 +120,26 @@ class NimbusMessagingStorage(
* Returns the next higher priority message which all their triggers are true.
*/
fun getNextMessage(surface: MessageSurfaceId, availableMessages: List<Message>): Message? =
getNextMessage(
surface,
availableMessages,
setOf(),
helper,
mutableMapOf(),
)
createMessagingHelper().use {
getNextMessage(
surface,
availableMessages,
setOf(),
it,
)
}

@Suppress("ReturnCount")
private fun getNextMessage(
surface: MessageSurfaceId,
availableMessages: List<Message>,
excluded: Set<String>,
helper: NimbusMessagingHelperInterface,
jexlCache: MutableMap<String, Boolean>,
): Message? {
val message = availableMessages
.filter { surface == it.surface }
.filter { !excluded.contains(it.id) }
.firstOrNull { isMessageEligible(it, helper, jexlCache) } ?: return null
.firstOrNull { isMessageEligible(it, helper) } ?: return null

val slug = message.data.experiment
if (slug != null) {
Expand Down Expand Up @@ -167,7 +174,6 @@ class NimbusMessagingStorage(
availableMessages,
excluded + message.id,
helper,
jexlCache,
)

ControlMessageBehavior.SHOW_NONE -> null
Expand All @@ -189,12 +195,12 @@ class NimbusMessagingStorage(
* The fully resolved (with all substitutions) action is returned as the second value
* of the [Pair].
*/
fun generateUuidAndFormatAction(action: String): Pair<String?, String> {
val helper = gleanPlumb.createMessageHelper(customAttributes)
val uuid = helper.getUuid(action)

return Pair(uuid, helper.stringFormat(action, uuid))
}
fun generateUuidAndFormatAction(action: String): Pair<String?, String> =
createMessagingHelper().use { helper ->
val uuid = helper.getUuid(action)
val url = helper.stringFormat(action, uuid)
Pair(uuid, url)
}

/**
* Updated the provided [metadata] in the storage.
Expand Down Expand Up @@ -259,23 +265,19 @@ class NimbusMessagingStorage(
fun isMessageEligible(
message: Message,
helper: NimbusMessagingHelperInterface,
jexlCache: MutableMap<String, Boolean> = mutableMapOf(),
): Boolean {
return message.triggers.all { condition ->
jexlCache[condition]
?: try {
if (malFormedMap.containsKey(condition)) {
return false
}
helper.evalJexl(condition).also { result ->
jexlCache[condition] = result
}
} catch (e: NimbusException.EvaluationException) {
reportMalformedMessage(message.id)
malFormedMap[condition] = message.id
logger.info("Unable to evaluate $condition")
false
try {
if (malFormedMap.containsKey(condition)) {
return false
}
helper.evalJexl(condition)
} catch (e: NimbusException.EvaluationException) {
reportMalformedMessage(message.id)
malFormedMap[condition] = message.id
logger.info("Unable to evaluate $condition")
false
}
}
}

Expand All @@ -290,3 +292,11 @@ class NimbusMessagingStorage(
)
}
}

/**
* A helper method to safely destroy the message helper after use.
*/
fun <R> NimbusMessagingHelperInterface.use(block: (NimbusMessagingHelperInterface) -> R) =
block(this).also {
this.destroy()
}
14 changes: 8 additions & 6 deletions ...s/src/test/java/mozilla/components/service/nimbus/messaging/NimbusMessagingStorageTest.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,8 @@ class NimbusMessagingStorageTest {
gleanPlumb,
messagingFeature,
)

`when`(gleanPlumb.createMessageHelper(any())).thenReturn(mock())
}

@After
Expand Down Expand Up @@ -489,7 +491,7 @@ class NimbusMessagingStorageTest {
Message.Metadata("same-id"),
)

doReturn(false).`when`(spiedStorage).isMessageEligible(any(), any(), any())
doReturn(false).`when`(spiedStorage).isMessageEligible(any(), any())

val result = spiedStorage.getNextMessage(HOMESCREEN, listOf(message))

Expand All @@ -508,7 +510,7 @@ class NimbusMessagingStorageTest {
Message.Metadata("same-id"),
)

doReturn(true).`when`(spiedStorage).isMessageEligible(any(), any(), any())
doReturn(true).`when`(spiedStorage).isMessageEligible(any(), any())

val result = spiedStorage.getNextMessage(HOMESCREEN, listOf(message))

Expand All @@ -530,7 +532,7 @@ class NimbusMessagingStorageTest {
Message.Metadata("same-id"),
)

doReturn(true).`when`(spiedStorage).isMessageEligible(any(), any(), any())
doReturn(true).`when`(spiedStorage).isMessageEligible(any(), any())

val result = spiedStorage.getNextMessage(HOMESCREEN, listOf(message))

Expand Down Expand Up @@ -565,7 +567,7 @@ class NimbusMessagingStorageTest {
Message.Metadata("same-id"),
)

doReturn(true).`when`(spiedStorage).isMessageEligible(any(), any(), any())
doReturn(true).`when`(spiedStorage).isMessageEligible(any(), any())

val result = spiedStorage.getNextMessage(
HOMESCREEN,
Expand Down Expand Up @@ -603,7 +605,7 @@ class NimbusMessagingStorageTest {
Message.Metadata("same-id"),
)

doReturn(true).`when`(spiedStorage).isMessageEligible(any(), any(), any())
doReturn(true).`when`(spiedStorage).isMessageEligible(any(), any())

val result = spiedStorage.getNextMessage(
HOMESCREEN,
Expand Down Expand Up @@ -652,7 +654,7 @@ class NimbusMessagingStorageTest {
Message.Metadata("same-id"),
)

doReturn(true).`when`(spiedStorage).isMessageEligible(any(), any(), any())
doReturn(true).`when`(spiedStorage).isMessageEligible(any(), any())

var result = spiedStorage.getNextMessage(
HOMESCREEN,
Expand Down
19 changes: 11 additions & 8 deletions fenix/app/src/main/java/org/mozilla/fenix/onboarding/OnboardingFragment.kt
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import androidx.fragment.app.Fragment
import androidx.localbroadcastmanager.content.LocalBroadcastManager
import androidx.navigation.fragment.findNavController
import mozilla.components.service.nimbus.evalJexlSafe
import mozilla.components.service.nimbus.messaging.use
import mozilla.components.support.base.ext.areNotificationsEnabledSafe
import mozilla.components.support.utils.BrowsersCache
import org.mozilla.fenix.R
Expand Down Expand Up @@ -228,7 +229,7 @@ class OnboardingFragment : Fragment() {
showAddWidgetPage: Boolean,
): List<OnboardingPageUiData> {
val jexlConditions = FxNimbus.features.junoOnboarding.value().conditions
val jexlHelper = requireContext().components.analytics.messagingStorage.helper
val jexlHelper = requireContext().components.analytics.messagingStorage.createMessagingHelper()

val privacyCaption = Caption(
text = getString(R.string.juno_onboarding_privacy_notice_text),
Expand All @@ -249,12 +250,14 @@ class OnboardingFragment : Fragment() {
},
),
)
return FxNimbus.features.junoOnboarding.value().cards.values.toPageUiData(
privacyCaption,
showDefaultBrowserPage,
showNotificationPage,
showAddWidgetPage,
jexlConditions,
) { condition -> jexlHelper.evalJexlSafe(condition) }
return jexlHelper.use {
FxNimbus.features.junoOnboarding.value().cards.values.toPageUiData(
privacyCaption,
showDefaultBrowserPage,
showNotificationPage,
showAddWidgetPage,
jexlConditions,
) { condition -> jexlHelper.evalJexlSafe(condition) }
}
}
}

0 comments on commit 0d8d7a0

Please sign in to comment.