Open Bug 1274731 Opened 8 years ago Updated 2 years ago

On linux, Firefox should speak with dbus asynchronously for the retrieving of battery status.

Categories

(Core :: DOM: Device Interfaces, defect, P3)

Product:
Core
Component:
DOM: Device Interfaces
Version:
46 Branch
Type:
defect

Tracking

()

People

(Reporter: simon__b, Unassigned)

References

(Blocks 1 open bug)

Details

(Keywords: hang, Whiteboard: btpp-followup-2016-06-06)

Attachments

(2 files, 3 obsolete files)

part 1 - getBattery() promise managed by BatteryManager
3.65 KB, patch
smaug
: review+
Details | Diff | Splinter Review
part 2 - Introduce RetrieveBatteryInformation()
44.08 KB, patch
karlt
: review-
Details | Diff | Splinter Review
User Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64; rv:46.0) Gecko/20100101 Firefox/46.0 Build ID: 20160512103502 Steps to reproduce: I went to reddit and clicked on this link: http://i.imgur.com/5fOhd02 Firefox also crashes if I go to that link in Safe Mode. firefox-46.0.1-4.fc23.x86_64 I produced a crash dump using abrt with killall -ILL firefox Actual results: Firefox hangs. Expected results: Firefox should not hang.
File too large (100 megs), I've uploaded it here, it will be hosted for 48 hours: http://expirebox.com/download/6adfda6b63f3d2e6404d2e9ca9577acb.html
Not with a safe profile, no. Just in safe mode. I will try a fresh profile next.
Same problem. The firefox using safe mode with a fresh profile hangs.
My Linux VM doesn't like the core dump format, but the salient bit of the core_backtrace is: , "frames": [ { "address": 140363356043465 , "build_id": "ffa3110fb53abcd28099f67ef4a5846cc1aec1cc" , "build_id_offset": 67785 , "function_name": "raise" , "file_name": "/lib64/libpthread.so.0" } , { "address": 140363103216853 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 36201685 , "function_name": "nsProfileLock::FatalSignalHandler(int, siginfo_t*, void*)" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363356043760 , "build_id": "ffa3110fb53abcd28099f67ef4a5846cc1aec1cc" , "build_id_offset": 68080 , "file_name": "/lib64/libpthread.so.0" } , { "address": 140363341912861 , "build_id": "7e84dede40ed53f99c2d0a0457ea0dd46a35bee8" , "build_id_offset": 1010461 , "function_name": "poll" , "file_name": "/lib64/libc.so.6" } , { "address": 140363218853167 , "build_id": "d63c2d889c0049a627dc24977db035eb0ef32550" , "build_id_offset": 183599 , "function_name": "socket_do_iteration" , "file_name": "/lib64/libdbus-1.so.3" } , { "address": 140363218848158 , "build_id": "d63c2d889c0049a627dc24977db035eb0ef32550" , "build_id_offset": 178590 , "function_name": "_dbus_transport_do_iteration" , "file_name": "/lib64/libdbus-1.so.3" } , { "address": 140363218753028 , "build_id": "d63c2d889c0049a627dc24977db035eb0ef32550" , "build_id_offset": 83460 , "function_name": "_dbus_connection_do_iteration_unlocked" , "file_name": "/lib64/libdbus-1.so.3" } , { "address": 140363218755772 , "build_id": "d63c2d889c0049a627dc24977db035eb0ef32550" , "build_id_offset": 86204 , "function_name": "_dbus_connection_block_pending_call" , "file_name": "/lib64/libdbus-1.so.3" } , { "address": 140363036917575 , "build_id": "af67846753967b5f6f87726d7afd0a441969aa3e" , "build_id_offset": 73543 , "function_name": "dbus_g_proxy_end_call_internal" , "file_name": "/lib64/libdbus-glib-1.so.2" } , { "address": 140363036926827 , "build_id": "af67846753967b5f6f87726d7afd0a441969aa3e" , "build_id_offset": 82795 , "function_name": "dbus_g_proxy_call" , "file_name": "/lib64/libdbus-glib-1.so.2" } , { "address": 140363081882658 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 14867490 , "function_name": "mozilla::hal_impl::UPowerClient::UpdateTrackedDeviceSync()" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363081883116 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 14867948 , "function_name": "mozilla::hal_impl::UPowerClient::BeginListening()" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363091638726 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 24623558 , "function_name": "mozilla::dom::battery::BatteryManager::Init()" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363085584846 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 18569678 , "function_name": "mozilla::dom::Navigator::GetDeprecatedBattery(mozilla::ErrorResult&)" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363087604487 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 20589319 , "function_name": "mozilla::dom::NavigatorBinding::get_battery(JSContext*, JS::Handle<JSObject*>, mozilla::dom::Navigator*, JSJitGetterCallArgs)" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363091627893 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 24612725 , "function_name": "mozilla::dom::GenericBindingGetter(JSContext*, unsigned int, JS::Value*)" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363112001687 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 44986519 , "function_name": "js::Invoke(JSContext*, JS::CallArgs const&, js::MaybeConstruct)" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363112006797 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 44991629 , "function_name": "js::Invoke(JSContext*, JS::Value const&, JS::Value const&, unsigned int, JS::Value const*, JS::MutableHandle<JS::Value>)" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363112007135 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 44991967 , "function_name": "js::InvokeGetter(JSContext*, JS::Value const&, JS::Value, JS::MutableHandle<JS::Value>)" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363112119985 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 45104817 , "function_name": "CallGetter(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<js::Shape*>, JS::MutableHandle<JS::Value>)" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363112216625 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 45201457 , "function_name": "js::NativeGetProperty(JSContext*, JS::Handle<js::NativeObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>)" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363108522825 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 41507657 , "function_name": "js::GetProperty(JSContext*, JS::Handle<JSObject*>, JS::Handle<JS::Value>, JS::Handle<jsid>, JS::MutableHandle<JS::Value>)" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363108713163 , "build_id": "bd0af220a5292513496e7d4909c2d84163d46e18" , "build_id_offset": 41697995 , "function_name": "js::jit::DoGetElemFallback(JSContext*, js::jit::BaselineFrame*, js::jit::ICGetElem_Fallback*, JS::Handle<JS::Value>, JS::Handle<JS::Value>, JS::MutableHandle<JS::Value>)" , "file_name": "/usr/lib64/firefox/libxul.so" } , { "address": 140363358510496 , "build_id_offset": 140363358510496 } ] based on which I suspect imgur (or an ad it loads...) is asking for battery information and we get stuck asking dbus for it... or something along those lines. :snorp, you seem to know what this stuff is about based on hg log on the battery manager... can you take a more in-depth look? (also pulling in :bz in the CC list in case he has thoughts...)
Flags: needinfo?(snorp)
Component: Untriaged → DOM: Device Interfaces
Product: Firefox → Core
Sounds to me like we make a call to dbus and that call just ... blocks. That's not OK.
Sorry, I don't know anything about how the desktop Linux backend works.
Flags: needinfo?(snorp)
Do you know who might know about it?
(In reply to Boris Zbarsky [:bz] from comment #8) > Do you know who might know about it? Indeed, that would be useful...
Flags: needinfo?(snorp)
btw it's not just that one imgur site that hangs my browser, my browser regularly hangs, particulary on reddit links, and needs to be killed. even embeds of youtube videos hang it.
Another few hangs with kills required... and I notice some of these in dmesg: Chrome_ChildThr[23711]: segfault at 0 ip 00005624a848c044 sp 00007f875f0fe390 error 6 in plugin-container[5624a8483000+3e000] Chrome_ChildThr[29231]: segfault at 0 ip 0000561f89322044 sp 00007f7a7b1fe390 error 6 in plugin-container[561f89319000+3e000] Chrome_ChildThr[29483]: segfault at 0 ip 0000564a6c97a044 sp 00007f7d1abfe390 error 6 in plugin-container[564a6c971000+3e000] Chrome_ChildThr[27634]: segfault at 0 ip 0000563d41574044 sp 00007f878f7fe390 error 6 in plugin-container[563d4156b000+3e000] Chrome_ChildThr[4660]: segfault at 0 ip 000055b84532c044 sp 00007f8a2a1fe390 error 6 in plugin-container[55b845323000+3e000]
Mounir may know.
Flags: needinfo?(mounir)
Whiteboard: btpp-followup-2016-06-06
The very first call to the UPowerClient (when starting to listen) blocks because at the time the Battery API required to have a value synchronously. The API changed in between and it is now returning a promise to be resolved when a value is found so the backend could be changed to no longer synchronously get the values. Though, looking at Navigator::GetBattery, it sounds like the promise is resolved ASAP.
Flags: needinfo?(mounir)
Summary: Firefox hangs when I visit imgur → Firefox hangs all the bloody time, like when I visit imgur, try to read a bill, try to watch a video, etc etc etc etc etc
Hang number six this evening. [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 393573344}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 409829128}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 427152249}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 443467475}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 459810082}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 477136154}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 493493705}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 509799465}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 527118379}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 543378133}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 560696656}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 576978170}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 593296631}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 610656811}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 627039227}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 643428752}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 659744638}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 677119342}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 693437996}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 709719053}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 727077127}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 743379309}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 760696687}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 776901103}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 793297826}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 810595228}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 826877550}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 843212743}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 860585906}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 876885407}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 893219975}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 910577253}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 926919221}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 943295024}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 960607548}, ffffffff <unfinished ...> [pid 31574] <... futex resumed> ) = -1 ETIMEDOUT (Connection timed out) [pid 31574] tgkill(31546, 31546, SIGVTALRM) = 0 [pid 31546] <... poll resumed> ) = ? ERESTART_RESTARTBLOCK (Interrupted by signal) [pid 31574] futex(0x7f0dc8d29190, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31574] futex(0x7f0dc41c1d8c, FUTEX_WAIT_BITSET_PRIVATE|FUTEX_CLOCK_REALTIME, 129, {1466108209, 7650000}, ffffffff <unfinished ...> [pid 31546] --- SIGVTALRM {si_signo=SIGVTALRM, si_code=SI_TKILL, si_pid=31546, si_uid=1000} --- [pid 31546] rt_sigreturn({mask=[]}) = -1 EINTR (Interrupted system call) [pid 31546] poll([{fd=100, events=POLLIN}], 1, 25000 <unfinished ...> [pid 31612] <... futex resumed> ) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 976735561}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729975, 993081109}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729976, 10393062}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729976, 26707985}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729976, 43076050}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729976, 60444510}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729976, 76765775}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729976, 93130078}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729976, 110306244}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729976, 126666650}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729976, 143853429}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729976, 160218573}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729976, 176491573}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0 [pid 31612] futex(0x7f0da96fec14, FUTEX_WAIT_BITSET_PRIVATE, 1, {1729976, 193835290}, ffffffff) = -1 ETIMEDOUT (Connection timed out) [pid 31612] futex(0x7f0da96febe8, FUTEX_WAKE_PRIVATE, 1) = 0
How do we get this radarized so we solve this source of hangs for linux folks?
Flags: needinfo?(overholt)
Let me see if I can find someone to take it.
Flags: needinfo?(overholt)
Assignee: nobody → amarchesini
Status: UNCONFIRMED → NEW
Ever confirmed: true
This second patch does several things: 1. Introduces RetrieveBatteryInformation(). This method makes hal to dispatch a notification about the batteryStatus. 2. If BatteryManager has a pending promise, the notification is used to resolve such promise. In this way I make GetBattery() really async. 3. For linux hal I moved all the 'sync' part into a single method. This method will go away once 2 b2g dependences will removed. I'll work on QuotaManager in order to use the async RetrieveBatteryInformation(). 4. The rest of UPowerClient works completely async. 5. UPowerClient code has been cleaned up.
Attachment #8765813 - Flags: review?(karlt)
Summary: Firefox hangs all the bloody time, like when I visit imgur, try to read a bill, try to watch a video, etc etc etc etc etc → On linux, Firefox should speak with dbus asynchronously for the retrieving of battery status.
Attachment #8765812 - Flags: review?(bugs) → review+
Comment on attachment 8765813 [details] [diff] [review] part 2 - Introduce RetrieveBatteryInformation() > /** >+ * Ask for battery information asynchronously. A notification will be >+ * disaptched when the data is available. dispatched >+RetrieveBatteryInformation() >+{ >+ class BatteryInformationRunnable final : public Runnable >+ { >+ public: >+ NS_IMETHOD >+ Run() override >+ { >+ hal::BatteryInformation currentInfo; >+ hal_impl::GetCurrentBatteryInformation(&currentInfo); >+ hal::NotifyBatteryChange(currentInfo); >+ return NS_OK; >+ } >+ }; >+ >+ RefPtr<BatteryInformationRunnable> runnable = >+ new BatteryInformationRunnable(); >+ >+ NS_WARN_IF(NS_FAILED(NS_DispatchToCurrentThread(runnable))); >+} >+ ... >+void >+RetrieveBatteryInformation() >+{ >+ class BatteryInformationRunnable final : public Runnable >+ { >+ public: >+ NS_IMETHOD >+ Run() override >+ { >+ hal::BatteryInformation currentInfo; >+ hal_impl::GetCurrentBatteryInformation(&currentInfo); >+ hal::NotifyBatteryChange(currentInfo); >+ return NS_OK; >+ } >+ }; >+ >+ RefPtr<BatteryInformationRunnable> runnable = >+ new BatteryInformationRunnable(); >+ >+ NS_WARN_IF(NS_FAILED(NS_DispatchToCurrentThread(runnable))); >+} just wondering if there was some way to have less duplicated code. >+void >+RetrieveBatteryInformation() >+{ >+ class BatteryInformationRunnable final : public Runnable >+ { >+ public: >+ NS_IMETHOD >+ Run() override >+ { >+ hal::BatteryInformation currentInfo; >+ hal_impl::GetCurrentBatteryInformation(&currentInfo); >+ hal::NotifyBatteryChange(currentInfo); >+ return NS_OK; >+ } >+ }; >+ >+ RefPtr<BatteryInformationRunnable> runnable = >+ new BatteryInformationRunnable(); >+ >+ NS_WARN_IF(NS_FAILED(NS_DispatchToCurrentThread(runnable))); >+} oh, 3rd time. > GetCurrentBatteryInformation(hal::BatteryInformation* aBatteryInfo) > { >- UPowerClient* upowerClient = UPowerClient::GetInstance(); >+ MOZ_ASSERT(aBatteryInfo); > >- aBatteryInfo->level() = upowerClient->GetLevel(); >- aBatteryInfo->charging() = upowerClient->IsCharging(); >- aBatteryInfo->remainingTime() = upowerClient->GetRemainingTime(); >+ class MOZ_RAII RAII final >+ { >+ public: >+ RAII() >+ : mDBusConnection(nullptr) >+ , mProxy(nullptr) >+ {} >+ >+ ~RAII() >+ { >+ if (mProxy) { >+ g_object_unref(mProxy); >+ } >+ >+ if (mDBusConnection) { >+ dbus_g_connection_unref(mDBusConnection); >+ } >+ } >+ >+ DBusGConnection* mDBusConnection; >+ DBusGProxy* mProxy; >+ }; Since I know next to nothing about dbus, not giving feedback about its use.
Attachment #8765813 - Flags: feedback+
Comment on attachment 8765813 [details] [diff] [review] part 2 - Introduce RetrieveBatteryInformation() This patch doesn't work if there are not batteries or UPower is not running.
Flags: needinfo?(snorp)
Flags: needinfo?(simon__b)
Attachment #8765813 - Flags: review?(karlt)
Attachment #8765813 - Attachment is obsolete: true
Attachment #8765926 - Flags: review?(karlt)
Blocks: 703612
Comment on attachment 8765926 [details] [diff] [review] part 2 - Introduce RetrieveBatteryInformation() Doing this async sounds great, thanks. (In reply to Andrea Marchesini (:baku) from comment #18) > 3. For linux hal I moved all the 'sync' part into a single method. This > method will go away once 2 b2g dependences will removed. I'll work on > QuotaManager in order to use the async RetrieveBatteryInformation(). > 4. The rest of UPowerClient works completely async. > 5. UPowerClient code has been cleaned up. Much has changed here and I haven't yet worked out how it all fits together, but here are some questions that I have from a first look through. Is the Linux GetCurrentBatteryInformation() changing from returning the currently known value to performing sync calls every invocation? If so, then I think this needs to wait until the Linux implementation can be removed. I can still review the other changes. >+NS_IMPL_CYCLE_COLLECTION_INHERITED(BatteryManager, >+ DOMEventTargetHelper, >+ mPendingPromise); This is only necessary if mPendingPromise is going to be held indefinitely. If mPendingPromise is cleared when UnregisterBatteryObserver(this) is called, then can this object be kept out of cycle collection? Is there any reason why a notification should not be received? >+ * Ask for battery information asynchronously. A notification will be >+ * disaptched when the data is available. >+ */ >+void RetrieveBatteryInformation(); Is this function needed? Could RegisterBatteryObserver() be sufficient? >+ mBatteryInfo.charging() =kDefaultCharging; Indentation.
Flags: needinfo?(amarchesini)
> Is the Linux GetCurrentBatteryInformation() changing from returning the > currently known value to performing sync calls every invocation? Yes. The reason I didn't remove it is because gonk and bluetooth are still using this sync method. Both components will be removed when b2g will be definitely removed from m-c. I wanted to make it completely independent from the rest of the code so that removing it will be easier. So, yes, this is always sync. And it should not be called in new code. > >+NS_IMPL_CYCLE_COLLECTION_INHERITED(BatteryManager, > >+ DOMEventTargetHelper, > >+ mPendingPromise); > > This is only necessary if mPendingPromise is going to be held indefinitely. It's always nice to CC/GC things correctly. BatteryManager is always CCed because it's a DOMEventTargetHelper. Having this Traverse/Unlink for mPendingPromise is not a bit deal. Smaug already gave me a positive feedback about it. > >+void RetrieveBatteryInformation(); > > Is this function needed? Yes it's needed because when this method is called, a notification is always sent. With RegisterBatteryObserver we dispatch notification only when something changes.
Flags: needinfo?(amarchesini)
(I'm a bit scared to comment here since you are all doing so much work. I upgraded to Fedora 24 and for some reason I am not able to trigger this bug tens of times per day any more. I guess it's very racey.)
(In reply to simon__b from comment #24) > (I'm a bit scared to comment here since you are all doing so much work. I > upgraded to Fedora 24 and for some reason I am not able to trigger this bug > tens of times per day any more. I guess it's very racey.) Does it happen with my patch as well?
Attachment #8765926 - Attachment is obsolete: true
Attachment #8765926 - Flags: review?(karlt)
Attachment #8775939 - Flags: review?(karlt)
Karlt, news?
Flags: needinfo?(karlt)
What's the status here? Karl, should someone else review this? Or are we decided by now that we're removing this API altogether, or...?
We still need this. QuotaManager uses HAL and this battery check.
(In reply to Andrea Marchesini [:baku] from comment #23) > > Is the Linux GetCurrentBatteryInformation() changing from returning the > > currently known value to performing sync calls every invocation? > > Yes. The reason I didn't remove it is because gonk and bluetooth are still > using this sync method. > Both components will be removed when b2g will be definitely removed from m-c. Have these been removed now? Can the sync HAL API be removed? Changing GetCurrentBatteryInformation() from returning the currently known value (after a single sync initialization) to performing sync calls every invocation is not an option. If this is going to go away, then I can review the other path, but it is not a priority until the sync API can go away. > > >+void RetrieveBatteryInformation(); > > > > Is this function needed? > > Yes it's needed because when this method is called, a notification is always > sent. > With RegisterBatteryObserver we dispatch notification only when something > changes. Why can't RegisterBatteryObserver() trigger an initial notification also?
Flags: needinfo?(karlt)
Comment on attachment 8775939 [details] [diff] [review] part 2 - Introduce RetrieveBatteryInformation() Review of attachment 8775939 [details] [diff] [review]: ----------------------------------------------------------------- I ask also Shawn to take a look. Please, move/cancel the request if you want.
Attachment #8775939 - Flags: review?(shuang)
Comment on attachment 8775939 [details] [diff] [review] part 2 - Introduce RetrieveBatteryInformation() (In reply to Andrea Marchesini [:baku] from comment #31) > > Changing GetCurrentBatteryInformation() from returning the currently known > > value (after a single sync initialization) to performing sync calls every > > invocation is not an option. > > As my patch writes in a comment, GetCurrentBatteryInformation() is not used > in gecko directly. (For some platform GetCurrentBatteryInformation() doesn't > involve a sync I/O operation). What do you mean by "directly"? If GetCurrentBatteryInformation() is not used, then why not have fill-in code to assert not reached, and return an error, or fallback values, if it is not possible to return an error? But the comment is worded quite differently: "At the moment it's currently used by QuotaManager and some b2g components." If QuotaManager is not b2g and not Gecko, but is the reason that this bug needs fixing (comment 29), then what uses QuotaManager? > > Why can't RegisterBatteryObserver() trigger an initial notification also? > > I don't understand what you mean with an initial notification. Why can't RegisterBatteryObserver() also do the equivalent of RetrieveBatteryInformation()? Why is it necessary to have the extra RetrieveBatteryInformation() call/method? > These patches have been waiting for a review since 2016-07, this is a bit > more than 24 hours. > And this is the first comment I receive since then. In 2016-07 (comment 22), I indicated that this approach is not suitable if you don't want to wait until GetCurrentBatteryInformation() can be removed from the Linux implementation. Please don't expect prompt reviews if you are not going to address the issues pointed out by the reviewer and provide the answers that the reviewer needs to review. If you want the reviewer to investigate, then the reviewer can prioritize that against other improvements. > I really would like to get rid of this sync I/O on main-thread on linux > because it creates many issues and it slows down firefox. If GetCurrentBatteryInformation() needs to continue to return reasonable values for now, then please don't change it to behave worse than it does now. If it is still used, then is returning fallback values when called before initialization has completed an option? This is an r- for GetCurrentBatteryInformation(). I didn't remove the review request in 2016-07 because there are other parts of the patch that could be useful if you prefer to wait until GetCurrentBatteryInformation() can be removed, and I have not reviewed those parts. As you have indicated that you do not want to wait for that, a different approach may be required and so I won't review those parts at this stage.
Flags: needinfo?(karlt)
Attachment #8775939 - Flags: review?(karlt) → review-
> If QuotaManager is not b2g and not Gecko, but is the reason that this bug > needs fixing (comment 29), then what uses QuotaManager? This is a kind of deadlock: QuotaManager cannot use the async hal method, because it doesn't exist. This method introduces by these patches. When it lands, we can fix QuotaManager. As follow up in a separate bug, probably. > Why can't RegisterBatteryObserver() also do the equivalent of > RetrieveBatteryInformation()? Why is it necessary to have the extra > RetrieveBatteryInformation() call/method? They have 2 different meanings and use-cases: - RegisterBatteryObserver: when the battery level changes, a notification is received. - RetrieveBatteryInformation: tell me, asynchronously, the status of the battery. It seems reasonable to have these 2 separate method. > If GetCurrentBatteryInformation() needs to continue to return reasonable > values for now, then please don't change it to behave worse than it does now. Actually, I didn't. GetCurrentBatteryInformation() returns the status of the battery synchronously. The behavior didn't change. What before was done in UpdateTrackedDeviceSync() now is part of GetCurrentBatteryInformation(). If you see any changes in how GetCurrentBatteryInformation() works, please, tell me more. > If it is still used, then is returning fallback values when called before > initialization has completed an option? Not at all. It returns always the correct value.
Flags: needinfo?(karlt)
(In reply to Andrea Marchesini [:baku] from comment #34) > > If QuotaManager is not b2g and not Gecko, but is the reason that this bug > > needs fixing (comment 29), then what uses QuotaManager? > > This is a kind of deadlock: QuotaManager cannot use the async hal method, > because it doesn't exist. > This method introduces by these patches. When it lands, we can fix > QuotaManager. As follow up in a separate bug, probably. OK. I think I get the picture now, thanks: The intention is that GetCurrentBatteryInformation() will not be used in Gecko, but it will still be used in Gecko after this patch lands until QuotaManager is changed. I'll mark comment 31 as a typo, assuming you intended to write something different from "GetCurrentBatteryInformation() is not used in gecko directly." I recommend unchecking all "but not when" "The change was made by me" boxes on https://bugzilla.mozilla.org/userprefs.cgi?tab=email I find that useful so that I can notice when I've made a mistake. Can GetCurrentBatteryInformation() be removed when QuotaManager is changed?
(In reply to Andrea Marchesini [:baku] from comment #34) > > Why can't RegisterBatteryObserver() also do the equivalent of > > RetrieveBatteryInformation()? Why is it necessary to have the extra > > RetrieveBatteryInformation() call/method? > > They have 2 different meanings and use-cases: > > - RegisterBatteryObserver: when the battery level changes, a notification is > received. > - RetrieveBatteryInformation: tell me, asynchronously, the status of the > battery. > > It seems reasonable to have these 2 separate method. It is reasonable to have 2 methods if we need 2 methods. If one is sufficient, then one is better than two, and so the second should not be added. If it is OK for RegisterBatteryObserver() to send the notification that would be generated by RetrieveBatteryInformation(), then there is no need for callers of RegisterBatteryObserver() to also call RetrieveBatteryInformation(). I would expect that anything calling RegisterBatteryObserver() also wants to know the initial status. Even if there are exceptions, I don't expect sending an initial notification to cause any problem in those exceptions. It is essentially a notification that the state has changed from unknown to known. RetrieveBatteryInformation() is not much use without RegisterBatteryObserver(). If there are no clients needing to call RetrieveBatteryInformation() more than once for each RegisterBatteryObserver() call, then there is no need for the RetrieveBatteryInformation() API at all. Even if there is a client that leaves an observer registered but does not take note of all changes in the battery state and subsequently wants to know about some state that it ignored, then it can RegisterBatteryObserver() again. Still this seems an unlikely scenario, and perhaps one that would be better served with GetCurrentBatteryInformation() just returning any cached state, as it does now, but with a way to detect when the state is unknown and/or pending.
(In reply to Andrea Marchesini [:baku] from comment #34) > > If GetCurrentBatteryInformation() needs to continue to return reasonable > > values for now, then please don't change it to behave worse than it does > > now. > > Actually, I didn't. GetCurrentBatteryInformation() returns the status of the > battery synchronously. > The behavior didn't change. Are you making a distinction here from the acknowledgement in comment 23 that there was a change in behavior, or are these statements inconsistent? > What before was done in > UpdateTrackedDeviceSync() now is part of GetCurrentBatteryInformation(). > > If you see any changes in how GetCurrentBatteryInformation() works, please, > tell me more. https://bugzilla.mozilla.org/attachment.cgi?id=8775939&action=diff#a/hal/linux/UPowerClient.cpp_sec4 GetCurrentBatteryInformation() merely returns the values currently known. It makes no effort to retrieve the values. If there is no existing UPowerClient singleton, then it is created with default/fallback battery state. UpdateTrackedDeviceSync() is called only from UPowerClient::BeginListening(), which happens only when the observer count increases to 1. The patch is proposing changing the behavior of GetCurrentBatteryInformation() to make sync calls. > > If it is still used, then is returning fallback values when called before > > initialization has completed an option? > > Not at all. It returns always the correct value. It looks like GetCurrentBatteryInformation() would currently return fallback values for QuotaManager when called before the UPowerClient state has been set (and after the last battery observer is unregistered). Is QuotaManager depending on something else to call RegisterBatteryObserver()?
Flags: needinfo?(karlt)
> Is QuotaManager depending on something else to call > RegisterBatteryObserver()? No. It seems just buggy code. I'll file a bug for this.
Attachment #8775939 - Attachment is obsolete: true
Attachment #8775939 - Flags: review?(shuang)
Attachment #8814320 - Flags: review?(karlt)
Is there anything we can do to help merging this fix? Manual tests or whatever? Thanks!
(In reply to Mathieu Leplatre (:leplatrem) from comment #40) Do you have reason to believe that this is still affecting people after changes for bug 1313580?
Flags: needinfo?(mathieu)
QuotaManager uses this API and it runs on the parent process.
Flags: needinfo?(mathieu)
Karlt, maybe we can ask somebody else to review this patch. Can you move the review request to somebody else, please?
Flags: needinfo?(karlt)
Comment on attachment 8814320 [details] [diff] [review] part 2 - Introduce RetrieveBatteryInformation() The RetrieveBatteryInformation() API is not required. Instead, ensure a notification is sent after RegisterBatteryObserver(). This patch is also still changing GetCurrentBatteryInformation() (and doesn't mention this in the commit message). That is not an improvement and not necessary. Once RegisterBatteryObserver() triggers the notification, QuotaManager can use this and listen for the notification.
Attachment #8814320 - Flags: review?(karlt) → review-
(In reply to Karl Tomlinson (:karlt) from comment #37) > It looks like GetCurrentBatteryInformation() would currently return fallback > values for QuotaManager when called before the UPowerClient state has been > set (and after the last battery observer is unregistered). > > Is QuotaManager depending on something else to call > RegisterBatteryObserver()? (In reply to Andrea Marchesini [:baku] from comment #38) > No. It seems just buggy code. I'll file a bug for this. (In reply to Karl Tomlinson (:karlt) from comment #41) > (In reply to Mathieu Leplatre (:leplatrem) from comment #40) > > Do you have reason to believe that this is still affecting people after > changes for bug 1313580? (In reply to Andrea Marchesini [:baku] from comment #42) > QuotaManager uses this API and it runs on the parent process. It would help to explain that you reintroduced this bug between comment 38 and comment 42 when you added the RegisterBatteryObserver() call for bug 1320275. The situation may be worse now because a single observer is repeatedly added and removed, triggering the sync call each time. Best fix would be to back out that change. This code was never ready for production. The leak of strings in the GPtrArray, identified during development, was not fixed.
Flags: needinfo?(karlt)
Blocks: 1320275
I don't have time to work on this bug anymore. The review process was extremely painful, with on average, 3 months waiting between patch-submitted and first comments, and 1 month for NI answers. I'm sure everybody is super busy doing so many other things, but I cannot keep this patches in mind and update them any 3 months.
Assignee: amarchesini → nobody
Priority: -- → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: