1467695 - System theme is not applied in the browser when enabled while a static theme is active

Status: VERIFIED FIXED

(WebExtensions :: Themes, defect, P2)

Description

[Alexandra Moga [:LexaSV]]

Attachment: default auto.gif

[Affected versions]:
- Firefox 62.0a1(20180607220114)

[Affected Platforms]:
- Windows 10x64


[Steps to reproduce]:
1. Log in to AMO-dev
2. Go to a static theme details page and install it (i.e. https://addons-dev.allizom.org/en-US/firefox/addon/pattern-for-st-11/)
3. Go to about:addons - Themes - and look at the list of available themes
4. With the static theme still being your active theme, click on 'Enable' for a system theme (i.e. Dark)
5. Check the theme applied in the browser

[Expected results]:
The Dark theme is applied in the browser

[Actual results]:
The Default theme is applied in the browser

Notes:
Reproduced only when enabling a predefined system theme (i.e Light or Dark) while a static theme is active

Comment 1

[Andrew Swan [:aswan]]

This is due to theme code that applies an "empty" theme when a theme is uninstalled:
https://searchfox.org/mozilla-central/rev/c621276fbdd9591f52009042d959b9e19b66d49f/toolkit/components/extensions/parent/ext-theme.js#321-323

This code used to run synchronously during the switch from a static theme to a LWT, but in bug 1461146 enabling and disabling became asynchronous so this code can accidentally run after the new theme has been applied.

Jared, you originally wrote that unload() code.  Since we have the invariant that exactly one theme is always active, a theme can only be disabled when another one is being simultaneously enabled.  If we agree that the new theme just takes over and the old one doesn't have to do any cleanup, our lives will be much simpler.  The alternative is to fix the theme update plumbing to fully wait for the old theme to disable before enabling the new one, but that code is still pretty hairy as a legacy of the days of complete themes.  I'd certainly prefer the first option.  Am I overlooking some reason that wouldn't work?

Comment 2

[Kris Maglione [:kmag]]

Attachment: Bug 1467695: Wait for async disable operations to finish when calling addonChanged listeners.

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

Comment 3

[Andrew Swan [:aswan]]

Comment on attachment 8984564 [details]
Bug 1467695: Wait for async disable operations to finish when calling addonChanged listeners.

https://reviewboard.mozilla.org/r/250440/#review257656

::: toolkit/mozapps/extensions/AddonManager.jsm:1536
(Diff revision 1)
> +      if (result) {
> +        await result;
> +      }

do you need the if?  i thought await on a primitive value like null or undefined was effectively a no-op

Comment 4

[Kris Maglione [:kmag]]

Comment on attachment 8984564 [details]
Bug 1467695: Wait for async disable operations to finish when calling addonChanged listeners.

https://reviewboard.mozilla.org/r/250440/#review257656

> do you need the if?  i thought await on a primitive value like null or undefined was effectively a no-op

Calling `await` always makes a trip through the microtask queue. If the value you `await` isn't a promise, it just gets passed through `Promise.resolve` and turned into one:

    js> async function meh() { print("a"); await null; print("b"); } meh(); print("c");
    a
    c
    b

It *probably* doesn't make a difference in this case, but this way is less risky.

Comment 5

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Andrew Swan [:aswan] (on PTO until 6/25) from comment #1)
> This is due to theme code that applies an "empty" theme when a theme is
> uninstalled:
> https://searchfox.org/mozilla-central/rev/
> c621276fbdd9591f52009042d959b9e19b66d49f/toolkit/components/extensions/
> parent/ext-theme.js#321-323
> 
> This code used to run synchronously during the switch from a static theme to
> a LWT, but in bug 1461146 enabling and disabling became asynchronous so this
> code can accidentally run after the new theme has been applied.
> 
> Jared, you originally wrote that unload() code.  Since we have the invariant
> that exactly one theme is always active, a theme can only be disabled when
> another one is being simultaneously enabled.  If we agree that the new theme
> just takes over and the old one doesn't have to do any cleanup, our lives
> will be much simpler.  The alternative is to fix the theme update plumbing
> to fully wait for the old theme to disable before enabling the new one, but
> that code is still pretty hairy as a legacy of the days of complete themes. 
> I'd certainly prefer the first option.  Am I overlooking some reason that
> wouldn't work?

I'm not sure how the timing will work for themes that have background scripts. That might be the only thing to be concerned about. If we can rule that out then I think the first option would work.

Comment 6

[Mike Taylor [:miketaylr]]

Kris, is this patch in a state ready to land?

Comment 7

[Kris Maglione [:kmag]]

Yup, sorry. I have too many balls in the air right now...

Comment 8

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46e5404ad50cf39cfe8b10bbd9c45b4325d95d6e
Bug 1467695: Wait for async disable operations to finish when calling addonChanged listeners. r=aswan

Comment 9

[Andreea Pavel [:apavel]]

Backed out for failing browser chrome at browser/base/content/test/general/browser_compacttheme.js and xpcshell at toolkit/mozapps/extensions/test/xpcshell/test_LightweightThemeManager.js

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=46e5404ad50cf39cfe8b10bbd9c45b4325d95d6e

Failure logs: bc: https://treeherder.mozilla.org/logviewer.html#?job_id=185279189&repo=mozilla-inbound&lineNumber=1950
xpcshell: https://treeherder.mozilla.org/logviewer.html#?job_id=185279452&repo=mozilla-inbound&lineNumber=1877

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/efa8bcee4bdf153e9b9393e633e55c800a0f41d5

Comment 10

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c0ebdd0be6ba6f9748690d0ae5479977e2ea54bb
Bug 1467695: Wait for async disable operations to finish when calling addonChanged listeners. r=aswan

Comment 11

[Andreea Pavel [:apavel]]

Backed out for failing browser chrome at browser/components/customizableui/test/browser_1007336_lwthemes_in_customize_mode.js

Push that caused the failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=c0ebdd0be6ba6f9748690d0ae5479977e2ea54bb

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=185290884&repo=mozilla-inbound&lineNumber=2371

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2138cef6ab5bdc24abaca087c1d0c0c0a1e300

Comment 12

[Kris Maglione [:kmag]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/510b02c8eb4c30437fd06058a6d46b827f0f1d78
Bug 1467695: Wait for async disable operations to finish when calling addonChanged listeners. r=aswan

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/510b02c8eb4c

Comment 14

[Alexandra Moga [:LexaSV]]

Verified fixed with Nightly 63.0a1 (Build ID 20180629100106) Windows 10x64

Comment 15

[Alexandra Moga [:LexaSV]]

Attachment: system theme enabled.gif

Comment 16

[Pascal Chevrel:pascalc (PTO until April 26)]

Hi Kris, are you considering having this patch uplifted to 62 beta or can I change the tracking flag for Firefox 62 from affected to wontfix and let it ride the trains?

Thanks

Comment 17

[Liz Henry (:lizzard) (relman/hg->git project)]

If this is an edge case it might be better to let this ride with 63. Mike, what do you think?

Comment 18

[Mike de Boer [:mikedeboer]]

This patch looks low-risk to me and fixes a regression in 62 - so I'd take it if Andrew and/ or Kris agrees.

Comment 19

[Andrew Swan [:aswan]]

I agree this is low risk.  I'm pretty sure this patch doesn't depend on any other patches that aren't in 62, but it would be good for Kris to confirm that.

Comment 20

[Kris Maglione [:kmag]]

Comment on attachment 8984564 [details]
Bug 1467695: Wait for async disable operations to finish when calling addonChanged listeners.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1461146
[User impact if declined]: This prevents users from switching between WebExtension themes and lightweight themes such as the built-in compact and dark themes.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: None
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a pretty trivial change that just waits for async operations to finish before starting operations which conflict with them.
[String changes made/needed]: None

Comment 21

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8984564 [details]
Bug 1467695: Wait for async disable operations to finish when calling addonChanged listeners.

Fix for regression in 62, sounds low risk from several developers' judgement. Let's uplift for beta 16.

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/0fc394c42009

Comment 23

[Alexandra Moga [:LexaSV]]

Verified fixed in Beta 62.0b20 (20180823143155) with Win10x64 and macOS High Sierra 10.13.2