[Form Autofill] Change ProfileStorage's APIs to async version

VERIFIED FIXED in Firefox 64

Status

()

P1
normal
VERIFIED FIXED
2 years ago
5 months ago

People

(Reporter: lchang, Assigned: timdream)

Tracking

(Blocks: 1 bug)

unspecified
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 verified)

Details

(Whiteboard: [form autofill:V2][Misc.] [webpayments-reserve])

Attachments

(1 attachment)

(Reporter)

Description

2 years ago
After introducing MasterPassword to ProfileStorage, there are many chances we need to call async functions in the storage. In order to better fulfill future requests, we should refactor its APIs to async version as soon as possible.
Component: Form Manager → Form Autofill
Whiteboard: [form autofill:V2] → [form autofill:V2][Misc.]
Let's get this done (removing MasterPassword.encryptSync() and decryptSync()) so we could move to OS secure storage.
Assignee: nobody → timdream
Status: NEW → ASSIGNED
This kind of works, <sarcasm>except that the autocomplete popup didn't show up</sarcasm>.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ddab76e969daa0861fd15c93cf19e59338c8951

I guess somewhere in the code base we rely on the observe message being throw back at the tick of receiving one.
BTW, I must praise the superb test coverage here. It's good to have the confidence to tell the patch is mostly ready once tests are all passed!
The two failed tests

browser/extensions/formautofill/test/mochitest/test_basic_autocomplete_form.html

and

browser/extensions/formautofill/test/mochitest/test_creditcard_autocomplete_off.html

cannot be run standalone (|mach test <filename>|) even without the patch. I have not yet found the reason why FormAutofillParent is lazily initialized when the test calls into updateFormHistory() and why it could not lazily initialize successfully there.
Sorry, I meant formAutofillStorage.
This is better now.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=973c003a3860b8f64f0a80ffb5d60ca1f6c86d14

The problem described in comment 4 may be separate. Will file another bug if that can't be fixed together.

I'll need to understand how we tag input fields in the content process and how that related to the timing of |savedFieldNames| being sent to the content process.

