Last Comment Bug 734869 - Shutting down gonk sensors leads to freeze on main thread
: Shutting down gonk sensors leads to freeze on main thread
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM: Device Interfaces (show other bugs)
: Trunk
: All Gonk (Firefox OS)
: -- normal (vote)
: mozilla14
Assigned To: StevenLee[:slee]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 714413 773792
Blocks: 734874
  Show dependency treegraph
 
Reported: 2012-03-12 07:24 PDT by Michael Wu [:mwu]
Modified: 2012-09-02 21:11 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Change the method of polling sensor data (4.58 KB, patch)
2012-03-15 02:46 PDT, StevenLee[:slee]
mwu.code: feedback-
Details | Diff | Splinter Review
patch (5.57 KB, patch)
2012-03-17 23:35 PDT, Andreas Gal :gal
no flags Details | Diff | Splinter Review
Change the method of polling sensor data - V3 (6.40 KB, patch)
2012-03-20 00:36 PDT, StevenLee[:slee]
no flags Details | Diff | Splinter Review
Change the method of polling sensor data - V4 (6.40 KB, patch)
2012-03-22 19:55 PDT, StevenLee[:slee]
mwu.code: review+
Details | Diff | Splinter Review
Changed the method of polling sensor data - V5 (6.79 KB, patch)
2012-03-25 20:31 PDT, StevenLee[:slee]
no flags Details | Diff | Splinter Review

Description Michael Wu [:mwu] 2012-03-12 07:24:30 PDT
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] 2012-03-12 07:28:55 PDT
Steven, do you want to look at this?
Comment 2 StevenLee[:slee] 2012-03-12 19:42:43 PDT
Sure~ I will take it.
Comment 3 StevenLee[:slee] 2012-03-15 02:46:04 PDT
Created attachment 606149 [details] [diff] [review]
Change the method of polling sensor data
Comment 4 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-03-15 11:43:44 PDT
Comment on attachment 606149 [details] [diff] [review]
Change the method of polling sensor data

Review race!
Comment 5 Mozilla RelEng Bot 2012-03-15 12:03:45 PDT
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/slee@mozilla.com-6c2de9f7ff42
 Timed out after 12 hours without completing.
Comment 6 Andreas Gal :gal 2012-03-17 23:03:35 PDT
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 2012-03-17 23:04:46 PDT
Please don't use tabs. Ever. Make sure you fix your editor.
Comment 8 Michael Wu [:mwu] 2012-03-17 23:12:32 PDT
(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] 2012-03-17 23:16:29 PDT
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 2012-03-17 23:28:04 PDT
I will whip up a new patch. There are a ton of race conditions in the code, too.
Comment 11 Andreas Gal :gal 2012-03-17 23:35:38 PDT
Created attachment 606945 [details] [diff] [review]
patch
Comment 12 Andreas Gal :gal 2012-03-17 23:36:20 PDT
Steven, please take this patch and test it and get reviews for it. Thanks.
Comment 13 Andreas Gal :gal 2012-03-17 23:37:20 PDT
Also please watch out for trailing white space on lines.
Comment 14 Michael Wu [:mwu] 2012-03-19 18:53:10 PDT
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] 2012-03-20 00:36:58 PDT
Created attachment 607473 [details] [diff] [review]
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] 2012-03-20 16:30:53 PDT
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] 2012-03-22 19:55:51 PDT
Created attachment 608582 [details] [diff] [review]
Change the method of polling sensor data - V4
Comment 18 Michael Wu [:mwu] 2012-03-25 11:05:07 PDT
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] 2012-03-25 20:31:58 PDT
Created attachment 609199 [details] [diff] [review]
Changed the method of polling sensor data - V5
Comment 21 Matt Brubeck (:mbrubeck) 2012-03-26 11:29:05 PDT
https://hg.mozilla.org/mozilla-central/rev/fa767f5ea5d7
Comment 22 Justin Lebar (not reading bugmail) 2012-08-31 02:05:24 PDT
Why do we have the SensorStatus::data field?  We never use it, as far as I can tell.
Comment 23 StevenLee[:slee] 2012-09-02 21:11:25 PDT
Hi Justin,

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

Note You need to log in before you can comment on or make changes to this bug.