Merge ~gunnarhj/chromium-browser/+git/snap-from-source:flash-fix into ~chromium-team/chromium-browser/+git/snap-from-source:stable

Proposed by Gunnar Hjalmarsson on 2019-06-14
Status: Merged
Merged at revision: 65f47e45168be9455ca02c38a3312ae460e7b3f9
Proposed branch: ~gunnarhj/chromium-browser/+git/snap-from-source:flash-fix
Merge into: ~chromium-team/chromium-browser/+git/snap-from-source:stable
Diff against target: 20 lines (+4/-3)
1 file modified
launcher/chromium.launcher (+4/-3)
Reviewer Review Type Date Requested Status
Olivier Tilloy 2019-06-14 Needs Fixing on 2019-06-17
Review via email: mp+368847@code.launchpad.net

Description of the change

This change is desired irrespective of what happens with my confinement idea. Without it Chromium unnecessarily complains about the plugin being too old.

To post a comment you must log in.
Olivier Tilloy (osomon) wrote :

Good catch Gunnar, thanks!

This looks mostly fine to me, but in practice it doesn't work because: "strings: command not found" inside confinement. We would need to add binutils-$ARCH and libbinutils to the stage packages (and we would probably want to keep only the strings binary, filtering out all the other utilities).

Also you will need to remove the double quotes around $FLASH_OPTIONS in the exec line, otherwise chromium will interpret the contents of the variable as one single parameter, and will fail to load the shared library.

review: Needs Fixing
Gunnar Hjalmarsson (gunnarhj) wrote :

Thanks for looking! Didn't realize that also strings() requires a confinement adjustment. (Why is the confinement sooo restrictive?)

If you talk to someone about modifying the confinement, please take the opportunity to ask them to fix it for adobe-flashplugin and pepperflashplugin-nonfree too. :)

As regards the quotes, are you sure? The "$@" is quoted, and it seems to allow for multiple options to be passed manually. But to be honest I was in doubt; submitted a commit which removes them.

Olivier Tilloy (osomon) wrote :

I tested with quotes, and got an error message that the shared lib wasn't recognized, the name contained the rest of the arguments, and I tested again without quotes and it worked.

Regarding the strings requirement, I think we can just drop it, grep can deal with binary data to some extent:

    grep -a -z LNX libpepflashplayer.so | cut -d ' ' -f 2 | sed -e "s/,/./g"

Can you do that change?

Thanks!

Gunnar Hjalmarsson (gunnarhj) wrote :

Ah, that was a nice trick. :) (I had simply stolen the use of strings() from the pepperflashplugin-nonfree package.)

Committed the change to this MP.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/launcher/chromium.launcher b/launcher/chromium.launcher
2index 03ba33d..2a35b7e 100755
3--- a/launcher/chromium.launcher
4+++ b/launcher/chromium.launcher
5@@ -52,11 +52,12 @@ fi
6 # distribution (https://get.adobe.com/flashplayer/) and copied there.
7 # Ref: https://launchpad.net/bugs/1771162
8 FLASHSO=$SNAP_USER_DATA/.local/lib/libpepflashplayer.so
9-if [ ! -e $FLASHSO ]; then
10- FLASHSO=
11+if [ -e $FLASHSO ]; then
12+ FLASHVERSION=$(grep -a -z LNX $FLASHSO | cut -d ' ' -f 2 | sed -e "s/,/./g")
13+ FLASH_OPTIONS="--ppapi-flash-path=$FLASHSO --ppapi-flash-version=$FLASHVERSION"
14 fi
15
16 # Custom version string for chrome://version
17 export CHROME_VERSION_EXTRA=snap
18
19-exec "$SNAP/usr/lib/chromium-browser/chrome" --no-default-browser-check --no-first-run --password-store=$PASSWORD_STORE --ppapi-flash-path=$FLASHSO "$@"
20+exec "$SNAP/usr/lib/chromium-browser/chrome" --no-default-browser-check --no-first-run --password-store=$PASSWORD_STORE $FLASH_OPTIONS "$@"

Subscribers

People subscribed via source and target branches