1801526 - Crash in [@ java.lang.IllegalArgumentException: at android.os.Parcel.createExceptionOrNull(Parcel.java)] "Already have a GeckoSurfaceTexture with that handle"

Status: RESOLVED FIXED

(GeckoView :: Core, defect, P2)

Description

[Chris Peterson [:cpeterson]]

Crash report: https://crash-stats.mozilla.org/report/index/099366d5-efd5-425a-b64e-1502d0221117

Java stack trace:

java.lang.IllegalArgumentException: Already have a GeckoSurfaceTexture with that handle
	at android.os.Parcel.createExceptionOrNull(Parcel.java:2444)
	at android.os.Parcel.createException(Parcel.java:2424)
	at android.os.Parcel.readException(Parcel.java:2407)
	at android.os.Parcel.readException(Parcel.java:2349)
	at org.mozilla.gecko.gfx.ISurfaceAllocator$Stub$Proxy.acquireSurface(ISurfaceAllocator.java:40)
	at org.mozilla.gecko.gfx.SurfaceAllocator.acquireSurface(SurfaceAllocator.java:33)

Comment 1

[Chris Peterson [:cpeterson]]

Jamie, could this crash be a regression from SurfaceTexture bug 1777952 in Fenix 104?

This is an old exception, but the crash volume spiked from less than 20 crash reports in Fenix <= 103 to about 1200 in Fenix 104. The first crash report in Nightly 104.0a1 was build ID 20220708093332 and SurfaceTexture bug 1777952 landed just the day before (2022-07-07).

java.lang.IllegalArgumentException: Already have a GeckoSurfaceTexture with that handle

https://searchfox.org/mozilla-central/rev/3d01e045479a6e5237958bd9aa8eb64306e1f48a/mobile/android/geckoview/src/main/java/org/mozilla/gecko/gfx/GeckoSurfaceTexture.java#234-248

Comment 2

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1777952

Comment 3

[Jamie Nicol [:jnicol]]

Yes, this is certainly a regression from bug 1777952. That bug was itself fixing a situation where we ended up with duplicated SurfaceTexture handles.

As part of that fix, we moved the code which calculated the handle value out of GeckoSurfaceTexture.acquire() and in to the caller RemoteSurfaceAllocator.acquireSurface(). Here we use the static volatile int sNextCounter as a monotonically increasing counter to calculate the handle value from.

That was fine when that code resided in a synchronized block in GeckoSurfaceTexture.acquire(). But bug 1777952 moved it out of that synchronized block, and I'm guessing what's now happening is a race condition between multiple threads reading the value and incrementing it.

We should replace it with an AtomicInteger.

Comment 4

[Jamie Nicol [:jnicol]]

Attachment: Bug 1801526 - Use AtomicInteger for GeckoSurfaceTexture handle counter. r?#geckoview-reviewers

Currently we use a static volatile int as a monotonically
incrementing counter from which we calculate handles for
GeckoSurfaceTextures. During a previous patch, the code which reads
and increments this counter was moved out of a synchronized block,
meaning it can potentially race. We are now seeing crashes due to
attempting to allocate different GeckoSurfaceTextures with the same
handle, and this could be the cause.

To fix this, we replace the counter with an AtomicInt, which has an
atomic getAndIncrement() function.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4feaa76d2f0d
Use AtomicInteger for GeckoSurfaceTexture handle counter. r=geckoview-reviewers,calu

Comment 6

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/4feaa76d2f0d

Comment 7

[BugBot [:suhaib / :marco/ :calixte]]

The patch landed in nightly and beta is affected.
:jnicol, is this bug important enough to require an uplift?

• If yes, please nominate the patch for beta approval.
• If no, please set status-firefox108 to wontfix.

For more information, please visit auto_nag documentation.

Comment 8

[Jamie Nicol [:jnicol]]

Comment on attachment 9304438 [details]
Bug 1801526 - Use AtomicInteger for GeckoSurfaceTexture handle counter. r?#geckoview-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Occasional content process crashes when allocating Surface for video and webgl
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very small change which makes a counter atomic
• String changes made/needed:
• Is Android affected?: Yes

Comment 9

[Donal Meehan [:dmeehan]]

Please consider also adding a release uplift request, we could include it in the 107 planned dot release

Comment 10

[Dianna Smith [:diannaS]]

Comment on attachment 9304438 [details]
Bug 1801526 - Use AtomicInteger for GeckoSurfaceTexture handle counter. r?#geckoview-reviewers

Approved for 108.0b6

Comment 11

[Jamie Nicol [:jnicol]]

Okay, I'd like to give it a few days to ensure there aren't any problems first, and will request uplift then

Comment 12

[Dianna Smith [:diannaS]]

https://hg.mozilla.org/releases/mozilla-beta/rev/a4e3302a1742

Comment 13

[Jamie Nicol [:jnicol]]

Looking good on nightly and beta, I will request release uplift now.

Comment 14

[Jamie Nicol [:jnicol]]

Comment on attachment 9304438 [details]
Bug 1801526 - Use AtomicInteger for GeckoSurfaceTexture handle counter. r?#geckoview-reviewers

Beta/Release Uplift Approval Request

• User impact if declined: Occasional content process crashes when allocating Surface for video and webgl on Android
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Very small change which makes a counter atomic
• String changes made/needed:
• Is Android affected?: Yes

Comment 15

[Donal Meehan [:dmeehan]]

Comment on attachment 9304438 [details]
Bug 1801526 - Use AtomicInteger for GeckoSurfaceTexture handle counter. r?#geckoview-reviewers

Approved for 107.0.1

Comment 16

[Donal Meehan [:dmeehan]]

https://hg.mozilla.org/releases/mozilla-release/rev/f78e2d50dc36