The default bug view has changed. See this FAQ.

Shutting down gonk sensors leads to freeze on main thread

RESOLVED FIXED in mozilla14

Status

()

Core
DOM: Device Interfaces
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: mwu, Assigned: slee)

Tracking

Trunk
mozilla14
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 4 obsolete attachments)

(Reporter)

Description

5 years ago
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
(Reporter)

Updated

5 years ago
Assignee: mwu → nobody
(Reporter)

Comment 1

5 years ago
Steven, do you want to look at this?
(Reporter)

Updated

5 years ago
Blocks: 734874
(Assignee)

Comment 2

5 years ago
Sure~ I will take it.
(Assignee)

Updated

5 years ago
Assignee: nobody → slee
(Assignee)

Comment 3

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

Review race!
Attachment #606149 - Flags: review?(mwu)
Attachment #606149 - Flags: review?(fabrice)

Comment 5

5 years ago
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

5 years ago
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

5 years ago
Please don't use tabs. Ever. Make sure you fix your editor.
(Reporter)

Comment 8

5 years ago
(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 .
(Reporter)

Comment 9

5 years ago
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().
Attachment #606149 - Flags: feedback-

Comment 10

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

Comment 11

5 years ago
Created attachment 606945 [details] [diff] [review]
patch
Attachment #606149 - Attachment is obsolete: true
Attachment #606149 - Flags: review?(mwu)
Attachment #606149 - Flags: review?(fabrice)

Comment 12

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

Comment 13

5 years ago
Also please watch out for trailing white space on lines.
(Assignee)

Updated

5 years ago
Attachment #606945 - Flags: review?(mwu)
(Reporter)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

5 years ago
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.
Attachment #607473 - Flags: review?(mwu)
(Assignee)

Updated

5 years ago
Attachment #606945 - Attachment is obsolete: true
Attachment #606945 - Flags: review?(mwu)
(Reporter)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Created attachment 608582 [details] [diff] [review]
Change the method of polling sensor data - V4
Attachment #607473 - Attachment is obsolete: true
Attachment #608582 - Flags: review?(mwu)
Attachment #607473 - Flags: review?(mwu)
(Reporter)

Comment 18

5 years ago
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.
Attachment #608582 - Flags: review?(mwu) → review+
(Assignee)

Comment 19

5 years ago
Created attachment 609199 [details] [diff] [review]
Changed the method of polling sensor data - V5
Attachment #608582 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Keywords: checkin-needed
http://hg.mozilla.org/integration/mozilla-inbound/rev/fa767f5ea5d7
Keywords: checkin-needed
Target Milestone: --- → mozilla14
https://hg.mozilla.org/mozilla-central/rev/fa767f5ea5d7
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 773792
Why do we have the SensorStatus::data field?  We never use it, as far as I can tell.
(Assignee)

Comment 23

5 years ago
Hi Justin,

I discussed with Marco this morning, we don't need this variables now.
You need to log in before you can comment on or make changes to this bug.