fix: separate local storage between WebView processes #54
Conversation
ItsChaceD
requested review from
a team,
OS-ruimoreiramendes,
alexgerardojacinto,
andredestro,
capacitor-bot,
markemer and
theproducer
and removed request for
a team
| val event = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { | ||
| intent.getSerializableExtra(EXTRA_EVENT_DATA, OSIABEvents::class.java) | ||
| } else { | ||
| intent.getSerializableExtra(EXTRA_EVENT_DATA) as? OSIABEvents | ||
| } |
There was a problem hiding this comment.
Minor: We can probably replace this with a one-liner for IntentCompat#getSerializableExtra (assuming the library already has an AndroidX dependency with the method available)
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.TIRAMISU) { | ||
| appContext.registerReceiver(receiver, filter, Context.RECEIVER_NOT_EXPORTED) | ||
| } else { | ||
| appContext.registerReceiver(receiver, filter) | ||
| } |
There was a problem hiding this comment.
Minor: Can maybe replace this with a one-liner ContextCompat#registerReceiver (assuming the library already has an AndroidX dependency that comes with this)
| const val EXTRA_EVENT_DATA = "com.outsystems.plugins.inappbrowser.osinappbrowserlib.EXTRA_EVENT_DATA" | ||
|
|
||
| private val _events = MutableSharedFlow<OSIABEvents>() | ||
| private val _events = MutableSharedFlow<OSIABEvents>(extraBufferCapacity = 64) |
There was a problem hiding this comment.
Why was this added?
| OSIABEvents.postEvent(event) | ||
| OSIABEvents.broadcastEvent(this@OSIABWebViewActivity, event) |
There was a problem hiding this comment.
Should we be calling both methods here?
| private var receiver: BroadcastReceiver? = null | ||
|
|
||
| @SuppressLint("UnspecifiedRegisterReceiverFlag") | ||
| fun registerReceiver(context: Context) { |
There was a problem hiding this comment.
So this is necessary because the WebView activity now runs in a separate process?
Wouldn't this be necessary for CustomTabs as well before-hand?
| @Target(AnnotationTarget.FUNCTION, AnnotationTarget.CLASS) | ||
| annotation class RequiresEventBridgeRegistration | ||
|
|
||
| sealed class OSIABEvents : Serializable { |
There was a problem hiding this comment.
Feels confusing for this class to be Serializable, subclasses have Context and Activity which are not serializable. The serialization is only relevant for the broadcast receiver, which is only called for webview, but from what I see in the PR you don't set OSIABWebViewActivity anymore for OSIABWebViewEvent.
Can you explain why you implemented it that way?
| flowHelper: OSIABFlowHelperInterface, | ||
| customTabsSessionCallback: (CustomTabsSession?) -> Unit | ||
| ) { | ||
| OSIABEvents.registerReceiver(context) |
There was a problem hiding this comment.
This is being called here but broadcastEvent isn't called here, so wondering about its usefulness.
Same for OSIABCustomTabsRouterAdapter
| private var receiver: BroadcastReceiver? = null | ||
|
|
||
| @SuppressLint("UnspecifiedRegisterReceiverFlag") | ||
| fun registerReceiver(context: Context) { |
There was a problem hiding this comment.
Wondering if we should have a way to unregister the receiver, e.g. after Close events are submitted, can we get rid of the receiver, rather than it living throughout the entire app's lifecycle?
| <activity | ||
| android:name=".views.OSIABWebViewActivity" | ||
| android:exported="false" | ||
| android:process=":OSInAppBrowser" |
There was a problem hiding this comment.
We should make sure plugins (OutSystems especially) document this change, wouldn't be surprised if it ends up breaking existing Android apps in some unexpected way, due to the many different ways people use InAppBrowser.
There was a problem hiding this comment.
Actually, doesn't this mean that existing apps that have cookies or local storage data for a certain webpage in InAppBrowser, will no longer have that data when they update the app to this version where the data directory changes?
Description
This PR implements localStorage isolation on Android API 28+ via setDataDirectorySuffix, bringing it into alignment with iOS behavior.
Due to Android system limitations, isolation can only be achieved by running the WebView in a separate
:OSInAppBrowserprocess with a dedicated data directory suffix. This requires events to be propagated between the processes via broadcast receivers. To avoid a breaking change, we keep the class signatures the same and add a custom@RequiresEventBridgeRegistrationannotation to proactively warn native library users of the new registration requirement.Context
https://outsystemsrd.atlassian.net/browse/RMET-4918?atlOrigin=eyJpIjoiOWI4NWVmMDIwZDhhNDRmMGJhMTljMWFlNTQxYmFkZjIiLCJwIjoiaiJ9
Type of changes
Platforms affected