Closed Bug 742226 Opened 12 years ago Closed 12 years ago

Implement a generic uevent poller

Categories

(Core :: Hardware Abstraction Layer (HAL), defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla14

People

(Reporter: cyu, Assigned: cyu)

References

Details

Attachments

(2 files, 6 obsolete files)

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.
Blocks: 736939
Also, this needs to build in ICS and GB. it might be easiest to just import the android code.

Phone
Attached patch Uevent poller implementation (obsolete) — Splinter Review
The poller that polls uevent from netlink socket and notifies registered observers.
Attachment #612471 - Flags: review?(jones.chris.g)
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-
(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.
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.
Uevent poller implementation v2. Updated per comment #4
Attachment #612471 - Attachment is obsolete: true
Attachment #613256 - Flags: review?(jones.chris.g)
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)
Component: General → Hardware Abstraction Layer (HAL)
Product: Boot2Gecko → Core
QA Contact: general → hal
Updated per review comment #12
Attachment #613256 - Attachment is obsolete: true
Attachment #613516 - Flags: review?(jones.chris.g)
Updated per comment #13
Attachment #613258 - Attachment is obsolete: true
Attachment #613517 - Flags: review?(jones.chris.g)
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+
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+
Keywords: checkin-needed
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 → ---
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+
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.
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 :)
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.)
Good enough for me. Thanks.
Flags: in-testsuite? → in-testsuite-
Blocks: 737153
Depends on: 764773
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: