734869 - Shutting down gonk sensors leads to freeze on main thread

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Michael Wu [:mwu]]

1. all sensors are turned off
2. poll on the sensor thread gets stuck because there are no sensor events.
3. we attempt to stop and join the sensor thread on the main thread.
4. freeze on main thread

Comment 1

[Michael Wu [:mwu]]

Steven, do you want to look at this?

Comment 2

[StevenLee[:slee]]

Sure~ I will take it.

Comment 3

[StevenLee[:slee]]

Attachment: Change the method of polling sensor data

Comment 4

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 606149 [details] [diff] [review]
Change the method of polling sensor data

Review race!

Comment 5

[Mozilla RelEng Bot]

Try run for 6c2de9f7ff42 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=6c2de9f7ff42
Results (out of 162 total builds):
    exception: 2
    success: 126
    warnings: 30
    other: 4
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-6c2de9f7ff42
 Timed out after 12 hours without completing.

Comment 6

[Andreas Gal :gal]

The current code is broken for other reasons, too. It seems sensors are not disabled by default so we are heavy duty polling sensors to death while idle (at least with the Nexus S).

Comment 7

[Andreas Gal :gal]

Please don't use tabs. Ever. Make sure you fix your editor.

Comment 8

[Michael Wu [:mwu]]

(In reply to Andreas Gal :gal from comment #6)
> The current code is broken for other reasons, too. It seems sensors are not
> disabled by default so we are heavy duty polling sensors to death while idle
> (at least with the Nexus S).

That's the orientation rotation code introduced by bug 709590 .

Comment 9

[Michael Wu [:mwu]]

Comment on attachment 606149 [details] [diff] [review]
Change the method of polling sensor data

f- for the moment since I haven't had the time to do a real review. However, as far as I can tell, the basic problem here hasn't been fixed and we can still disable the sensors while still stuck on device.poll().

Comment 10

[Andreas Gal :gal]

I will whip up a new patch. There are a ton of race conditions in the code, too.

Comment 11

[Andreas Gal :gal]

Attachment: patch

Comment 12

[Andreas Gal :gal]

Steven, please take this patch and test it and get reviews for it. Thanks.

Comment 13

[Andreas Gal :gal]

Also please watch out for trailing white space on lines.

Comment 14

[Michael Wu [:mwu]]

Comment on attachment 606945 [details] [diff] [review]
patch

Review of attachment 606945 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/gonk/GonkSensor.cpp
@@ +170,3 @@
>  {
> +  if (!sActivatedSensors) {
> +    NS_NewThread(getter_AddRefs(sSwitchThread));

Hm this doesn't look right to me. We can get two consecutive EnableSensorNotifications call, which would generate an extra thread unnecessarily because sActivatedSensors doesn't get a chance to increment. It should simply generate a thread when there isn't one available.

Comment 15

[StevenLee[:slee]]

Attachment: Change the method of polling sensor data - V3

1. Fix the problem that it may generate extra threads.
2. Fix the problem that the status will be incorrect when apps enable sensor-A but disable sensor-B.

Comment 16

[Michael Wu [:mwu]]

Comment on attachment 607473 [details] [diff] [review]
Change the method of polling sensor data - V3

Review of attachment 607473 [details] [diff] [review]:
-----------------------------------------------------------------

::: hal/gonk/GonkSensor.cpp
@@ +76,5 @@
>  }
>  
>  namespace hal_impl {
>  
> +typedef struct SensorStatus {

Fortunately this is C++ not C and a typedef should not be necessary. (if it is, use a class)

@@ +135,5 @@
>  
>      void Switch() {
> +      int index = HardwareSensorToHalSensor(mSensor.type);
> +
> +      if (sSensorStatus[index].count == 0 && !mActivate) {

Hm, so this would protect against deactivating more than we've activated, right?

I suspect that would be a bug in the caller and we should MOZ_ASSERT when this happens. count wouldn't have to be declared for non-debug builds.

Comment 17

[StevenLee[:slee]]

Attachment: Change the method of polling sensor data - V4

Comment 18

[Michael Wu [:mwu]]

Comment on attachment 608582 [details] [diff] [review]
Change the method of polling sensor data - V4

Review of attachment 608582 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the assertion fixed.

::: hal/gonk/GonkSensor.cpp
@@ +136,5 @@
>  
>      void Switch() {
> +      int index = HardwareSensorToHalSensor(mSensor.type);
> +
> +      MOZ_ASSERT(sSensorStatus[index].count == 0 && mActivate);

This assertion will always fail if we're turning off a sensor. I think you want MOZ_ASSERT(sSensorStatus[index].count || mActivate) here.

Comment 19

[StevenLee[:slee]]

Attachment: Changed the method of polling sensor data - V5

Comment 20

[Dão Gottwald [:dao]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/fa767f5ea5d7

Comment 21

[Matt Brubeck (:mbrubeck)]

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

Comment 22

[Justin Lebar (not reading bugmail)]

Why do we have the SensorStatus::data field?  We never use it, as far as I can tell.

Comment 23

[StevenLee[:slee]]

Hi Justin,

I discussed with Marco this morning, we don't need this variables now.