1780557 - Crash in [@ java.lang.SecurityException: at android.hardware.SystemSensorManager$BaseEventQueue.enableSensor(SystemSensorManager.java) ]

Status: RESOLVED FIXED

(GeckoView :: General, defect, P1)

Description

[Iorga Gabriel]

Attachment: Untitled.mp4

https://crash-stats.mozilla.org/report/index/2548a44e-d01f-47ae-a3f9-9fb530220721
App version 104
Phone: Pixel 3XL Android Version 12
It happens every time

Comment 1

[Mira Lobontiu (Android QA)]

I was able to reproduce this crash also, on Oppo Reno 6 (Android 12), on the Focus debug 104.0 (build 104.0a1-20220720095935, AC 104.0.20220720190100).

STR:

1. Load cnn.com.
2. Focus crashes.

Not reproducible on other websites.
Not reproducible on Samsung Galaxy Tab A6 (Android 5.1.1).

Comment 2

[Chris Peterson [:cpeterson]]

P3 because only 9 users have hit this crash.

Why is this a problem for Focus, but not Fenix? Does Fenix request the HIGH_SAMPLING_RATE_SENSORS permission, but Focus does not? Why does cnn.com need to a sensor sampling rate of 0 microseconds?? Maybe we should not allow any web content to request such a fast sampling rate.

java.lang.SecurityException: To use the sampling rate of 0 microseconds, app needs to declare the normal permission HIGH_SAMPLING_RATE_SENSORS.
	at android.hardware.SystemSensorManager$BaseEventQueue.enableSensor(SystemSensorManager.java:792)
	at android.hardware.SystemSensorManager$BaseEventQueue.addSensor(SystemSensorManager.java:712)
	at android.hardware.SystemSensorManager.registerListenerImpl(SystemSensorManager.java:209)
	at android.hardware.SensorManager.registerListener(SensorManager.java:832)
	at android.hardware.SensorManager.registerListener(SensorManager.java:739)
	at org.mozilla.gecko.GeckoAppShell.enableSensor(GeckoAppShell.java:636)
	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:677)

Comment 3

[Iorga Gabriel]

It's the same Crash also on Fenix . I reproduced it .

Comment 4

[Iorga Gabriel]

Attachment: Fenix Crash .mp4

Comment 5

[Chris Peterson [:cpeterson]]

