mozilla / addons

β˜‚ Umbrella repository for Mozilla Addons ✨
Other
125 stars 41 forks source link

Find a way to stop Android being passed updates to static themes from LWTS #6109

Closed muffinresearch closed 5 years ago

muffinresearch commented 5 years ago

When the migration from LWT to static themes happens if Android Firefox requests an update it could be update from a working LWT to a non-working static theme. (hattip to @krupa) for pointing out this case)

I've confirmed with Andrew Swan that https://bugzilla.mozilla.org/show_bug.cgi?id=1430276 is platform independent so if we serve updates to android this is likely to happen.

Since support for static themes in Android is low priority we need to investigate the easiest ways to prevent the updates.

muffinresearch commented 5 years ago

/cc @jorgev @jvillalobos for input.

jvillalobos commented 5 years ago

I'm not jorgev here πŸ˜„

Does the theme update request include the platform? If so, we could just ignore the ones coming from Android.

muffinresearch commented 5 years ago

I'm not jorgev here πŸ˜„

Oops - good catch ☺️.

@eviljeff mentioned there is no platform as part of the request so we'd be reliant on UA parsing for this.

eviljeff commented 5 years ago

note: UA parsing comes with the usual drawbacks - it can be changed by the user/addons and to any random string. So it's likely a) some desktop users won't get the update because they'll have changed their UA to something weird and b) some android users will get an update because they'll have changed their UA to Firefox desktop's UA.

If we opt for accepting as many UAs as possible (e.g. the UA != Android's UA) then we'll do more b) damage; if we do the opposite and only accept an exact UA (e.g. the UA == Desktop's UA) then we'll leave more a) users out in the cold.

jvillalobos commented 5 years ago

I'd go with UA == Desktop's UA, since leaving users out in the cold in this case generally means being stuck with the exact same theme, just in LWT form.

eviljeff commented 5 years ago

tl;dr: I couldn't find a reliable way to identify Firefox Desktop from the user_agent string. How about just "not Android"?

Reference: https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/User-Agent/Firefox Example desktop UA: Mozilla/5.0 (Windows NT x.y; Win64; x64; rv:10.0) Gecko/20100101 Firefox/10.0 Example mobile UA: Mozilla/5.0 (Android 4.4; Mobile; rv:41.0) Gecko/41.0 Firefox/41.0

So, we can parse for known desktop OSs (Windows NT, Macintosh, X11) but if there is desktop Firefox on a non-tier1 OS we would miss it. Also some of the other non-desktop apps (Focus, Klar, Firefox for FireTV) identify as Linux in the UA, though I don't know if they supported LWT or support static themes, even if they have the same update code.

If we're okay just excluding only people with a default Android user agent then this works and is super simple:

def is_desktop_ua(user_agent):
    return 'Android' not in user_agent
kumar303 commented 5 years ago

I think "not android" would be a good approach. We detect Android browsers in a similar way:

https://github.com/mozilla/addons-frontend/blob/5932af990aa299d621e35ffc8d46e417c44199c9/src/core/utils/index.js#L87

jvillalobos commented 5 years ago

I'm okay with the "not Android" approach.

AlexandraMoga commented 5 years ago

@eviljeff I've set up my Android devices (added the required prefs in about:config) and I've installed some lw themes. In parallel, I've installed the same themes on desktop to make sure that they are receiving updates.

If there is no other specific prerequisite, you can try to migrate:

ID 8390522 - https://addons-dev.allizom.org/en-US/firefox/addon/firefox-galaxy/ ID 184305 - https://addons-dev.allizom.org/en-US/firefox/addon/animated-snoopy-nap/ ID 385587 - https://addons-dev.allizom.org/en-US/firefox/addon/flag-of-co-385587-2/

eviljeff commented 5 years ago

@AlexandraMoga I migrated the last two (184305, 385587) - the first one isn't a valid addon id. (Is two enough?)

AlexandraMoga commented 5 years ago

@eviljeff could you please migrate the following themes too:

ID 17245 - https://addons-dev.allizom.org/en-US/firefox/addon/killer-tomatoes/?src=search ID 382627 - https://addons-dev.allizom.org/en-US/firefox/addon/flag-of-mexico/?src=search ID 25542 - https://addons-dev.allizom.org/en-US/firefox/addon/summer-fractal/?src=search

After the first test, things seem to be working, but I like to double check.

After the migration, when I open Firefox or Nightly on Android, there is no theme displayed. I suppose this is the expected behavior:

image

eviljeff commented 5 years ago

@AlexandraMoga No, I don't think that's right - The issue is that Android users shouldn't get an update applied so they keep the LWT as before. If, after the migration and triggering an update, the theme goes away and they just get a grey default UI then the patch isn't working as expected.

AlexandraMoga commented 5 years ago

@eviljeff I understand now. I'm confirming again that after the migration, when I restart Firefox on Android (I have set a pref that takes care of the updates in the background), the lwt installed is no longer applied in the broeser.

If I go to about:addons, I can see that the theme is disabled. However, if I re-enable the theme, it will be applied in the browser.

lwt enable

AlexandraMoga commented 5 years ago

The preferences set in about:config in order to test this are:

extensions.webapi.testing = true xpinstall.signatures.required = false xpinstall.signatures.dev-root = true extensions.update.url = pointed to -dev extensions.update.background.url = pointed to -dev extensions.autoupdate.interval = 1 (to allow background updates to happen immediately)

Tested on an Android 8.0.0 device with, FF63, and Nightly 65. UA: Mozilla/5.0 (Android 8.0.0; Mobile; rv:63.0) Gecko/63.0 Firefox/63.0 Mozilla/5.0 (Android 8.0.0; Mobile; rv:65.0) Gecko/65.0 Firefox/65.0

eviljeff commented 5 years ago

Current status:

Short of finding the cause of the problem found during testing and fixing it, the options are:

See mozilla/addons#6170 for re-enabling support for themes on Android. Might be as early as the December release if they can uplift; might be next year.

@jvillalobos thoughts?

jvillalobos commented 5 years ago

Given the recent developments in Android support, I like the option of disabling automatic updates until that's done. I don't want to hold off on this migration for so long because it's not clear what the plan for Q1 will look like.

eviljeff commented 5 years ago

QA: Now there shouldn't be any updates served for LWT that have been migrated (on Desktop or Android). But LWT shouldn't be disabled on the user's browser after they've been migrated either.

AlexandraMoga commented 5 years ago

@eviljeff can you migrate the following themes please:

ID - 171543 - https://addons-dev.allizom.org/en-US/firefox/addon/stylus-blue/ ID - 115937 - https://addons-dev.allizom.org/en-US/firefox/addon/galaxy-blue/ ID - 374018 - https://addons-dev.allizom.org/en-US/firefox/addon/blue-arts/

eviljeff commented 5 years ago

@AlexandraMoga done

AlexandraMoga commented 5 years ago

@eviljeff on a first check, things are looking good this time. I will post the full test results once I've finished.

AlexandraMoga commented 5 years ago

This is now verified fixed on AMO -dev with FF63 and Nightly 65 on Win10x64 and Android 8.0