Closed Bug 1647883 Opened 2 months ago Closed 1 month ago

GeckoSession object identity is a pocket full of lies

Categories

(GeckoView :: General, defect, P1)

Unspecified
Android

Tracking

(firefox77 wontfix, firefox78 wontfix, firefox79 wontfix, firefox80 fixed)

RESOLVED FIXED
mozilla80
Tracking Status
firefox77 --- wontfix
firefox78 --- wontfix
firefox79 --- wontfix
firefox80 --- fixed

People

(Reporter: aklotz, Assigned: aklotz)

References

(Blocks 1 open bug, Regression)

Details

(Keywords: regression, Whiteboard: [geckoview:m80])

Attachments

(2 files)

In bug 1490493 we modified GeckoSession's overrides of Object.hashCode and Object.equals to match on GeckoSession.mId.

This might be useful for knowing whether two GeckoSessions refer to the same underlying Gecko constructs, however these overrides are bad for Java object identity: two distinct objects claim to be equal when their mIds match, regardless of other state encapsulated by those objects.

This causes weird things to happen, for example GeckoSessionTestRule.mSubSessions only contains a single entry when multiple successive GeckoSession objects were created.

This is easily reproduced by running SessionLifecycleTest#readFromParcel_chained and watching the logcat for content process creation and destruction. This test essentially leaks a session permanently, and AFAICT the leak appears to be a result of these overrides.

I think that we definitely need hashCode and equals to take GeckoSession state into account; we'll need to find an alternative to solve bug 1490493's original problem.

(I realize that removing the GeckoSession / Parcel stuff will probably help here, but that does not help much with existing deployments)

Indeed, changing hashCode and equals to just use mWindow, mSettings, and mId was enough to fix the leak in that test. Unfortunately this creates other failures because of code that was expecting this weird object identity.

I'm investigating further to figure out what we need here...

Whiteboard: [geckoview:m80]
Priority: -- → P1

It's fairly easy to just revert the overridden hashCode and equals outside of tests, however our tests fail because now we need to explicitly manage our association between GeckoSession and its WebExtension.Port when a session is reconstituted from a Parcel.

Because GeckoSession's overrides of hashCode and equals look solely at session ID, this may cause
strange behaviors if a GeckoSession is reloaded with session state from a previous instance, and the
previous instance still exists. For example, suppose the previous instance is closed and the new instance is
open. As far as the Android runtime is concerned, both objects are equivalent. Trying to insert both objects
into the same container will not work as expected.

In this patch, we revert those overrides. To ensure that we still have a short-circuit path in
GeckoView.restoreSession, we add and utilize a new, package-scoped, equalsId method.

The only fallout from part 1 is a single test where we run evaluateJS on a session, serialize it to a Parcel,
deserialize it to new, distinct GeckoSession object, and then perform additional evaluateJS on the latter.

Since the deserialization of the Parcel transferred the session's contents from the original session to the new
session, we also need to transfer the WebExtension.Port used by evaluateJS to the new session.

Note that this fix is kind of hacky, but because we are going to deprecate the parcelability of GeckoSession, I
do not believe that it is worth the effort to implement a "perfect" fix; that would likely involve adding new APIs
to support another API that we are going to deprecate anyway.

Depends on D81746

Pushed by aklotz@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d98ba38985bb
Part 1 - Remove hashCode and equals overrides from GeckoSession in favour of new, package-scoped GeckoSession.equalsId method; r=geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/a3cc583cc2ba
Part 2 - Update junit tests to transfer a WebExtension.Port between sessions; r=geckoview-reviewers,agi
Backout by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ad7541217f25
Backed out 2 changesets for gecko decision task issues. CLOSED TREE
Pushed by csabou@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/13673a711ea1
Part 1 - Remove hashCode and equals overrides from GeckoSession in favour of new, package-scoped GeckoSession.equalsId method; r=geckoview-reviewers,agi
https://hg.mozilla.org/integration/autoland/rev/f10679133987
Part 2 - Update junit tests to transfer a WebExtension.Port between sessions; r=geckoview-reviewers,agi
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla80
You need to log in before you can comment on or make changes to this bug.