Merge lp://qastaging/~unity-api-team/indicator-network/lp1411714-14.09 into lp://qastaging/indicator-network/14.09
Status: | Merged |
---|---|
Approved by: | Antti Kaijanmäki |
Approved revision: | 471 |
Merged at revision: | 470 |
Proposed branch: | lp://qastaging/~unity-api-team/indicator-network/lp1411714-14.09 |
Merge into: | lp://qastaging/indicator-network/14.09 |
Diff against target: |
364 lines (+76/-78) 4 files modified
src/dbus-cpp/services/ofono.h (+10/-4) src/indicator/modem.cpp (+53/-58) src/menumodel-cpp/gio-helpers/util.cpp (+12/-15) src/menumodel-cpp/gio-helpers/util.h (+1/-1) |
To merge this branch: | bzr merge lp://qastaging/~unity-api-team/indicator-network/lp1411714-14.09 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Jussi Pakkanen (community) | Approve | ||
Pete Woods (community) | Approve | ||
Review via email: mp+247407@code.qastaging.launchpad.net |
Commit message
* Guard access to ofono::Modem properties that have a
shared_ptr inside them with a mutex.
* simplify GMainLoopDispatcher
Description of the change
The crash was caused by a memory corruption caused by a threading issue.
One example http://
on frame #20 after the control leaves Modem::
the stack is unwinded which results in destructor of SimManager
to be called on frame #11, although the simmgr shared_ptr inside
update() appears to be empty (points to 0x0) and the destuctor for
the SimManager object has already been ran once.
The cause of the corruption is explained here:
http://
org::ofono:
core:
core:
Core::Property<T> defines:
private:
mutable T value;
Which in SimManager's case for example becomes:
mutable std::shared_
Core::Property<T> defines the get() and set() as:
inline virtual const T& get() const
{
if (getter)
return value;
}
inline virtual void set(const T& new_value)
{
if (value != new_value)
{
value = new_value;
if (setter)
}
}
Now, in indicator-network we have two threads running:
thread 1: worker thread for dbus-cpp
thread 2: thread that runs GMainLoop
As an example the corruption happens when thread 1 does in ofono.h:
simManager.
and at the same time thread 2 does in modem.cpp:
auto simmgr = d->m_ofonoModem
set() writes to core::Property:
---
The situation is fixed in this MP by guarding the read and write to the properties by a mutex org::ofono:
---
This MP also simplifies the locking around GMainLoopDispatch as the original code was seen as error prone regarding of locking.
There are several calls like this:
std::unique_ lock<std: :mutex> lock(m_ ofonoModem- >_lock) ; >simManager. get(); >networkRegistr ation.get( );
auto simmgr = m_ofonoModem-
auto netreg = m_ofonoModem-
lock.unlock();
That is, the caller needs to be aware of the locking procedures. This is a potential future problem waiting to happen because when new calls to this function are added, one or more of them will forget to do this.
Could we instead change the getter function to be something like this:
void get(std: :shared_ ptr<Foo> &o) { :unique_ lock<.. .> ...
// Comment saying why we do this with a link to the web page
std:
o = internal_variable;
}
In this way the get function is guaranteed to be thread safe as it can not be called incorrectly.