The implementation of theme.reset has a check at https://searchfox.org/mozilla-central/rev/23f836a71cfe961373c8bd0d0219ec60a64b3c8f/toolkit/components/extensions/parent/ext-theme.js#478

Since defaultTheme is never falsey, there is never an early return, and any call to theme.reset unconditionally clears the theme of the window.

The condition should probably be changed to: "if override is set and the override is set by the current extension". This matches the documented behavior: "Resets any theme that was applied using the theme.update() method." ( https://developer.mozilla.org/en-US/docs/Mozilla/Add-ons/WebExtensions/API/theme/reset ), and also the behavior of of theme.reset before a patch broke it (https://searchfox.org/mozilla-central/diff/fef7bcd6aacbdf241adeb9502f966e90abe6b073/toolkit/components/extensions/ext-theme.js#383):

The condition should be changed to:

  let theme = windowOverrides.get( ... );
  if (!theme || theme.extension !== extension) {
    // theme.update hasn't been invoked.
    return;
  }

or, if calling theme.reset is always supposed to reset the window to the default theme:

  let theme = windowOverrides.get( ... );
  if (theme && theme.extension !== extension) {
    // Theme was set by another extension, leave it be.
    return;
  }

... and a unit test should be added to ensure that the expected behavior happens.

The priority flag is not set for this bug.
:jimm, could you have a look please?

For more information, please visit auto_nag documentation.

Marking as good-first-bug.

The first comment already shows how this bug can be fixed.

Those who are interested in working on this good-first-bug can get started with the tutorial at https://wiki.mozilla.org/WebExtensions/Contribution_Onramp

Hello.
I have a question about your guidelines.

There are two ways to modify it.

1. theme.update hasn't been invoked.
2. Theme was set by another extension, leave it be.

If I modify code as follows, Is it satisfied with the above two cases?

let theme = windowOverrides.get( ... );
if (!theme || theme.extension !== extension) {
     return;
}

If I solve the issue, should I fill out the test case on there[0]?
I'll get confirmation from you and I gonna try to solve this issue.

[0]
https://dxr.mozilla.org/mozilla-central/source/toolkit/components/extensions/test/browser/browser_ext_themes_dynamic_updates.js

Yes, that condition looks right.

I suggest to register a new test in https://searchfox.org/mozilla-central/rev/11d9c7b7fa82fdfb8ac2a8f0864e9d8d5fe2b926/toolkit/components/extensions/test/browser/browser.ini

You can look at the other tests in that directory if you need examples - https://searchfox.org/mozilla-central/search?q=theme.reset&case=false&regexp=false&path=test

I submitted patch. I think it be insufficient.
If you have a time, Let me know if there's anything wrong with a misprint or something after reviewing code.

(In reply to Myeongjun Go from comment #6)

I submitted patch.

Thanks! I've assigned the bug and reviewed the patch.

I think it be insufficient.

I think that you meant to say "sufficient". "insufficient" means that you believe that the patch has is incomplete.

Hello Rob.
I used checkin-needed keyword to apply patch. But I can't find the keyword.
Instead, I found other keywords(i.e.checkin-needed-tb) and information about the keyword.
The keyword info said that Do not use for Phabricator patches, or Firefox patches.
What proper keywords can I use to apply patches?

The checkin-needed keyword was recently removed.

Do you see a "View Stack in Lando" option in Phabricator?
And if so, are you able to land the patch from there?

Let me know if you are able to add that tag. If not, then I can land the patch for you.
I'm just curious whether new contributors can add the tag - if you can, then I will update the information on the wiki.

I entered into "View Stack in Lando" menu and clicked Privew Landing for landing.
And then I saw that
Landing is Blocked: You have insufficient permissions to land. Level 3 Commit Access is required. See the FAQ for help.

I looked for another way. But It looks the same way I've just tried [0].
So, I can't land the patch now.
Let me know if you have other workflow. I'll refer to it.

[0]
https://moz-conduit.readthedocs.io/en/latest/lando-user.html

Lando doesn't work for you, but are you able to add the "Check-in Needed" tag to Phabricator after clicking on Edit revision?

Edit revision = https://wiki.mozilla.org/File:Check-in_needed_phabricator_edit_revision.png

I added Check-in Needed tag.
If there's anything you want to check on, feel free to ask me.
Thank you.

Backed out for failing bc at browser_ext_themes_dynamic_getCurrent.js

Push with failures https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=281137925&resultStatus=pending%2Crunning%2Csuccess%2Ctestfailed%2Cbusted%2Cexception&searchStr=linux%2Cx64%2Cdebug%2Cmochitests%2Cwith%2Cfission%2Cenabled%2Ctest-linux64%2Fdebug-mochitest-browser-chrome-fis-e10s-4%2Cm-fis%28bc4%29&revision=92914b96908c2dd96a042938456728a02c19336d

Failure log https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=281137925&repo=autoland&lineNumber=45795

Backout https://hg.mozilla.org/integration/autoland/rev/28673e2781e85ebb3d54e80ad8aa81f097dad038

Adding Check-in Needed seems to have worked.
Unfortunately, the patch has been reverted ("backed out") because of test failures.

Could you look into the failing tests (run them locally) and update the patch?

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:myeongjun.ko, could you have a look please?
For more information, please visit auto_nag documentation.

Sorry for the late reply. I've been on vacation.
This test case[0] also fails in the latest version.
I think that the test case requires debugging by line.

[0] browser_ext_themes_dynamic_getCurrent.js
https://dxr.mozilla.org/mozilla-central/rev/781f53bf9c789c27bc9d788a2b435c6304f03c88/toolkit/components/extensions/test/browser/browser_ext_themes_dynamic_getCurrent.js

Shall I add a Check-in Needed tag? And should I take a look at the test case?
Or I'll look at the test case where the error occurs.

You should check why the test is failing, and update the patch.
Adding "Check-in Needed" without updating the patch would trigger the test failure again, and result in a backout.

Please look into the test failure, and feel free to ping me if you need more help with understanding the issue.

Comment on attachment 9107157 [details]
Bug 1585290 - Preserve theme from other extension upon theme.reset() r=robwu

Revision D52148 was moved to bug 1585289. Setting attachment 9107157 [details] to obsolete.

Thanks Rob.
I revised failed test code(i.e browser_ext_themes_dynamic_getCurrent.js). And I submitted patch again.
I gonna attach "Check-in Needed" tag after reviewing.

You made a mistake while updating the patch summary, which caused the patch to move to an unrelated bug. Could you update the bug number of the patch to move it back?

Meanwhile I'll review your change.

Oh. Sorry.
I lacked an explanation for patches.
I added comment :)
Feel free to tell me if you have any opinion.

Backed out for failures on Database.jsm

Backout link: https://hg.mozilla.org/integration/autoland/rev/cdd7a121a36b9963a0086dc6f2ef2ec3327f2c15
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=295094547&repo=autoland&lineNumber=2746

Also xpcshell failures: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=295096370&repo=autoland&lineNumber=3576

Relanded by the above comment.

https://hg.mozilla.org/mozilla-central/rev/504c03744391

Thanks to everyone for helping me to solve this issue.