GeckoSession object identity is a pocket full of lies
Categories
(GeckoView :: General, defect, P1)
Tracking
(firefox77 wontfix, firefox78 wontfix, firefox79 wontfix, firefox80 fixed)
People
(Reporter: bugzilla, Assigned: bugzilla)
References
(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.
|
Assignee | |
Comment 1•4 years ago
|
||
(I realize that removing the GeckoSession / Parcel stuff will probably help here, but that does not help much with existing deployments)
|
|
Assignee | |
Comment 2•4 years ago
|
||
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...
|
|
Assignee | |
Updated•4 years ago
|
|
|
Assignee | |
Updated•4 years ago
|
|
|
Assignee | |
Comment 3•4 years ago
|
||
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.
|
|
Assignee | |
Comment 4•4 years ago
|
||
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.
|
|
Assignee | |
Comment 5•4 years ago
|
||
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
|
||
Comment 9•4 years ago
|
||
| bugherder | ||
https://hg.mozilla.org/mozilla-central/rev/13673a711ea1
https://hg.mozilla.org/mozilla-central/rev/f10679133987
|
||
Updated•4 years ago
|
|
||
Updated•4 years ago
|
Description
•