1330986 - Activity stream - There are some missing icons and not properly rendered few ones on context menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Carmen Fat [:cfat] - Ecosystem QA]

Attachment: Missing icons.png

Environment: 
Device:  Motorola Razr xt910 (Android 4.4.4);
Build: Nightly 53.0a1 (2017-01-12);

Prerequisites: Activity stream enabled.

Steps to reproduce:
1. Launch Firefox;
2. Tap on the context menu icon on a website either from Top sites area or Highlights(3 dot menu);
3. Observe the icons of the context menu's options.

Expected result:
Icons from the options are properly displayed.

Actual result:
Some of the icons are not displayed;
Few icons that are displayed are not properly rendered. 

Notes:
Please see the screenshot attached.
This also happens on landscape mode.
On Nexus 5 (Android 6.0.1) and Prestigio Grace 5x (Android 4.4.2) this is not reproducible.

Comment 1

[Sebastian Kaspari (:sebastian; :pocmo)]

ahunt: Could this be the rendering of vector drawables failing on this device? Would be a pity to go back to PNGs.

Comment 2

[Andrzej Hunt :ahunt]

Interestingly, the icons that are actually corrupted, are the same ones that get corrupted in the 6.0-specific Bug (Bug 1325929). I'd be interested in knowing if that disappears once that fix lands.

We're not the only people seeing this type of corruption, so it does seem VectorDrawable specific:
https://code.google.com/p/android/issues/detail?id=209558

I'm not quite sure why some icons are completely missing, but it's possible that if there's enough corruption in the right places, then icons can disappear (at least one person has a screenshot with a missing icon in that bug).

*if* landing Bug 1325929 fixes the issue, then I can try and perform the same cleanup on the remaining icons, otherwise I'll keep digging.

Comment 3

[Carmen Fat [:cfat] - Ecosystem QA]

Tested again on latest Nightly build 53.0a1 (2017-01-15) using Motorola Razr xt910 (Android 4.4.4) but there are still icons that are not displayed and few of those who are displayed are not properly rendered.