(I don't think I will work on this bug today, though)
There was an indeed a test failure happens because we no longer send the state change chrome event to payment content when the observer message listener returns.

This can be fixed with a timeout in the test or another notifyObserver in paymentDialogWrapper.onAutofillStorageChange() that only the test will be listening. I am not sure which one we'll prefer.

https://hg.mozilla.org/try/rev/5b2d52215c66e92802bddeef5be22a89233a6846#l4.49

I still haven't fixed the timeout mentioned in comment 4. I now suspect it might be unrelated to the timing of formAutofillStorage but rather a problem within FormHistory or AutoCompletePopup itself, given that trying to initialize the storage and/or introduce a timeout before the test starts didn't fix the problem on Try. Will need to dig deeper...
The cause of the other failure ended up to be the same with comment 7.

There is a race between the async version of _updateSaveFieldNames() and the clean-up function of the mochitests, which eventually run in a loop like

data.guids.forEach(guid => this.formAutofillStorage.addresses.remove(guid));

I've updated the function to keep it sync. Sadly Try is closed right now so I don't know if that fixes things for sure.
Priority: P3 → P1
Whiteboard: [form autofill:V2][Misc.] → [form autofill:V2][Misc.][webpayments]
Whiteboard: [form autofill:V2][Misc.][webpayments] → [form autofill:V2][Misc.] [webpayments-reserve]
Comment on attachment 9004471 [details]
Bug 1399367 - Remove MasterPassword.{encrypt|decrypt}Sync() methods

Matthew N. [:MattN] (away until Sept. 9) has approved the revision.
Attachment #9004471 - Flags: review+

Comment 15

7 months ago
Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/56aa054d4cc0
Remove MasterPassword.{encrypt|decrypt}Sync() methods r=MattN
Backed out for failures on browser/extensions/formautofill/test/unit/test_activeStatus.js

backout: https://hg.mozilla.org/integration/autoland/rev/d7b738242b7ba029c546ee92d9c11d41f896fd31

push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=56aa054d4cc02a3c1c845d81ba8cc10f43260f05&selectedJob=197533059

failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=197533059&repo=autoland&lineNumber=2116

[task 2018-09-05T02:31:27.487Z] 02:31:27     INFO -  TEST-START | browser/extensions/formautofill/test/unit/test_activeStatus.js
[task 2018-09-05T02:31:28.563Z] 02:31:28  WARNING -  TEST-UNEXPECTED-FAIL | browser/extensions/formautofill/test/unit/test_activeStatus.js | xpcshell return code: 0
[task 2018-09-05T02:31:28.564Z] 02:31:28     INFO -  TEST-INFO took 1078ms
[task 2018-09-05T02:31:28.567Z] 02:31:28     INFO -  >>>>>>>
[task 2018-09-05T02:31:28.568Z] 02:31:28     INFO -  PID 13516 | JavaScript strict warning: /builds/worker/workspace/build/tests/xpcshell/tests/browser/extensions/formautofill/test/unit/head.js -> resource://testing-common/sinon-2.3.2.js, line 8941: ReferenceError: reference to undefined property "iso-8859-8-i"
[task 2018-09-05T02:31:28.569Z] 02:31:28     INFO -  PID 13516 | JavaScript strict warning: resource://testing-common/AddonTestUtils.jsm, line 310: ReferenceError: reference to undefined property "testScope"
[task 2018-09-05T02:31:28.570Z] 02:31:28     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "iso-8859-8-i"" {file: "/builds/worker/workspace/build/tests/xpcshell/tests/browser/extensions/formautofill/test/unit/head.js -> resource://testing-common/sinon-2.3.2.js" line: 8941}]"
[task 2018-09-05T02:31:28.571Z] 02:31:28     INFO -  "CONSOLE_MESSAGE: (warn) [JavaScript Warning: "ReferenceError: reference to undefined property "testScope"" {file: "resource://testing-common/AddonTestUtils.jsm" line: 310}]"
[task 2018-09-05T02:31:28.572Z] 02:31:28     INFO -  (xpcshell/head.js) | test MAIN run_test pending (1)
[task 2018-09-05T02:31:28.574Z] 02:31:28     INFO -  (xpcshell/head.js) | test run_next_test 0 pending (2)
[task 2018-09-05T02:31:28.575Z] 02:31:28     INFO -  (xpcshell/head.js) | test MAIN run_test finished (2)
[task 2018-09-05T02:31:28.576Z] 02:31:28     INFO -  running event loop
[task 2018-09-05T02:31:28.577Z] 02:31:28     INFO -  browser/extensions/formautofill/test/unit/test_activeStatus.js | Starting head_initialize
[task 2018-09-05T02:31:28.578Z] 02:31:28     INFO -  (xpcshell/head.js) | test head_initialize pending (2)
[task 2018-09-05T02:31:28.579Z] 02:31:28     INFO -  PID 13516 | 1536114687721	addons.manager	DEBUG	Application has been upgraded
[task 2018-09-05T02:31:28.583Z] 02:31:28     INFO -  PID 13516 | 1536114687734	addons.manager	DEBUG	Loaded provider scope for resource://gre/modules/addons/XPIProvider.jsm: ["XPIProvider", "XPIInternal"]
[task 2018-09-05T02:31:28.587Z] 02:31:28     INFO -  PID 13516 | 1536114687737	addons.manager	DEBUG	Loaded provider scope for resource://gre/modules/LightweightThemeManager.jsm: ["LightweightThemeManager"]
[task 2018-09-05T02:31:28.590Z] 02:31:28     INFO -  PID 13516 | 1536114687741	addons.manager	DEBUG	Loaded provider scope for resource://gre/modules/addons/GMPProvider.jsm
[task 2018-09-05T02:31:28.594Z] 02:31:28     INFO -  PID 13516 | 1536114687743	addons.manager	DEBUG	Loaded provider scope for resource://gre/modules/addons/PluginProvider.jsm
[task 2018-09-05T02:31:28.597Z] 02:31:28     INFO -  PID 13516 | 1536114687743	addons.manager	DEBUG	Starting provider: XPIProvider
[task 2018-09-05T02:31:28.607Z] 02:31:28     INFO -  PID 13516 | 1536114687744	addons.xpi	DEBUG	startup
[task 2018-09-05T02:31:28.610Z] 02:31:28     INFO -  PID 13516 | 1536114687744	addons.xpi	WARN	List of valid built-in add-ons could not be parsed.: [Exception... "Component returned failure code: 0xc1f30001 (NS_ERROR_NOT_INITIALIZED) [nsIXPCComponents_Utils.readUTF8URI]"  nsresult: "0xc1f30001 (NS_ERROR_NOT_INITIALIZED)"  location: "JS frame :: resource://gre/modules/addons/XPIProvider.jsm :: startup :: line 2093"  data: no] Stack trace: startup()@resource://gre/modules/addons/XPIProvider.jsm:2093
[task 2018-09-05T02:31:28.612Z] 02:31:28     INFO -  PID 13516 | callProvider()@resource://gre/modules/AddonManager.jsm:206
[task 2018-09-05T02:31:28.613Z] 02:31:28     INFO -  PID 13516 | _startProvider()@resource://gre/modules/AddonManager.jsm:654
[task 2018-09-05T02:31:28.614Z] 02:31:28     INFO -  PID 13516 | startup()@resource://gre/modules/AddonManager.jsm:807
[task 2018-09-05T02:31:28.615Z] 02:31:28     INFO -  PID 13516 | startup()@resource://gre/modules/AddonManager.jsm:2802
[task 2018-09-05T02:31:28.616Z] 02:31:28     INFO -  PID 13516 | observe()@jar:file:///builds/worker/workspace/build/application/firefox/omni.ja!/components/addonManager.js:66
[task 2018-09-05T02:31:28.617Z] 02:31:28     INFO -  PID 13516 | promiseStartupManager()@resource://testing-common/AddonTestUtils.jsm:773
Flags: needinfo?(timdream)

Comment 18

7 months ago
Pushed by tchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4b3b3a1db0df
Remove MasterPassword.{encrypt|decrypt}Sync() methods r=MattN

Comment 19

7 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/4b3b3a1db0df
Status: ASSIGNED → RESOLVED
Last Resolved: 7 months ago
status-firefox64: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Flags: qe-verify+
QA Contact: hani.yacoub
An overall sanity check on the Form Autofill feature would be good to make sure the basic behaviour all still works properly.

Comment 21

5 months ago
Hi Matt,

We made the sanity check on Form Autofill, with Web Payments turned off. We found the following bugs: Bug 1496069, Bug 1496429, Bug 1496472, that also can be reproduced on Beta, so they are not issues caused by Web Payments. Bug 1496058 is caused by selecting other countries than US, Germany or Canada. Bug 1497523 is only reproducible on Nightly 64 and it is neither caused by Web Payments, it reproduces due to the different positioning in the preferences page compared to Beta.

Please let us know if there is anything else that we need to verify and you can see the test run results by visiting this link: https://testrail.stage.mozaws.net/index.php?/plans/view/12775

Should I mark this bug as verified?
Flags: needinfo?(MattN+bmo)
Yes, thank you very much!
Flags: needinfo?(MattN+bmo)

Updated

5 months ago
Status: RESOLVED → VERIFIED
status-firefox64: fixed → verified
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.