Merge lp://qastaging/~unity-api-team/indicator-network/lp1411714-14.09 into lp://qastaging/indicator-network/14.09

Proposed by Antti Kaijanmäki
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
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://pastebin.ubuntu.com/9805338/

    on frame #20 after the control leaves Modem::Private::update()
    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://cppwisdom.quora.com/shared_ptr-is-almost-thread-safe

org::ofono::Interface::Modem has two core::Properties that contain shared_ptr:

    core::Property<NetworkRegistration::Ptr> networkRegistration;
    core::Property<SimManager::Ptr> simManager;

Core::Property<T> defines:

  private:
    mutable T value;

Which in SimManager's case for example becomes:

    mutable std::shared_ptr<SimManager> value;

Core::Property<T> defines the get() and set() as:

    inline virtual const T& get() const
    {
        if (getter)
            mutable_get() = getter();

        return value;
    }

    inline virtual void set(const T& new_value)
    {
        if (value != new_value)
        {
            value = new_value;

            if (setter)
                setter(value);

            signal_changed(value);
        }
    }

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.set(std::shared_ptr<SimManager>());

and at the same time thread 2 does in modem.cpp:
    auto simmgr = d->m_ofonoModem->simManager.get();

set() writes to core::Property::value in one thread while get() reads the value from another simultaneously.

---

The situation is fixed in this MP by guarding the read and write to the properties by a mutex org::ofono::Interface::Modem::_lock.

---

This MP also simplifies the locking around GMainLoopDispatch as the original code was seen as error prone regarding of locking.

To post a comment you must log in.
Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

There are several calls like this:

std::unique_lock<std::mutex> lock(m_ofonoModem->_lock);
auto simmgr = m_ofonoModem->simManager.get();
auto netreg = m_ofonoModem->networkRegistration.get();
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) {
   // Comment saying why we do this with a link to the web page
   std::unique_lock<...> ...
   o = internal_variable;
}

In this way the get function is guaranteed to be thread safe as it can not be called incorrectly.

review: Needs Fixing
Revision history for this message
Pete Woods (pete-woods) wrote :

At a purely functional level, these changes look reasonable to me.

I agree with Jussi's comment, however.

review: Approve
Revision history for this message
Jussi Pakkanen (jpakkane) wrote :

Apparently the lock thing is harder than it appears.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches