StateSaver is not working on devices

Bug #1363112 reported by Bill Filler on 2014-08-29
22
This bug affects 3 people
Affects Status Importance Assigned to Milestone
Ubuntu UI Toolkit
Fix Released
Critical
Zsombor Egri
apparmor-easyprof-ubuntu (Ubuntu)
High
Jamie Strandboge
camera-app (Ubuntu)
High
Jamie Strandboge
ubuntu-ui-toolkit (Ubuntu)
Undecided
Unassigned
Vivid
Undecided
Unassigned

Bug Description

build 212
The StateSaver is not working anymore on devices, so apps saved state is no longer restored when they are relaunched after being killed by oom killer (or manually via kill -2)

Related branches

Bill Filler (bfiller) on 2014-08-29
Changed in ubuntu-ui-toolkit (Ubuntu):
importance: Undecided → Critical
Changed in ubuntu-ui-toolkit:
importance: Undecided → Critical
tags: added: rtm14
Bill Filler (bfiller) wrote :

We tested on camera-app and gallery-app (both are click) and neither working anymore

Zsombor Egri (zsombi) on 2014-08-29
Changed in ubuntu-ui-toolkit (Ubuntu):
assignee: nobody → Zsombor Egri (zsombi)
Changed in ubuntu-ui-toolkit:
assignee: nobody → Zsombor Egri (zsombi)
Changed in ubuntu-ui-toolkit (Ubuntu):
assignee: Zsombor Egri (zsombi) → nobody
assignee: nobody → Zsombor Egri (zsombi)
Bill Filler (bfiller) wrote :

Steps to test:

- run camera-app
- swipe up from bottom to reveal controls
- turn on flash and turn on HDR
- from a terminal, log in and find pid of camera-app, then do "kill -2 <pid>"
- camera-app should die
- relaunch camera-app

Expected results:
- when camera is started the flash should be enabled and HDR setting should be enabled

Actual results:
- flash is not enabled and HDR is not enabled

no longer affects: ubuntu-ui-toolkit (Ubuntu)
Zsombor Egri (zsombi) wrote :

I've tested this with 212, and the app saves the state if it is killed while in foreground. The design we discussed back in Orlando sprint (2014 spring time) was that if app lifecycle manager will need to kill the app, then it will wake it up first and send the SIGTERM signal, so app can save the state. This seems not coming to the app.

Pastebin for the state data: http://pastebin.ubuntu.com/8179492/

If the app is killed while in background, its PID will only be removed once it gets waken up.

Zsombor Egri (zsombi) wrote :

I can confirm, with r213 flashed anew, the state file doesn't get saved at all, anywhere.

Changed in ubuntu-ui-toolkit:
status: New → Confirmed
Zsombor Egri (zsombi) on 2014-09-02
Changed in ubuntu-ui-toolkit:
status: Confirmed → In Progress
Zsombor Egri (zsombi) wrote :

There is one thing that needs to be fixed in camera app: the application name.

Because camera app doesn't use the MainView the app name is not set by the QML documents. However the application name is set in main.cpp to "camera". But, the application ID for app armor seams to be com.ubuntu.camera, so the confinement folder in XDG_RUNTIME_DIR will be /run/user/32011/confined/com.ubuntu.camera, which means the app won't be able to save the state file under /run/user/32011/confined/camera.

The branch has been checked with UITK gallery, and it works.

Zsombor Egri (zsombi) wrote :

The previous implementation worked, because originally we were using TMP folder for storing the app state data, and that used to be XDG_RUNTIME_DIR/confined/<appname>. But that had security issues as the tmp folder for the app on desktop is /tmp, which is publicly available. The same structure should be applied on desktops as on phone, i.e. under the runtime dir.

Checking the app launcher bugs I found this one: https://bugs.launchpad.net/ubuntu-app-launch/+bug/1276658

In here you can see that the TMP folder is created with the package name, which is "com.ubuntu.camera" for the Camera app:
http://bazaar.launchpad.net/~indicator-applet-developers/ubuntu-app-launch/trunk.14.10/view/head:/helpers.c#L512

So not setting the app name correctly won't let state file to be saved properly.

You can use the .deb packages I made from the MR above to test camera and gallery apps: https://launchpad.net/~ubuntu-sdk-team/+archive/ubuntu/testing/+build/6315632

Florian Boucault (fboucault) wrote :

I'll fix the camera app's name but I think we should really document that clearly at the very beginning of the StateSaver documentation.

tags: added: touch-2014-09-04
Zsombor Egri (zsombi) wrote :

Florian, I agree, I'll add this to the documentation!

Zsombor Egri (zsombi) wrote :

Update: seems XDG_RUNTIME_DIR/<APP_PKGNAME> folder wasn't added to the policy, so the reason why we couldn't save the state. The policy is being added as we speak, so hopefully we will get it soon.

Jamie Strandboge (jdstrand) wrote :

Note, I have never seen a denial against XDG_RUNTIME_DIR/<APP_PKGNAME> which is why this hasn't come up prior to now. This path will be allowed in the next version of apparmor-easyprof-ubuntu.

