When FormAutofill is disabled, Preferences Spotlight is broken

VERIFIED FIXED in Firefox 61



10 months ago
10 months ago


(Reporter: david.olah, Assigned: mconley)



61 Branch
Firefox 62

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox-esr60 unaffected, firefox60 unaffected, firefox61+ verified, firefox62+ verified)



(1 attachment)



10 months ago
[Affected versions]: 
Firefox 61.0b7

[Affected platforms]:
Platforms: All

[Steps to reproduce]:

1. Open Firefox
2. Go to about:studies
3. Click on the "Update Preferences" button

[Expected result]:
- User is redirected to about:preferences#privacy and the page is autoscrolled to "Firefox Data Collection and Use" section 

[Actual result]:
- user is redirected to about:preferences#privacy but the page remains on the top of the page

[Video to reproduce]: 

- I tried to reproduce the same issue on the first and last Nightly 61 and on the first Nightly 62 but it is not reproducing only on the Beta version 61.0b7. Regarding this, I wasn't able to make a mozregression.

Comment 1

10 months ago
Okay, I think I've hunted this down. There's a dependency on formautofill resources here: https://searchfox.org/mozilla-central/rev/5a744713370ec47969595e369fd5125f123e6d24/browser/components/preferences/in-content/preferences.js#232

But in bug 1446585, the resource URIs for the System Add-on were made to be dynamic - and the registration only occurs _after_ FormAutofill detects whether or not it should be enabled. In regions outside of the US (and not RTL), FormAutofill is disabled, meaning that the resource URIs aren't registered.

And this breaks that line in preferences, since lazy-getting the FormAutofillParent.jsm results in an import failure.
Blocks: 1446585
Keywords: regression, regressionwindow-wanted
Summary: Privacy page is not autoscrolled to "Data Collection and Use" when "Update Preferences" button is pressed → When FormAutofill is disabled, Preferences Spotlight is broken

Comment 2

10 months ago
This breaks Spotlight in most locales outside of the US. We're lucky we caught this (thanks, David!).

We have a few choices:

1) Move the registration for FormAutofill resource URIs to before the enabled check
2) Break the dependency between Preferences and FormAutofill.

(1) Returns us to the behaviour before bug 1446585. (2) fixes the issue as well, but also doesn't prevent us from making the same mistake in the future. I suspect (1) is the safest approach, and we might want to also consider moving FormAutofill into the tree as a normal component at some point, because this seems like a footgun.

Cc'ing MattN, although I know he's got his hands full with Web Payments stuff - he still might want to know about FormAutofill things.
Keywords: regressionwindow-wanted
Priority: -- → P1

Comment 3

10 months ago
I'll take this.
Assignee: nobody → mconley


10 months ago
tracking-firefox61: --- → ?

Comment 4

10 months ago
Test automation didn't catch this because the in-content Preferences tests force FormAutofill to be enabled:


Comment 6

10 months ago
I'm trying to determine if we ever run mochitests outside of the en-US locale on either Nightly or Beta. If so, I think we can (and should) modify the browser_spotlight.js test so that it also runs without FormAutofill enabled.

Comment 7

10 months ago
mochitest-browser tests are only ever run on en-US. I'll just split out the browser_spotlight.js tests into one specially for FormAutofill, and only enable FormAutofill for those tests instead of across all about:preferences tests.

Comment 8

10 months ago
While trying to get an automated testing story put together on this, I realized how sensitive it is, since FormAutofill in the non-Nightly case falls back to "detect" which only enables FormAutofill in certain locales. We default to running in the en-US locale, which causes FormAutofill to be enabled.

I think I'm going to punt on automated testing here. Instead, I've kicked off a discussion in bug 1449055 regarding whether or not we can absorb FormAutofill as a normal component instead of an add-on. I've also filed bug 1464886 to consider adding a runtime pref for enabling and disabling FormAutofill to make testing it easier.
status-firefox60: --- → unaffected
status-firefox62: --- → affected
status-firefox-esr52: --- → unaffected
status-firefox-esr60: --- → unaffected
tracking-firefox61: ? → +
tracking-firefox62: --- → +
Comment on attachment 8981202 [details]
Bug 1464405 - Register FormAutofill resource URIs before checking if the feature should be enabled.


Thanks. While I'm okay with this for now (especially for uplift) I really think breaking the dependency between autofill and preferences is the correct solution.
Attachment #8981202 - Flags: review?(MattN+bmo) → review+

Comment 10

10 months ago
Pushed by mconley@mozilla.com:
Register FormAutofill resource URIs before checking if the feature should be enabled. r=MattN

Comment 11

10 months ago
Last Resolved: 10 months ago
status-firefox62: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 62
Please request Beta approval on this when you get a chance.
Flags: needinfo?(mconley)

Comment 13

10 months ago
Comment on attachment 8981202 [details]
Bug 1464405 - Register FormAutofill resource URIs before checking if the feature should be enabled.

Approval Request Comment
[Feature/Bug causing the regression]:

Form Autofill, but only in locales where it's disabled by default.

[User impact if declined]:

The Preferences "Spotlight" feature (which allows links in the Firefox UI to send users to about:preferences, and then highlight the relevant parts of the page) will not work. The user can still set preferences, but they aren't automatically sent to the right scroll position on the page, and there's no highlighting.

[Is this code covered by automated tests?]:

Unfortunately no, and it's actually a bit tricky to test this case in automation due to its dependence on locale.

[Has the fix been verified in Nightly?]:

This fix only affects Beta and up (since Form Autofill is enabled by default on Nightly), so no.

[Needs manual test from QE? If yes, steps to reproduce]: 

It can't hurt. Just follow the STR in comment 0 - if you can't reproduce the problem, we're all good.

[List of other uplifts needed for the feature/fix]:


[Is the change risky?]:


[Why is the change risky/not risky?]:

We're changing how Form Autofill works slightly so that it goes back to the original resource registration behavior it has had for quite some time. That's a known good configuration.

[String changes made/needed]:

Flags: needinfo?(mconley)
Attachment #8981202 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Comment on attachment 8981202 [details]
Bug 1464405 - Register FormAutofill resource URIs before checking if the feature should be enabled.

Ensures that the preferences spotlight works whether form autofill is enabled or not. Approved for 61.0b11.
Attachment #8981202 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 15

10 months ago
status-firefox61: affected → fixed

Comment 16

10 months ago
Build ID 	20180603221731
User Agent 	Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:62.0) Gecko/20100101 Firefox/62.0

Verified as fixed on Firefox Nightly 62.0a1 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 16.04 x64.
status-firefox62: fixed → verified

Comment 17

10 months ago
Actually this issue was not reproducible on Nightly, I'll verify it on Beta once 61.0b11-candidates will be available.

Comment 18

10 months ago
Build ID: 20180604143143
User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:61.0) Gecko/20100101 Firefox/61.0

Verified as fixed on Firefox 61.0b11 on Windows 10 x 64, Windows 7 x32, Mac OS X 10.12 and Ubuntu 18.04 x64.
status-firefox61: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.