Closed
Bug 742226
Opened 12 years ago
Closed 12 years ago
Implement a generic uevent poller
Categories
(Core :: Hardware Abstraction Layer (HAL), defect)
Tracking
()
RESOLVED
FIXED
mozilla14
People
(Reporter: cyu, Assigned: cyu)
References
Details
Attachments
(2 files, 6 obsolete files)
4.75 KB,
patch
|
cjones
:
review+
|
Details | Diff | Splinter Review |
7.03 KB,
patch
|
cyu
:
review+
|
Details | Diff | Splinter Review |
This bug is forked from bug 736939 to track the requirement for a generic uevent poller for at least the following requirements: - battery update - audio switch - usb device hotplug - usb cable connection/disconnection There is an initial implementation in patch part 1 in the 736939, but there are some refinements: - refactor current battery updater code to use this uevent backend - the current implementation uses android's uevent decoder (class NetlinkEvent), we might need to come back to reimplement it considering support for Linux platform.
Also, this needs to build in ICS and GB. it might be easiest to just import the android code. Phone
Assignee | ||
Comment 2•12 years ago
|
||
The poller that polls uevent from netlink socket and notifies registered observers.
Attachment #612471 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 3•12 years ago
|
||
Attachment #612472 -
Flags: review?(jones.chris.g)
Comment on attachment 612471 [details] [diff] [review] Uevent poller implementation High-level issue: it's very hard to manage object lifetimes across multiple threads, which this patch and the next attempt to do. Indeed, there's a use-after-free bug/race-condition in the second patch that could result in a security vulnerability. To make the model simplest for IUeventObserver, it'd be nice to have the Broadcast() happen on the main thread. But I understand why you don't want to do that here: we want to discard unrecognized uevents as quickly as possible, to avoid dispatching a lot of garbage to the main thread from the IO thread. So we need to have all this code run on the IO thread. What this means for this patch is, we have to restrict all the UeventPoller and IUeventObserver code to run only on the IO thread. They need to assert when an attempt is made to use them off the IO thread. I'll explain this more in comments below. The end result of this will be that the API will be a bit harder to use for clients, but it will be safer memory-management- (and security-) wise. >diff --git a/hal/gonk/UeventPoller.cpp b/hal/gonk/UeventPoller.cpp >+class NetlinkPoller : public RefCounted<NetlinkPoller>, >+ public MessageLoopForIO::Watcher >+// Mutex mMutex; >+// This code doesn't need a mutex, please remove this. >+void >+NetlinkPoller::OnFileCanReadWithoutBlocking(int fd) >+{ >+ if (ret <= 0) { >+ mReadWatcher.StopWatchingFileDescriptor(); >+ close(mSocket.mFd); >+ mIOLoop->PostTask(FROM_HERE, new UeventInitTask()); >+ return; >+ } We should never get an unrecognized error here. If we do, there's probably something badly wrong in the kernel. We might as well bail out here or _exit(1). I don't think that re-initializing the socket is likely to help. Do you know of a reason that we might get into this error case? >+void >+sRegisterUeventListener(IUeventObserver *aObserver) >+void >+RegisterUeventListener(IUeventObserver *aObserver) Remove the "sRegister" helper, and assert in RegisterUeventListener() that it's running on the IO thread. >+void static >+sUnregisterUeventListener(IUeventObserver *aObserver) >+void >+UnregisterUeventListener(IUeventObserver *aObserver) (Same here.) >diff --git a/hal/gonk/UeventPoller.h b/hal/gonk/UeventPoller.h >+namespace mozilla { >+namespace hal_impl { >+ >+typedef mozilla::Observer<NetlinkEvent> IUeventObserver; >+ >+void RegisterUeventListener(IUeventObserver *aObserver); >+ >+void UnregisterUeventListener(IUeventObserver *aObserver); >+ Document here that this code is callable only from the IO thread, and that Broadcast()s to observers only happen on the IO thread. The rest of the patch looks mostly OK. Please re-post another version with the threading model fixed up :).
Attachment #612471 -
Flags: review?(jones.chris.g)
Comment on attachment 612472 [details] [diff] [review] Refactor battery updater using the uevent poller The DisableBatteryNotifications() code can racily delete the battery observer while the IO thread still holds a reference to it. That can result in a security vulnerability.
Attachment #612472 -
Flags: review?(jones.chris.g) → review-
Assignee | ||
Comment 6•12 years ago
|
||
(In reply to Chris Jones [:cjones] [:warhammer] from comment #5) > The DisableBatteryNotifications() code can racily delete the battery > observer while the IO thread still holds a reference to it. That can result > in a security vulnerability. There is another solution to consider: refcount the observer and then AddRef() when the observer is registered and Release() when unregistered, but the problem here is the registered observer should have refcount capability. This can be done in 2 levels: 1. Create abstract class RefcountedObserver which inherits from Observer and RefCounted. Then RegisterUevnetListener() and UnregisterUeventListener() accepts refcounted observer. 2. Make all existing Observers refcounted. Then the ObserverList will increment/decrement the refcount so it will never hold a deleted observer. Since observer code generally runs in another thread and the observer is the primary object shared between observing and notifying threads, this can prevent from similar errors in the future. Chris, what do you think?
Threadsafe refcounted objects aren't really easier to manage than non-refcounted, in cases like this. I strongly recommend we keep this code IO-thread-only.
Assignee | ||
Comment 8•12 years ago
|
||
Or another simpler fix: just allocate the object statically. This saves us from newing/deleting it. The observer has a simple ctor and should not add noticeable cost to process startup.
Observers can be added and removed at any time. The battery observer is added/removed because it holds XPCOM objects that get counted as leaks on shutdown. TBH, I think the change I'm suggesting makes part 1 simpler, and doesn't change part 2 very much :). Not sure about the headphone code yet. So the changes shouldn't require much more work, I don't think.
Assignee | ||
Comment 10•12 years ago
|
||
Uevent poller implementation v2. Updated per comment #4
Attachment #612471 -
Attachment is obsolete: true
Attachment #613256 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 11•12 years ago
|
||
Updated per comment #4
Attachment #612472 -
Attachment is obsolete: true
Attachment #613258 -
Flags: review?(jones.chris.g)
Comment on attachment 613256 [details] [diff] [review] Uevent poller implementation (v2) >diff --git a/hal/gonk/UeventPoller.cpp b/hal/gonk/UeventPoller.cpp >+namespace mozilla { >+namespace hal_impl { >+ >+void ShutdownUevent(); >+ |static void| please, and on definition below. >+class NetlinkPoller : public RefCounted<NetlinkPoller>, This class doesn't need to be refcounted; see below. >+ virtual ~NetlinkPoller() >+ { >+ mReadWatcher.StopWatchingFileDescriptor(); This is unnecessary, mReadWatcher does this automatically. >+ // no writing to the netlink socket >+ virtual void OnFileCanWriteWithoutBlocking(int fd) { MOZ_ASSERT(false); } Use MOZ_NOT_REACHED() here. >+ typedef ObserverList<NetlinkEvent> UeventObserverList; >+ UeventObserverList mUeventObserverList; >+ Nit: there's an extraneous newline here, please remove it. >+static RefPtr<NetlinkPoller> sPoller; >+ This can be just an nsAutoPtr: there's only ever 0 or 1 references to the sPoller object. >+void >+RegisterUeventListener(IUeventObserver *aObserver) >+{ >+ MOZ_ASSERT(MessageLoop::current() == XRE_GetIOMessageLoop()); >+ >+ if (!sPoller) >+ InitializeUevent(); >+ MOZ_ASSERT(sPoller); No need to assert the poller here either: |::operator new| is infallible in Gecko, meaning that |new Foo()| never returns NULL or throws an exception. And similarly, if sPoller is NULL, we'll safely crash. >+void >+UnregisterUeventListener(IUeventObserver *aObserver) >+ MOZ_ASSERT(sPoller); No need to assert the poller here: if it's null, we'll crash, which is just as good :). >+ sPoller->UnregisterObserver(aObserver); >+} >+ >+} // hal_impl >+} // mozilla >+ >diff --git a/hal/gonk/UeventPoller.h b/hal/gonk/UeventPoller.h >+#include "mozilla/Observer.h" >+ >+// from android >+#include "sysutils/NetlinkEvent.h" Nit: Please remove this comment, list this header before "mozilla/Observer.h", and include it as <sysutils/NetlinkEvent.h>. >+ Nit: Extra newline here: just one please. >+/** >+ * Register for uevent notification. Note that the method should run on the >+ * <b> IO Thread </b> >+ * @aObserver the observer to be added Please also add that the observer's Notify() method is only called on the IO thread. This patch looks good! :D I'd like to see one more version with the comments above addressed.
Attachment #613256 -
Flags: review?(jones.chris.g)
Comment on attachment 613258 [details] [diff] [review] Refactor battery updater using the uevent poller (v2) >diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp >+static BatteryObserver *sBatteryObserver = NULL; Note here that the IO thread "owns" this object: only the IO thread may create/destroy it. >+static void >+sEnableBatteryNotifications() Please call this |RegisterBatteryObserverIOThread()| >+{ Assert here that this code is running on the IO thread. >+ if (!sBatteryObserver) Because the messages from main-thread to IO-thread are serialized in the order they're generated on the main thread, the state will "mirror" the main-thread state, except that it might be delayed a bit. What that means here is that |RegisterBatteryObserverIOThread()| should *never* be called if |sBatteryObserver| already exists. So please assert !sBatteryObserver here. >+ sBatteryObserver = new BatteryObserver; Please use |new BatteryObserver();| --- the idiom |new BatteryObserver;| has a specific meaning that's not intended here :). It's a little distracting. >+static void >+sDisableBatteryNotifications() Similarly to above, please call this |UnregisterBatteryObserverIOThread()|. >+{ >+ if (sBatteryObserver) { Similar to the logic above, sBatteryObserver should always be non-null here. Please assert that. This looks better. I would like to see the next version with the comments above fixed. Almost there! :)
Attachment #613258 -
Flags: review?(jones.chris.g)
Updated•12 years ago
|
Component: General → Hardware Abstraction Layer (HAL)
Product: Boot2Gecko → Core
QA Contact: general → hal
Assignee | ||
Comment 14•12 years ago
|
||
Updated per review comment #12
Attachment #613256 -
Attachment is obsolete: true
Attachment #613516 -
Flags: review?(jones.chris.g)
Assignee | ||
Comment 15•12 years ago
|
||
Updated per comment #13
Attachment #613258 -
Attachment is obsolete: true
Attachment #613517 -
Flags: review?(jones.chris.g)
Updated•12 years ago
|
Attachment #613516 -
Flags: review?(jones.chris.g) → review+
Comment on attachment 613517 [details] [diff] [review] Refactor battery updater using the uevent poller (v3) (In reply to Chris Jones [:cjones] [:warhammer] from comment #13) > Comment on attachment 613258 [details] [diff] [review] > Refactor battery updater using the uevent poller (v2) > > >diff --git a/hal/gonk/GonkHal.cpp b/hal/gonk/GonkHal.cpp > > >+static BatteryObserver *sBatteryObserver = NULL; > > Note here that the IO thread "owns" this object: only the IO thread > may create/destroy it. > Missed this comment. r=me with that comment added. \o/
Attachment #613517 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Comment 17•12 years ago
|
||
updated per comment #16
Attachment #613517 -
Attachment is obsolete: true
Attachment #613896 -
Flags: review?(jones.chris.g)
Comment on attachment 613896 [details] [diff] [review] Refactor battery updater using the uevent poller (v4) (For future reference, if I say "r=me if you do X, Y, Z" in a review comment, it means that you don't need to request review from me again; you have my r+ if you do X, Y, Z. But always feel free to request review again if you want to :) .)
Attachment #613896 -
Flags: review?(jones.chris.g) → review+
Assignee | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 19•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f0af0cf88a9 https://hg.mozilla.org/integration/mozilla-inbound/rev/dd28759eba7b No tests?
Comment 20•12 years ago
|
||
Backed out due to b2g bustage. You got bitrotted by bug 728171. Please rebase. https://hg.mozilla.org/integration/mozilla-inbound/rev/1ae36c75ffb3 Here's a log: https://tbpl.mozilla.org/php/getParsedLog.php?id=10854889&tree=Mozilla-Inbound UeventPoller.cpp /usr/bin/ccache /builds/slave/m-in-b2g/build/gonk-toolchain/prebuilt/linux-x86/toolchain/arm-eabi-4.4.3/bin/arm-eabi-g++ -o UeventPoller.o -c -fvisibility=hidden -DMOZILLA_INTERNAL_API -D_IMPL_NS_COM -DEXPORT_XPT_API -DEXPORT_XPTC_API -D_IMPL_NS_GFX -D_IMPL_NS_WIDGET -DIMPL_XREAPI -DIMPL_NS_NET -DIMPL_THEBES -DSTATIC_EXPORTABLE_JS_API -DOSTYPE=\"Linux\" -DOSARCH=Linux -DEXCLUDE_SKIA_DEPENDENCIES -DOS_LINUX=1 -DOS_POSIX=1 -I/builds/slave/m-in-b2g/build/ipc/chromium/src -I/builds/slave/m-in-b2g/build/ipc/glue -I../ipc/ipdl/_ipdlheaders -I/builds/slave/m-in-b2g/build/hal -I. -I../dist/include -I../dist/include/nsprpub -I/builds/slave/m-in-b2g/build/obj-b2g/dist/include/nspr -I/builds/slave/m-in-b2g/build/obj-b2g/dist/include/nss -fPIC -DANDROID -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/include/ -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/kernel/common -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/arch-arm/include -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/kernel/arch-arm -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libm/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/opengl/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/native/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/hardware/libhardware/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/hardware/libhardware_legacy/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/system/core/include -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/services/sensorservice -fno-rtti -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wno-ctor-dtor-privacy -Wno-overlength-strings -Wno-invalid-offsetof -Wno-variadic-macros -Wno-long-long -mandroid -fno-short-enums -fno-exceptions -DMOZ_ENABLE_JS_DUMP -I/builds/slave/m-in-b2g/build/gonk-toolchain/ndk/sources/cxx-stl/stlport/stlport/ -march=armv7-a -mthumb -mfpu=vfp -mfloat-abi=softfp -fno-exceptions -fno-strict-aliasing -std=gnu++0x -ffunction-sections -fdata-sections -pipe -DNDEBUG -DTRIMMED -g -Os -freorder-blocks -fno-reorder-functions -fno-omit-frame-pointer -funwind-tables -DANDROID -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/include/ -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/kernel/common -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/arch-arm/include -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libc/kernel/arch-arm -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic/libm/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/opengl/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/native/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/hardware/libhardware/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/hardware/libhardware_legacy/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/system/core/include -isystem /builds/slave/m-in-b2g/build/gonk-toolchain/bionic -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/include -I/builds/slave/m-in-b2g/build/gonk-toolchain/frameworks/base/services/sensorservice -DMOZILLA_CLIENT -include ../mozilla-config.h -MD -MF .deps/UeventPoller.pp /builds/slave/m-in-b2g/build/hal/gonk/UeventPoller.cpp ../../hal/gonk/UeventPoller.cpp: In member function 'bool mozilla::hal_impl::NetlinkPoller::OpenSocket()': ../../hal/gonk/UeventPoller.cpp:82: error: 'class mozilla::ScopedClose' has no member named 'mFd' ../../hal/gonk/UeventPoller.cpp:83: error: 'class mozilla::ScopedClose' has no member named 'mFd' ../../hal/gonk/UeventPoller.cpp:88: error: 'class mozilla::ScopedClose' has no member named 'mFd' ../../hal/gonk/UeventPoller.cpp:92: error: 'class mozilla::ScopedClose' has no member named 'mFd' ../../hal/gonk/UeventPoller.cpp:97: error: 'class mozilla::ScopedClose' has no member named 'mFd' ../../hal/gonk/UeventPoller.cpp:101: error: 'class mozilla::ScopedClose' has no member named 'mFd' ../../hal/gonk/UeventPoller.cpp:110: error: 'class mozilla::ScopedClose' has no member named 'mFd' ../../hal/gonk/UeventPoller.cpp:113: error: 'class mozilla::ScopedClose' has no member named 'mFd' make[5]: *** [UeventPoller.o] Error 1 make[5]: *** Waiting for unfinished jobs.... In file included from ../../gonk-toolchain/frameworks/base/include/utils/Log.h:31, from ../../gonk-toolchain/frameworks/base/include/utils/Vector.h:24, from ../../gonk-toolchain/frameworks/base/include/utils/SortedVector.h:24, from ../../gonk-toolchain/frameworks/base/include/utils/KeyedVector.h:24, from ../../gonk-toolchain/frameworks/base/services/sensorservice/SensorDevice.h:23, from ../../hal/gonk/GonkSensor.cpp:13: ../../gonk-toolchain/system/core/include/cutils/log.h:320:1: warning: "LOG_ASSERT" redefined In file included from ../../ipc/chromium/src/base/string_util_posix.h:13, from ../../ipc/chromium/src/base/string_util.h:106, from ../../ipc/chromium/src/chrome/common/ipc_message_utils.h:13, from ../dist/include/IPC/IPCMessageUtils.h:42, from ../ipc/ipdl/_ipdlheaders/mozilla/hal_sandbox/PHal.h:14, from ../../hal/Hal.h:10, from ../../hal/gonk/GonkSensor.cpp:9: ../../ipc/chromium/src/base/logging.h:101:1: warning: this is the location of the previous definition ../../hal/gonk/GonkSensor.cpp:21:1: warning: "LOG" redefined ../../ipc/chromium/src/base/logging.h:87:1: warning: this is the location of the previous definition ../../hal/gonk/GonkHal.cpp:38:1: warning: "LOG" redefined In file included from ../../ipc/chromium/src/base/observer_list.h:13, from ../../ipc/chromium/src/base/message_loop.h:16, from ../../hal/gonk/GonkHal.cpp:7: ../../ipc/chromium/src/base/logging.h:87:1: warning: this is the location of the previous definition ../../hal/gonk/GonkHal.cpp: In function 'void mozilla::hal_impl::GetCurrentBatteryInformation(mozilla::hal::BatteryInformation*)': ../../hal/gonk/GonkHal.cpp:300: warning: format '%s' expects type 'char*', but argument 3 has type 'char (*)[16]' make[5]: Leaving directory `/builds/slave/m-in-b2g/build/obj-b2g/hal' make[4]: *** [libs_tier_platform] Error 2 make[4]: Leaving directory `/builds/slave/m-in-b2g/build/obj-b2g' make[3]: *** [tier_platform] Error 2 make[3]: Leaving directory `/builds/slave/m-in-b2g/build/obj-b2g' make[2]: *** [default] Error 2 make[2]: Leaving directory `/builds/slave/m-in-b2g/build/obj-b2g' make[1]: *** [realbuild] Error 2 make[1]: Leaving directory `/builds/slave/m-in-b2g/build' make: *** [build] Error 2
Target Milestone: mozilla14 → ---
Assignee | ||
Comment 21•12 years ago
|
||
Rebase the patch on bug 728171. Test result: https://tbpl.mozilla.org/?tree=Try&rev=ab39af663ec5 The failures should not be relevant to the patch
Attachment #613516 -
Attachment is obsolete: true
Attachment #614690 -
Flags: review+
Updated•12 years ago
|
Keywords: checkin-needed
Comment 22•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/04ddd85d3d13 https://hg.mozilla.org/integration/mozilla-inbound/rev/54d4e69c07af Again, no tests?
Keywords: checkin-needed
Target Milestone: --- → mozilla14
Assignee | ||
Comment 23•12 years ago
|
||
Hi Ryan, This is gonk hal layer implementation so I had only manual tests and regression tests on try server. Do you think I need to have more tests? Thanks.
Comment 24•12 years ago
|
||
Automatic tests are always a good thing. If you feel that the existing tests are sufficient, that's fine. I'm just asking the question :)
Comment 25•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/04ddd85d3d13 https://hg.mozilla.org/mozilla-central/rev/54d4e69c07af
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
We'll test this code through higher-level interfaces, the DOM APIs and features that rely on this. The code here isn't a public interface so the value of unit testing it is close to nil. (It's also extremely hard to unit test.)
You need to log in
before you can comment on or make changes to this bug.
Description
•