1585290 - theme.reset clears window theme even if another extension had called theme.update

Status: RESOLVED FIXED

(WebExtensions :: Themes, defect, P5)

Description

[Rob Wu [:robwu]]

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.

Comment 1

[BugBot [:suhaib / :marco/ :calixte]]

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

For more information, please visit auto_nag documentation.

Comment 2

[Rob Wu [:robwu]]

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

Comment 3

[Myeongjun Go]

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

Comment 4

[Rob Wu [:robwu]]

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

Comment 5

[Myeongjun Go]

Attachment: Bug 1585290 - Preserve theme from other extension upon theme.reset() r=robwu

Comment 6

[Myeongjun Go]

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.

Comment 7

[Rob Wu [:robwu]]

(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.

Comment 8

[Myeongjun Go]

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?

Comment 9

[Rob Wu [:robwu]]

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?

Comment 10

[Rob Wu [:robwu]]

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.

Comment 11

[Myeongjun Go]

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

Comment 12

[Rob Wu [:robwu]]

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

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/92914b96908c
theme.reset clears window theme even if another extension had called r=robwu

Comment 14

[Myeongjun Go]

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

Comment 15

[Andreea Pavel [:apavel]]

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

Comment 16

[Rob Wu [:robwu]]

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?

Comment 17

[BugBot [:suhaib / :marco/ :calixte]]

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.

Comment 18

[Myeongjun Go]

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

Comment 19

[Myeongjun Go]

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.

Comment 20

[Rob Wu [:robwu]]

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 21

[Phabricator Automation]

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.

Comment 22

[Myeongjun Go]

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.

Comment 23

[Rob Wu [:robwu]]

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.

Comment 24

[Myeongjun Go]

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

Comment 25

[Myeongjun Go]

Attachment: 0116_browser.theme.getCurrent()_results.png

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/f9f7b5b1e6ec
Preserve theme from other extension upon theme.reset() r=robwu

Comment 27

[Narcis Beleuzu [:NarcisB]]

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

Comment 28

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/504c03744391
Preserve theme from other extension upon theme.reset() r=robwu

Comment 29

[Cosmin Sabou [:CosminS]]

Relanded by the above comment.

Comment 30

[Dorel Luca [:dluca]]

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

Comment 31

[Myeongjun Go]

Thanks to everyone for helping me to solve this issue.