Changed in camera-app (Ubuntu):
status: New → In Progress
assignee: nobody → Jamie Strandboge (jdstrand)
Launchpad Janitor (janitor) wrote :

Status changed to 'Confirmed' because the bug affects multiple users.

Changed in apparmor-easyprof-ubuntu (Ubuntu):
status: New → Confirmed
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package camera-app - 3.0.0+14.10.20140903-0ubuntu1

---------------
camera-app (3.0.0+14.10.20140903-0ubuntu1) utopic; urgency=low

  [ Michał Sawicz ]
  * Add :any to python dependency and install the camera app module in
    app-private libdir.

  [ Ugo Riboni ]
  * Add translations inside the .desktop file (LP: #1318008)

  [ Florian Boucault ]
  * Disable mode switching while recording videos.
  * Disable camera switching while recording videos. (LP: #1358302)
  * Photo roll: more fine grained media folder monitoring as to properly
    forward deletions to the view. Workarounded a couple of Qt bugs
    related to snapMode SnapOneItem. (LP: #1355407)
  * Do not allow switching to the camera roll while recording a video.
  * Rename application from 'camera' to 'com.ubuntu.camera' to make
    state saving work again. Seamlessly transition ~/Pictures/camera and
    ~/Videos/camera to new locations. (LP: #1363112)
  * Remove the X-Ubuntu-Gettext-Domain from the .desktop file, which
    causes the click scope not to load the inline translations. (LP:
    #1325626)
  * Align snapshot with viewfinder as to assure visual continuity in the
    transition from one to the other. (LP: #1362822)
  * Added cancel button in content export mode. (LP: #1362823)
 -- Ubuntu daily release <email address hidden> Wed, 03 Sep 2014 18:52:44 +0000

Changed in camera-app (Ubuntu):
status: In Progress → Fix Released
Bill Filler (bfiller) on 2014-09-05
Changed in camera-app (Ubuntu):
importance: Undecided → High
Changed in apparmor-easyprof-ubuntu (Ubuntu):
importance: Undecided → High
assignee: nobody → Jamie Strandboge (jdstrand)
Changed in apparmor-easyprof-ubuntu (Ubuntu):
status: Confirmed → In Progress
tags: added: touch-2014-09-11
removed: touch-2014-09-04
Launchpad Janitor (janitor) wrote :

This bug was fixed in the package apparmor-easyprof-ubuntu - 1.2.22

---------------
apparmor-easyprof-ubuntu (1.2.22) utopic; urgency=medium

  * Updates for abstract and anonymous socket mediation (LP: #1362199):
    - ubuntu/*/ubuntu-*:
      + use dbus-strict and dbus-session-strict abstractions and remove
        duplicated policy
      + allow ubuntu-sdk and ubuntu-webapp connect, receive and send on the
        maliit abstract socket
      + allow write access to owner /{,var/}run/user/*/@{APP_PKGNAME}/{,**}
    - ubuntu/*/unconfined: allow unix
    - ubuntu/webview:
      + allow oxide to talk to sandbox via unix sockets
      + allow sandbox to talk to @{APP_PKGNAME}_@{APP_APPNAME}_@{APP_VERSION}
        peer
      + allow various unix perms from base abstract for the sandbox to use
        unix sockets
    - debian/control: Depends on apparmor >= 2.8.96~2541-0ubuntu4
  * ubuntu/webview: use @{APP_PKGNAME}_@{APP_APPNAME}_@{APP_VERSION} for
    signal now that we have @{APP_APPNAME} available (LP: #1363112)
  * ubuntu/debug: 'audit deny @{HOME}/.local/share/ r' which is used by the
    SDK to see if confined
  * debian/control: Depends on apparmor >= 2.8.96~2541-0ubuntu4~
 -- Jamie Strandboge <email address hidden> Fri, 05 Sep 2014 15:17:07 -0500

Changed in apparmor-easyprof-ubuntu (Ubuntu):
status: In Progress → Fix Released
Bill Filler (bfiller) on 2014-09-18
tags: added: touch-2014-09-25
removed: touch-2014-09-11
PS Jenkins bot (ps-jenkins) wrote :

Fix committed into lp:ubuntu-ui-toolkit/staging at revision None, scheduled for release in ubuntu-ui-toolkit, milestone Unknown

Changed in ubuntu-ui-toolkit:
status: In Progress → Fix Committed
Zoltan Balogh (bzoltan) on 2014-10-01
Changed in ubuntu-ui-toolkit:
milestone: none → 10.09
Bill Filler (bfiller) wrote :

using rtm proposed build 75 (latest on krillin)
I am still having the same problems as before as reported in comment #2

Bill Filler (bfiller) on 2014-10-01
Changed in ubuntu-ui-toolkit:
status: Fix Committed → Confirmed
Zsombor Egri (zsombi) wrote :

Seems QStandardPath has some problems with XDG_RUNTIME_DIR, investigation continues...

Zsombor Egri (zsombi) wrote :

I can confirm that QStandardPaths::writableLocation() is the main cause this and bug #1359831 is happening. The method checks whether the path specified is writable, and returns an empty string if cannot chmod +w on the path. The log we captured yesterday was not visible so far, therefore we diid not suspected that. Previous tests with the branch were working on device with any arbitrary sample app.

Adding some logs to StateSaver resulted in the following:
QStandardPaths: wrong permissions on runtime directory /run/user/32011
[StateSaver] PATH "/com.ubuntu.camera/statesaver.appstate"

Which means that the state file was about to be created in the root folder due to QStandardPaths::writableLocation() returned empty string.

Zsombor Egri (zsombi) wrote :

So, an iteration gives me the following log:
QStandardPaths: wrong permissions on runtime directory /run/user/32011
[StateSaver] No XDG_RUNTIME_DIR path set, cannot create appstate file.

The code prints this out is:
    QStringList runtimeDirs = QStandardPaths::standardLocations(QStandardPaths::RuntimeLocation);
    QString runtimeDir = runtimeDirs.size() > 0 ? runtimeDirs[0] : qgetenv("XDG_RUNTIME_DIR");
    if (runtimeDir.isEmpty()) {
        qWarning() << "[StateSaver] No XDG_RUNTIME_DIR path set, cannot create appstate file.";
        return;
    }

Which means that runtimeDirs string list is empty. If I do change the code to
    QStringList runtimeDirs = QStandardPaths::standardLocations(QStandardPaths::RuntimeLocation);
    QString runtimeDir = (runtimeDirs.size() > 0 && !runtimeDirs[0].isEmpty()) ? runtimeDirs[0] : qgetenv("XDG_RUNTIME_DIR");
    if (runtimeDir.isEmpty()) {
        qWarning() << "[StateSaver] No XDG_RUNTIME_DIR path set, cannot create appstate file.";
        return;
    }

then I get the path correctly, because qgetenv() will be used to fetch the path. So, all in all seems QStandardPaths is just not good for this folder :/

Zsombor Egri (zsombi) wrote :

Bill, I have ARM packages for the attached toolkit branch, could you also test with it? Camera app works for me, I can see the states restored (again) and also the state file created.

https://launchpad.net/~ubuntu-sdk-team/+archive/ubuntu/testing/+build/6427230

Zsombor Egri (zsombi) wrote :

Jamie, I tried to add the following code to log the file permissions:

    QStringList runtimeDirs = QStandardPaths::standardLocations(QStandardPaths::RuntimeLocation);
    QString runtimeDir = (runtimeDirs.size() > 0 && !runtimeDirs[0].isEmpty()) ? runtimeDirs[0] : qgetenv("XDG_RUNTIME_DIR");
    if (runtimeDir.isEmpty()) {
        qWarning() << "[StateSaver] No XDG_RUNTIME_DIR path set, cannot create appstate file.";
        return;
    }
    QString path = QString("%1/%2/statesaver.appstate").arg(runtimeDir).arg(applicationName);
    qDebug() << "[StateSaver] PATH" << path;

    // log permissions
    QFile::Permissions wanted = QFile::ReadUser | QFile::WriteUser | QFile::ExeUser;
    QFile xdg(runtimeDir);
    qDebug() << QString("[StateSaver] PERMISSIONS %1 WANTED %2").arg(xdg.permissions(), 0, 16).arg(wanted, 0, 16);

And I'm getting the following result:
QStandardPaths: wrong permissions on runtime directory /run/user/32011
[StateSaver] PATH "/run/user/32011/com.ubuntu.camera/statesaver.appstate"
"[StateSaver] PERMISSIONS 7700 WANTED 700"

So, QFile::permissions() return 0x7700 for XDG_RUNTIME_DIR, which is weird!

Jamie Strandboge (jdstrand) wrote :

I'm guessing with the logging you are not doing the same comparison. xdg.permissions() and wanted don't have the same number of digits. I suggest looking at http://qt-project.org/doc/qt-4.8/qfile.html and reading the code for QFile.permissions() to make sure your logging is doing exactly the same thing.

Zsombor Egri (zsombi) wrote :

Exactly, QFile.permissions() returns Qt-format permissions flags, which should not be mixed with UNIX permissions. What I printed out is the same QStandardPaths is having, and both are in Qt format. It's a coincidence that User RWX is the same as UNIX flags :)

I filed a bug for QStandardPaths, meanwhile we can have the workaround in the last MR.

Bill Filler (bfiller) wrote :

I tested the debs from comment 19 and all working as expected!

PS Jenkins bot (ps-jenkins) wrote :

Fix committed into lp:ubuntu-ui-toolkit/staging at revision None, scheduled for release in ubuntu-ui-toolkit, milestone Unknown

Changed in ubuntu-ui-toolkit:
status: Confirmed → Fix Committed
Tim Peeters (tpeeters) on 2014-10-24
Changed in ubuntu-ui-toolkit:
status: Fix Committed → Fix Released
Changed in ubuntu-ui-toolkit (Ubuntu):
status: New → Fix Released
Changed in ubuntu-ui-toolkit (Ubuntu Vivid):
status: New → Fix Released
To post a comment you must log in.
This report contains Public information  Edit
Everyone can see this information.

Duplicates of this bug

Other bug subscribers