(In reply to Iorga Gabriel from comment #3)

It's the same Crash also on Fenix . I reproduced it .

Interesting. In that case, I'll send this bug to GeckoView triage for prioritization.

What device were you testing? I tried loading cnn.com and edition.cnn.com on my Samsung Galaxy A51 in Focus or Fenix, but couldn't reproduce the crash. Maybe it only happens on certain devices, though we have crash reports from some pretty popular phones, such as Google Pixel 6.

Comment 6

[Chris Peterson [:cpeterson]]

Seems like we just need to add HIGH_SAMPLING_RATE_SENSORS to GV's AndroidManifest.xml.

The page is trying to access the Accelerometer sensor. The page doesn't specify the 0 microsecond delay; that comes from us passing [SENSOR_DELAY_FASTEST](https://developer.android.com/reference/android/hardware/SensorManager#SENSOR_DELAY_FASTEST) ("get sensor data as fast as possible") here:

https://searchfox.org/mozilla-central/rev/1061fae5e225a99ef5e43dbdf560a91a0c0d00d1/mobile/android/geckoview/src/main/java/org/mozilla/gecko/GeckoAppShell.java#637

The Android 12 compatibility guide says:

For apps targeting Android 12 (API level 31) and higher, a SecurityException is thrown when they do not have HIGH_SAMPLING_RATE_SENSORS permission, run in debug mode, and request sampling rates that are faster than 200 Hz.

Are these crashing users running in debug mode??

The Android Sensors Overview says:

If your app needs to gather motion sensor data at a higher rate (than RATE_NORMAL), you must declare the HIGH_SAMPLING_RATE_SENSORS permission, as shown in the following code snippet. Otherwise, if your app tries to gather motion sensor data at a higher rate without declaring this permission, a SecurityException occurs.

<manifest ...>
    <uses-permission android:name="android.permission.HIGH_SAMPLING_RATE_SENSORS"/>
    <application ...>
        ...
    </application>
</manifest>

Comment 7

[Iorga Gabriel]

I tested with Pixel 3XL Android Version 12 in debug mode and in release mode and the crash is happening in both ways .

Comment 8

[Chris Peterson [:cpeterson]]

Adding to our Q3 backlog. The fix looks easy and will fix a crash.

Comment 9

[Olivia Hall [:olivia]]

Saw this crash today in a development GeckoView Example and a development Fenix with same GVE when visiting kroger.com, mcdonalds.com, and cnn.com on a Pixel 4a.

08-10 10:43:28.509 10671 10702 W System.err: java.lang.SecurityException: To use the sampling rate of 0 microseconds, app needs to declare the normal permission HIGH_SAMPLING_RATE_SENSORS.
08-10 10:43:28.509 10671 10702 W System.err: 	at android.hardware.SystemSensorManager$BaseEventQueue.enableSensor(SystemSensorManager.java:792)
08-10 10:43:28.510 10671 10702 W System.err: 	at android.hardware.SystemSensorManager$BaseEventQueue.addSensor(SystemSensorManager.java:712)
08-10 10:43:28.510 10671 10702 W System.err: 	at android.hardware.SystemSensorManager.registerListenerImpl(SystemSensorManager.java:216)
08-10 10:43:28.510 10671 10702 W System.err: 	at android.hardware.SensorManager.registerListener(SensorManager.java:832)
08-10 10:43:28.511 10671 10702 W System.err: 	at android.hardware.SensorManager.registerListener(SensorManager.java:739)
08-10 10:43:28.511 10671 10702 W System.err: 	at org.mozilla.gecko.GeckoAppShell.enableSensor(GeckoAppShell.java:648)
08-10 10:43:28.512 10671 10702 W System.err: 	at org.mozilla.gecko.mozglue.GeckoLoader.nativeRun(Native Method)
08-10 10:43:28.512 10671 10702 W System.err: 	at org.mozilla.gecko.GeckoThread.run(GeckoThread.java:677)

Comment 10

[Chris Peterson [:cpeterson]]

(In reply to Olivia Hall from comment #9)

Saw this crash today in a development GeckoView Example and a development Fenix with same GVE when visiting kroger.com, mcdonalds.com, and cnn.com on a Pixel 4a.

In that case, I'll tag this bug for our 106 sprint.

Comment 11

[Jonathan Almeida [:jonalmeida]]

We're seeing this crash on Focus quite easily right now.

See the pull request comment and the ones below.

Comment 12

[Olivia Hall [:olivia]]

Attachment: Bug 1780557 - High Rate Sensor Android Permission

Beginning in Android 12, apps needing to use high sensor rates need to
have the HIGH_SAMPLING_RATE_SENSORS permission requested.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbd904585a76
High Rate Sensor Android Permission r=geckoview-reviewers,calu

Comment 14

[Marian-Vasile Laza]

https://hg.mozilla.org/mozilla-central/rev/dbd904585a76

Comment 15

[Mihai Adrian Carare [:mcarare]]

My question for this issue and the fix linked is if we really need the SENSOR_DELAY_FASTEST value for the listener.

First of all, the adding of the HIGH_SAMPLING_RATE_SENSORS is discouraged, a message appears when added:

Most apps don't need access to high sensor sampling rate.

Second, the documentation also mentions setting a 0 delay ( using SENSOR_DELAY_FASTEST ) drains the battery faster. I think that rather than just adding this permission in Fenix/ Focus, we should assess if a higher delay is still suited for our purposes.

As an alternative, we have SENSOR_DELAY_GAME with a delay of only 0.02 seconds or SENSOR_DELAY_UI with 0.06 seconds delay. The documentation (and name ) for these states they are suitable for games/user interface.

Also note that if we only need sensor data for screen orientation changes, a SENSOR_DELAY_NORMAL (0.2 seconds) is documented to be suitable.

Comment 16

[Olivia Hall [:olivia]]

Thanks for taking a look at this!

My question for this issue and the fix linked is if we really need the SENSOR_DELAY_FASTEST value for the listener.

I agree, this was one of my concerns too. I think changing to SensorManager.SENSOR_DELAY_NORMAL would be ideal.

One concern I had with changing to a lower sampling rate is potential unintended consequences to the Sensor API and Gecko's use of the sensors. Making this change would require downgrading the sampling rate of six sensors. I'm not quite sure how impactful the sampling rate could actually be to those APIs and erred on the side of caution. I think it could potentially cause problems, but be subtle enough to go undetected for a while.

As an alternative, we have SENSOR_DELAY_GAME with a delay of only 0.02 seconds or SENSOR_DELAY_UI with 0.06 seconds delay. The documentation (and name ) for these states they are suitable for games/user interface.

This seems like a good middle of the road option to me - this might be a way to avoid unintended consequences of going to SENSOR_DELAY_NORMAL and also avoid adding a permission. Also, it looks like Chrome is using the GAME sampling rate for orientation - not quite sure why they picked a faster rate, but it raises some questions. (Other rate Chrome uses too, brought up by :jonalmeida.)

Please let me know what you think. I could add a follow-up patch or make this a new bug for a longer investigation. I think the biggest decision is weighing the risk/reward of potentially impacting existing APIs in subtle ways versus bringing in a new permission for existing behavior for Android 12.

I also think resolving this core issue reasonably quickly (but prudently) is important too, because when I experienced this bug, it was a complete application crash.

Comment 17

[Mihai Adrian Carare [:mcarare]]

I'm not entirely sure how the delay increase would affect things. I haven't seen where in code we react to those updates (maybe it's at a lower level) so I only intuitively guess that if other apps can work with that we could do that too.

I am all for solving the crash, so I could the permissions for Focus/ Fenix. As long as we keep investigating an appropriate balance between having stream-like updates and resource usage.

Comment 18

[Olivia Hall [:olivia]]

An edge case example might be a website making a game and now the game play isn't fluid/usable because of the recently throttled sample rate. However, I think this is an extreme example and most uses are going to be websites using it to make slight tweaks to the orientation and layout of their page - so the difference in responsiveness between SENSOR_DELAY_FASTEST and SENSOR_DELAY_NORMAL won't be impactful.

I think moving to a lower rate is the right way to go. However, there is also a feeling of an overabundance of caution for side-effects. Maybe a companion bug for more research? Or, I also think moving to the GAME rate could be a good solution.

Comment 19

[Chris Peterson [:cpeterson]]

Olivia, so you are suggesting we remove the HIGH_SAMPLING_RATE_SENSORS permission and instead use a slower sensor rate? Should we reopen this bug or file a new bug for those changes?

Comment 20

[Olivia Hall [:olivia]]

(In reply to Chris Peterson [:cpeterson] from comment #19)

Olivia, so you are suggesting we remove the HIGH_SAMPLING_RATE_SENSORS permission and instead use a slower sensor rate? Should we reopen this bug or file a new bug for those changes?

I'm in favor of reopening the bug and slowing the sensor rate to the SENSOR_DELAY_GAME rate and removing the permission. I think adding only the permission is the more conservative solution, but using the GAME rate likely will mitigate any potential side-effects of slowing the rate.

I'm not sure any additional information would come out of researching this further on a new bug because the side-effects on changing the rate would be mostly around the use of the Sensor API by websites. One follow-up bug might be if it is possible to slow the rate even further to the SENSOR_DELAY_NORMAL for battery-life performance or if that does impact how things like JS games work too much.

Comment 21

[Jonathan Almeida [:jonalmeida]]

(In reply to Mihai Adrian Carare from comment #17)

I am all for solving the crash, so I could the permissions for Focus/ Fenix.

The fix that landed in Olivia's patch landed adds the permission to the GV manifest, so when Focus/Fenix are built the manifest merger should include this permission as well without the apps needing to add the permission. Maybe I have misunderstood something?

(In reply to Olivia Hall from comment #20)

One follow-up bug might be if it is possible to slow the rate even further to the SENSOR_DELAY_NORMAL for battery-life performance or if that does impact how things like JS games work too much.

I agree - this sounds like a good follow up for now.

Comment 22

[Mihai Adrian Carare [:mcarare]]

(In reply to Jonathan Almeida (:jonalmeida) from comment #21)

The fix that landed in Olivia's patch landed adds the permission to the GV manifest, so when Focus/Fenix are built the manifest merger should include this permission as well without the apps needing to add the permission. Maybe I have misunderstood something?

No, I haven't taken into account that patch. It should avoid crashing on its own, without needing to add it explicitly in Fenix / Focus manifest.
(it should be listed in the merged manifest)

Comment 23

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

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

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

For more information, please visit auto_nag documentation.

Comment 24

[Olivia Hall [:olivia]]

I'm in favor of reopening the bug and slowing the sensor rate to the SENSOR_DELAY_GAME rate and removing the permission. I think adding only the permission is the more conservative solution, but using the GAME rate likely will mitigate any potential side-effects of slowing the rate.

If the consensus if that adding the permission isn't an issue, then I think it is okay to leave the bug as-is too. I think either the current permission solution or changing to the GAME rate approach works, especially if we follow-up later with a bug for researching SENSOR_DELAY_NORMAL.

Comment 25

[Chris Peterson [:cpeterson]]

Setting status-firefox105 = wontfix. The crash rate is so low, so I don't think we need to uplift this fix to Beta 105. I don't know if uplifting new permissions is risky.