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

Status: VERIFIED FIXED

(Toolkit :: Form Autofill, enhancement, P1)

Description

[Luke Chang [:lchang] (inactive)]

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.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Let's get this done (removing MasterPassword.encryptSync() and decryptSync()) so we could move to OS secure storage.

Comment 2

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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!

Comment 4

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 5

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Sorry, I meant formAutofillStorage.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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)

Comment 7

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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

Comment 8

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Bug 1399367 - Remove MasterPassword.{encrypt|decrypt}Sync() methods

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

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.

Comment 10

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7289ce465aeeefb43ca5c2ea1167e6ab8538e03

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0a7369963f61da5b585b80aeaaab89422ecea297

Comment 12

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=97c7bed16c5d97110b2e56312e3e8c68ba95dffc

Comment 13

[Matthew N. [:MattN]]

Comment on attachment 9004471 [details]
Bug 1399367 - Remove MasterPassword.{encrypt|decrypt}Sync() methods

Matthew N. [:MattN] (away until Sept. 9) has approved the revision.

Comment 14

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Rebased

https://treeherder.mozilla.org/#/jobs?repo=try&revision=250d821f0ff11a844de80313e930cf2dd0c454ad

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/56aa054d4cc0
Remove MasterPassword.{encrypt|decrypt}Sync() methods r=MattN

Comment 16

[Natalia Csoregi [:nataliaCs]]

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

Comment 17

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Sorry about that, my Try syntax didn't include xpcshell

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

https://phabricator.services.mozilla.com/D4420

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/4b3b3a1db0df
Remove MasterPassword.{encrypt|decrypt}Sync() methods r=MattN

Comment 19

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/4b3b3a1db0df

Comment 20

[Matthew N. [:MattN]]

An overall sanity check on the Form Autofill feature would be good to make sure the basic behaviour all still works properly.

Comment 21

[David Olah]

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?

Comment 22

[Matthew N. [:MattN]]

Yes, thank you very much!