Bugzilla

OK So updated current theory, what follows is my understanding (warning: things might be incorrect):

`ViewBoundFeatureWrapper` is used by AC to manage the lifetime of objects connected to a `View`. In particular, it's used to `start` and `stop` the `SessionFeature` which in turns will (indirectly) `setSession` and `release` the `GeckoView` instance associated to the feature.

By design, `GeckoView` can only have one session set at any given time, so it's crucial that only one `ViewBoundFeatureWrapper` "owns" a `GeckoView` at any time. Failure to do so, i.e. calling `setSession` with two different sessions or `release` twice in a row, will cause the crash in this bug.

Focus (and maybe Fenix too?), however, creates multiple `ViewBoundFeatureWrapper` instances for the same `GeckoView` when opening a custom tab, [here](https://searchfox.org/mozilla-mobile/rev/fa83a8e7b1d8a836bbf6a99e9243a52bddb9ab20/focus-android/app/src/main/java/org/mozilla/focus/activity/CustomTabActivity.kt#89) and [here](https://searchfox.org/mozilla-mobile/rev/fa83a8e7b1d8a836bbf6a99e9243a52bddb9ab20/focus-android/app/src/main/java/org/mozilla/focus/fragment/BrowserFragment.kt#189).

(1) Note now, that `EngineViewPresenter` will call `renderTab` every time any of the following properties change: the tab Id, the session associated to the tab, the "crashed" propery and the firstContentfulPaint property. `renderTab` will in turn call `setSession` on the `GeckoView` instance every time any of the above properties are updated, even if the property does not change which session is being displayed (e.g. in the case of the `firstContentfulPaint` event).

The bug is now possible with the following chain of events:

* User creates a custom tab, this creates two separate `ViewBoundFeatureWrapper` for the GeckoView in the custom tab
* User taps on the "Open in Focus" button, *while the page is loading*
* While migrating the tab to the main browser, Focus [calls `release`](https://searchfox.org/mozilla-mobile/rev/fa83a8e7b1d8a836bbf6a99e9243a52bddb9ab20/focus-android/app/src/main/java/org/mozilla/focus/fragment/BrowserFragment.kt#672) on one of the `ViewBoundFeatureWrapper` instances to `release` the Session from the `GeckoView` instance, but crucially, not on the other `ViewBoundFeatureWrapper` instance.
* While the tab is being migrated, a `FirstContenfulPaint` even fires. The first `ViewBoundFeatureWrapper` does not respond, as it was correctly stopped in the step above, but the other one does, calling `setSession` on the Custom Tab view as explained in (1).
* The tab finished migrating to the browser, which calls `setSession` again, this time on the main browser View, triggering the crash.

There are other combinations of events that trigger this bug in this area which I'm not going to go since they involve the same fix.

The two problems that I see here are:

* We create multiple `ViewBoundFeatureWrapper` for the same `GeckoView`. This is always going to end up badly, we should come up with a way to enforce that this never happens.
* We call `setSession` regardless of which event is causing `ifAnyChanged` to trigger in `EngineViewPresenter`. I think that's wrong and that could also cause problems, we should only do it if a relevant change in the `session` happened.

OK So updated current theory, what follows is my understanding (warning: things might be incorrect):

`ViewBoundFeatureWrapper` is used by AC to manage the lifetime of objects connected to a `View`. In particular, it's used to `start` and `stop` the `SessionFeature` which in turns will (indirectly) `setSession` and `release` the `GeckoView` instance associated to the feature.

By design, `GeckoView` can only have one session set at any given time, so it's crucial that only one `ViewBoundFeatureWrapper` "owns" a `GeckoView` at any time. Failure to do so, i.e. calling `setSession` with two different sessions or `release` twice in a row, will cause the crash in this bug.

Focus (and maybe Fenix too?), however, creates multiple `ViewBoundFeatureWrapper` instances for the same `GeckoView` when opening a custom tab, [here](https://searchfox.org/mozilla-mobile/rev/fa83a8e7b1d8a836bbf6a99e9243a52bddb9ab20/focus-android/app/src/main/java/org/mozilla/focus/activity/CustomTabActivity.kt#89) and [here](https://searchfox.org/mozilla-mobile/rev/fa83a8e7b1d8a836bbf6a99e9243a52bddb9ab20/focus-android/app/src/main/java/org/mozilla/focus/fragment/BrowserFragment.kt#189).

(1) Note now, that `EngineViewPresenter` will call `renderTab` every time any of the [following properties change](https://searchfox.org/mozilla-mobile/rev/fa83a8e7b1d8a836bbf6a99e9243a52bddb9ab20/android-components/components/feature/session/src/main/java/mozilla/components/feature/session/engine/EngineViewPresenter.kt#40-43): the tab Id, the session associated to the tab, the "crashed" propery and the firstContentfulPaint property. `renderTab` will in turn [call `setSession`](https://searchfox.org/mozilla-mobile/rev/fa83a8e7b1d8a836bbf6a99e9243a52bddb9ab20/android-components/components/feature/session/src/main/java/mozilla/components/feature/session/engine/EngineViewPresenter.kt#89) on the `GeckoView` instance every time any of the above properties are updated, even if the property does not change which session is being displayed (e.g. in the case of the `firstContentfulPaint` event).

The bug is now possible with the following chain of events:

* User creates a custom tab, this creates two separate `ViewBoundFeatureWrapper` for the GeckoView in the custom tab
* User taps on the "Open in Focus" button, *while the page is loading*
* While migrating the tab to the main browser, Focus [calls `release`](https://searchfox.org/mozilla-mobile/rev/fa83a8e7b1d8a836bbf6a99e9243a52bddb9ab20/focus-android/app/src/main/java/org/mozilla/focus/fragment/BrowserFragment.kt#672) on one of the `ViewBoundFeatureWrapper` instances to `release` the Session from the `GeckoView` instance, but crucially, not on the other `ViewBoundFeatureWrapper` instance.
* While the tab is being migrated, a `FirstContenfulPaint` even fires. The first `ViewBoundFeatureWrapper` does not respond, as it was correctly stopped in the step above, but the other one does, calling `setSession` on the Custom Tab view as explained in (1).
* The tab finished migrating to the browser, which calls `setSession` again, this time on the main browser View, triggering the crash.

There are other combinations of events that trigger this bug in this area which I'm not going to go since they involve the same fix.

The two problems that I see here are:

* We create multiple `ViewBoundFeatureWrapper` for the same `GeckoView`. This is always going to end up badly, we should come up with a way to enforce that this never happens.
* We call `setSession` regardless of which event is causing `ifAnyChanged` to trigger in `EngineViewPresenter`. I think that's wrong and that could also cause problems, we should only do it if a relevant change in the `session` happened.

OK So updated current theory, what follows is my understanding (warning: things might be incorrect):

`ViewBoundFeatureWrapper` is used by AC to manage the lifetime of objects connected to a `View`. In particular, it's used to `start` and `stop` the `SessionFeature` which in turns will (indirectly) `setSession` and `release` the `GeckoView` instance associated to the feature.

By design, `GeckoView` can only have one session set at any given time, so it's crucial that only one `ViewBoundFeatureWrapper` "owns" a `GeckoView` at any time. Failure to do so, i.e. calling `setSession` with two different sessions or `release` twice in a row, will cause the crash in this bug.

Focus (and maybe Fenix too?), however, creates multiple `ViewBoundFeatureWrapper` instances for the same `GeckoView` when opening a custom tab, [here](https://searchfox.org/mozilla-mobile/rev/fa83a8e7b1d8a836bbf6a99e9243a52bddb9ab20/focus-android/app/src/main/java/org/mozilla/focus/activity/CustomTabActivity.kt#89) and [here](https://searchfox.org/mozilla-mobile/rev/fa83a8e7b1d8a836bbf6a99e9243a52bddb9ab20/focus-android/app/src/main/java/org/mozilla/focus/fragment/BrowserFragment.kt#189).

(1) Note now, that `EngineViewPresenter` will call `renderTab` every time any of the [following properties change](https://searchfox.org/mozilla-mobile/rev/fa83a8e7b1d8a836bbf6a99e9243a52bddb9ab20/android-components/components/feature/session/src/main/java/mozilla/components/feature/session/engine/EngineViewPresenter.kt#40-43): the tab Id, the session associated to the tab, the "crashed" propery and the firstContentfulPaint property. `renderTab` will in turn [call `setSession`](https://searchfox.org/mozilla-mobile/rev/fa83a8e7b1d8a836bbf6a99e9243a52bddb9ab20/android-components/components/feature/session/src/main/java/mozilla/components/feature/session/engine/EngineViewPresenter.kt#89) on the `GeckoView` instance every time any of the above properties are updated, even if the property does not change which session is being displayed (e.g. in the case of the `firstContentfulPaint` event).

The bug is now possible with the following chain of events:

* User creates a custom tab, this creates two separate `ViewBoundFeatureWrapper` for the GeckoView in the custom tab
* User taps on the "Open in Focus" button, *while the page is loading*
* While migrating the tab to the main browser, Focus [calls `release`](https://searchfox.org/mozilla-mobile/rev/fa83a8e7b1d8a836bbf6a99e9243a52bddb9ab20/focus-android/app/src/main/java/org/mozilla/focus/fragment/BrowserFragment.kt#672) on one of the `ViewBoundFeatureWrapper` instances to `release` the Session from the `GeckoView` instance, but crucially, not on the other `ViewBoundFeatureWrapper` instance.
* While the tab is being migrated, a `FirstContenfulPaint` even fires. The first `ViewBoundFeatureWrapper` does not respond, as it was correctly stopped in the step above, but the other one does, calling `setSession` on the Custom Tab view as explained in (1).
* The tab finished migrating to the browser, which calls `setSession` again, this time on the main browser View, triggering the crash.

There are other combinations of events that trigger this bug in this area which I'm not going to go into since they involve the same fix.

The two problems that I see here are:

* We create multiple `ViewBoundFeatureWrapper` for the same `GeckoView`. This is always going to end up badly, we should come up with a way to enforce that this never happens.
* We call `setSession` regardless of which event is causing `ifAnyChanged` to trigger in `EngineViewPresenter`. I think that's wrong and that could also cause problems, we should only do it if a relevant change in the `session` happened.