(In reply to Andrzej Hunt :ahunt from comment #2)
> Interestingly, the icons that are actually corrupted, are the same ones that
> get corrupted in the 6.0-specific Bug (Bug 1325929). I'd be interested in
> knowing if that disappears once that fix lands.

Not only the 3 icons from Bug 1325929 are corrupted, but all of them are.
Also, I have noticed that those 3 affected icons are more frequently displayed than the others.
Please see the following screenshot and notice that all the context menu icons have problems in rendering when displayed: http://imgur.com/a/uenm9

Comment 4

[Andrzej Hunt :ahunt]

It turns out I can reproduce aspects of this on an N4 running 4.4.4 too, but only on Nightly, and not in my own builds.

That suggests we're probably hitting the following Android bug, which only affects proguarded builds (which explains why nightly is affected, but not local builds):
https://code.google.com/p/android/issues/detail?id=209558#makechanges

There's a workaround which I'll test. There's a claim that "newer" versions of the support library will address this, but that doesn't appear to be verified anywhere - and I doubt we'll be in a position to use 25.X support libraries anytime soon regardless.

Comment 5

[Andrzej Hunt :ahunt]

(In reply to Andrzej Hunt :ahunt from comment #4)
> It turns out I can reproduce aspects of this on an N4 running 4.4.4 too, but
> only on Nightly, and not in my own builds.

And I forgot to mention what I'm seeing: I can see missing icons (the first time I opened the menu it was fine, closing and reopening results in different icons missing every time). I occasionally see corruption too (on the icons that are rendered), it's not consistent and doesn't happen every time.

Comment 6

[Andrzej Hunt :ahunt]

Attachment: Bug 1330986 - Remove all remaining VectorDrawable arc curves

See https://code.google.com/p/android/issues/detail?id=209558 . On Devices
running Android 4 and below, VectorDrawable's can be corrupted due
to overzealous proguarding. This doesn't appear to have been
fixed in the support library yet, and even if it were fixed we
still wouldn't be able to switch to the most modern support
library without significant work.

Review commit: https://reviewboard.mozilla.org/r/105090/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/105090/

Comment 7

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c5f4649d2c04

Comment 8

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8827405 [details]
Bug 1330986 - Remove all remaining VectorDrawable arc curves

https://reviewboard.mozilla.org/r/105090/#review105902

In case this works: r+ - Just read the issue on b.android.com and thought I could review the patch while I'm here. :)

Comment 9

[Andrzej Hunt :ahunt]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1809c057444f

Comment 10

[Andrzej Hunt :ahunt]

Comment on attachment 8827405 [details]
Bug 1330986 - Remove all remaining VectorDrawable arc curves

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105090/diff/1-2/

Comment 11

[Andrzej Hunt :ahunt]

My first attempt didn't work, for my second attempt I also added
' proguard "-keepattributes LocalVariableTable"'

That has resulted in the corruption disappearing and all icons working. I don't see any increase in APK size when adding the LocalVariableTable line. (In fact I see a 10kb decrease). 

What I also noticed is that the share icon didn't seem to have issues: both in nightly, and in the patched try-built versions the icon never shows any corruption, and doesn't disappear. One person on the google bug mentioned that arc paths seem to cause issues: the share icon doesn't have any, other icons do.

I was able to remove arc paths in a local test either by:
- Opening in boxy SVG, ungroup, save
- inkscape FILENAME.svg --verb=EditSelectAll --verb=SelectionSimplify --verb=FileSave

I haven't actually tested the output SVGs yet, so we can't be certain that the above process actually helps. Since the proguard changes are enough to get icons working I think we should stick with that, which saves us from having to convert new SVG icons every single time we want to add them to the app.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/17f52807a316
Override proguard config to fix VectorDrawable corruption on Android 4 r=sebastian

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/17f52807a316

Comment 14

[Oana Horvath [:ohorvath - out of office]]

Attachment: Screenshot_2017-02-02.png

The issue is still reproducible on Motorola Razr (Android 4.4.4) & Asus Transformer Pad TF300T (Android 4.2.1). 
note: Does not reproduce on first try, it will reproduce after a few activities are added to highlights.

Comment 15

[Andrzej Hunt :ahunt]

Ughh. It looks like the icons that fail still have arc curves (which I'd partially fixed in Bug 1325929).

Hopefully converting the remaining icons should fix them (I've tested repeatedly on the TF200T: the bookmark, share, and private tab icons are always rendered correctly - they also have no arc curves. The other icons occasionally fail, and do contain arc curves.)

Comment 16

[Andrzej Hunt :ahunt]

(Note to self: if the letter A or A is contained somewhere within the android:pathData string, the VectorDrawable has an arc curve.)

Comment 17

[Andrzej Hunt :ahunt]

Attachment: Bug 1330986 - Fix filename typo

Review commit: https://reviewboard.mozilla.org/r/109206/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/109206/

Comment 18

[Andrzej Hunt :ahunt]

Comment on attachment 8827405 [details]
Bug 1330986 - Remove all remaining VectorDrawable arc curves

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/105090/diff/2-3/

Comment 19

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8832968 [details]
Bug 1330986 - Fix filename typo

https://reviewboard.mozilla.org/r/109206/#review110352

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/370e3956a238
Remove all remaining VectorDrawable arc curves r=sebastian
https://hg.mozilla.org/integration/autoland/rev/2e9ca1cb6df9
Fix filename typo r=sebastian

Comment 21

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/370e3956a238
https://hg.mozilla.org/mozilla-central/rev/2e9ca1cb6df9

Comment 22

[Oana Horvath [:ohorvath - out of office]]

Verified as fixed on the latest Nightly 54 (2017-02-05).