Closed
Bug 734869
Opened 12 years ago
Closed 12 years ago
Shutting down gonk sensors leads to freeze on main thread
Categories
(Core :: DOM: Device Interfaces, defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: mwu, Assigned: slee)
References
Details
Attachments
(1 file, 4 obsolete files)
6.79 KB,
patch
|
Details | Diff | Splinter Review |
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•12 years ago
|
Assignee: mwu → nobody
Reporter | ||
Comment 1•12 years ago
|
||
Steven, do you want to look at this?
Assignee | ||
Comment 2•12 years ago
|
||
Sure~ I will take it.
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → slee
Assignee | ||
Comment 3•12 years ago
|
||
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•12 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•12 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•12 years ago
|
||
Please don't use tabs. Ever. Make sure you fix your editor.
Reporter | ||
Comment 8•12 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•12 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•12 years ago
|
||
I will whip up a new patch. There are a ton of race conditions in the code, too.
Comment 11•12 years ago
|
||
Attachment #606149 -
Attachment is obsolete: true
Attachment #606149 -
Flags: review?(mwu)
Attachment #606149 -
Flags: review?(fabrice)
Comment 12•12 years ago
|
||
Steven, please take this patch and test it and get reviews for it. Thanks.
Comment 13•12 years ago
|
||
Also please watch out for trailing white space on lines.
Assignee | ||
Updated•12 years ago
|
Attachment #606945 -
Flags: review?(mwu)
Reporter | ||
Comment 14•12 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•12 years ago
|
||
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•12 years ago
|
Attachment #606945 -
Attachment is obsolete: true
Attachment #606945 -
Flags: review?(mwu)
Reporter | ||
Comment 16•12 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•12 years ago
|
||
Attachment #607473 -
Attachment is obsolete: true
Attachment #608582 -
Flags: review?(mwu)
Attachment #607473 -
Flags: review?(mwu)
Reporter | ||
Comment 18•12 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•12 years ago
|
||
Attachment #608582 -
Attachment is obsolete: true
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 20•12 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/fa767f5ea5d7
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Comment 21•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fa767f5ea5d7
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Comment 22•12 years ago
|
||
Why do we have the SensorStatus::data field? We never use it, as far as I can tell.
Assignee | ||
Comment 23•12 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.
Description
•