956332 - Reorganize Login Manager tests and update interfaces

Status: VERIFIED FIXED

(Toolkit :: Password Manager, defect)

Description

[:Paolo Amadini]

Make a preliminary update of the Login Manager tests for the work in bug 853549.

Comment 1

[:Paolo Amadini]

Attachment: The patch

I've update the interfaces to remove the obsolete method as requested, and also to remove methods that were used only by regression tests, that are now de-XPCOMified.

I've added a way to wait for initalization to be completed through the promiseInitialized method, like we do in other services. This works well in tests and production code, and is simpler to use than an observer notification.

Tests now work on the outer service instead of the storage module directly, so that we can implement changes there, and divert some data from the storage module to the Permission Manager. There is a test-only mechanism to reload the storage back-end to check for correct persistence.

The empty forceSave internal method will be implemented in JSON back-end, that conversely will not have an initWithFile method, that is now only used by the mozStorage-specific tests. The currently unused getTempFile function will also be used by the LoginImport module tests.

I've kept precise tests for legacy behavior, I think we should file separate bugs if we want to improve the behavior there. There is also a bug that I'll file for which modifyLogin allows the creation of duplicate logins that are forbidden by addLogin.

I've renewed all tests except two, one for notifications and the other for the SQLite-specific behavior, that I've only slightly modified. The new tests include several previously untested cases, as well as the cases checked by the old removed tests.

Comment 2

[Justin Dolske [:Dolske]]

Comment on attachment 8355580 [details] [diff] [review]
The patch

Review of attachment 8355580 [details] [diff] [review]:
-----------------------------------------------------------------

Haven't looked at the test changes yet, had a few questions first.

::: toolkit/components/passwordmgr/nsILoginManager.idl
@@ +28,5 @@
> +     *
> +     * @return Promise that is resolved when initialization is complete, and is
> +     *         rejected in case initialization failed.
> +     */
> +    jsval promiseInitialized();

I don't understand how these comments match up with the code.

AIUI, when the implementation does |return Promise.resolve(this.__storagePromise);|, all the caller is getting is another promise that reflects other's state, but the underlying promise isn't actually being resolved? Or am I wrong?

I think you just want |readonly attribute jsval storagePromise;|?

Or is the intent to make this actually wait (spin event loop) until the promise has been resolved?

The "but does not guarantee" part is also a bit confusing. Does this just mean that the caller gets the right data from the API but the migrated data may not yet be flushed to disk? I think that's only relevant to our own testing, so it might be better to just ignore this detail for developers, or clarify it as "Tests can call this API to ensure that any data format migrations have been written to disk", or somesuch.

::: toolkit/components/passwordmgr/nsILoginManagerStorage.idl
@@ +25,5 @@
> +     * This method may optionally return a promise that is resolved when
> +     * initialization is complete, and may be rejected in case initialization
> +     * failed.  This is used by the "promiseInitialized" method of the Login
> +     * Manager, however other methods of this interface may be called before
> +     * the promise is resolved or rejected.

The described behavior feels vague and wishy-washy. If we just require a promise to be returned, that would be simpler and have well-defined error handling.

::: toolkit/components/passwordmgr/nsLoginManager.js
@@ +224,5 @@
> +     */
> +    promiseInitialized : function () {
> +        if (!this._storage) {
> +            return Promise.reject(
> +                   new Error("Initialization of storage component failed."));

Hmm, in the past we tried hard to defer loading/initializing |_storage| until we really needed it, but that's just extra complexity and doesn't actually help anymore. Also makes this function more complicated.

Let's just kill the _storage getter, and roll that code directly into init() [removing the try/catch around the __storage.init(), too] . You're now forcing creation of it there anyway.

In other words, when Login Manager is created exactly one of two things will happen:

  A) Its init() succeeds, and it will this be guaranteed to have a non-null _storage and _storagePromise.

  B) Its init() fails when getting a _storage or calling _storage.init(), and so it throws. Thus the XPCOM creation of the service fails, and no further error-checking in login manager is needed. (This function just becomes a 1-line Promise.resolve() call).

::: toolkit/components/passwordmgr/test/unit/head.js
@@ +17,5 @@
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "DownloadPaths",
> +                                  "resource://gre/modules/DownloadPaths.jsm");

?

@@ +46,5 @@
> +// While the previous test file should have deleted all the temporary files it
> +// used, on Windows these might still be pending deletion on the physical file
> +// system.  Thus, start from a new base number every time, to make a collision
> +// with a file that is still pending deletion highly unlikely.
> +let gFileCounter = Math.floor(Math.random() * 1000000);

?

@@ +62,5 @@
> + *       after calling nsIFile.createUnique, because on Windows the delete
> + *       operation in the file system may still be pending, preventing a new
> + *       file with the same name to be created.
> + */
> +function getTempFile(aLeafName)

This isn't used.

Comment 3

[Justin Dolske [:Dolske]]

(In reply to :Paolo Amadini from comment #1)

> Tests now work on the outer service instead of the storage module directly,
> so that we can implement changes there, and divert some data from the
> storage module to the Permission Manager. There is a test-only mechanism to
> reload the storage back-end to check for correct persistence.

I don't think this is a good idea, especially if Mobile ends up using a different storage backend (bug 946857). Backend-specific tests should grab a storage module directly. 

> The empty forceSave internal method

This is really hacky. :(

Can you instead have the tests wait/listen for the existing passwordmgr-storage-changed notification? (Or, for the future, a new passwordmgr-storage-flushed, which would currently be dispatched at the same point -changed is because we're currently synchronous).

Or, turned around, have the storage module listen for a please-flush-now notification, instead of poking through layers to call a method? (And won't we need something like this anyway to ensure things flush when xpcom-shutdown fires?)

Comment 4

[:Paolo Amadini]

Attachment: Updated patch

I've simplified the initalization based on the comments above, and based on our discussion for which we are OK with breaking nsILoginManagerStorage interface compatibility.

Now the observer service is used for the storage module replacement done by tests. I've kept the nsIVariant unwrapping for the initWithFile method used by mozStorage-specific tests, because it turns out it's easier and will be only used in that module, not in the JSON one.

Comment 5

[:Paolo Amadini]

dev-doc-needed and addon-compat for changes to nsILoginManager and nsILoginManagerStorage.

Comment 6

[Marco Mucci [:MarcoM]]

Project Deferred - Being Removed From Desktop Backlog.

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 8361642 [details] [diff] [review]
Updated patch

Review of attachment 8361642 [details] [diff] [review]:
-----------------------------------------------------------------

I only skimmed through some of the tests, if there was anything you wanted specific attention on please ask.

Comment 8

[:Paolo Amadini]

https://tbpl.mozilla.org/?tree=Try&rev=55761fcb782e

Comment 9

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/7c60268706f3

Comment 10

[Ryan VanderMeulen [:RyanVM]]

Backed out for xperf regressions.
https://hg.mozilla.org/integration/fx-team/rev/83e1045e24bd

https://tbpl.mozilla.org/php/getParsedLog.php?id=37083720&tree=Fx-Team

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Also, please cancel unneeded Try jobs to avoid wasting limited infra resources.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

This also had reproducible Valgrind failures that went away on backout.
https://tbpl.mozilla.org/php/getParsedLog.php?id=37088945&tree=Fx-Team

Comment 13

[Firefox Commit Bot]

The following changesets are now in Firefox Nightly:

> 7c60268706f3 Bug 956332 - Reorganize Login Manager tests and update interfaces. r=dolske
> 83e1045e24bd Backed out changeset 7c60268706f3 (bug 956332) for xperf regressions.

Nightly Build Information:

        ID: 20140402030201
 Changeset: 4941a2ac0786109b08856738019b016a6c5a66a6
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=4941a2ac0786
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central

Download Links:

>         Linux x86: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-i686.tar.bz2
>      Linux x86_64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64.tar.bz2
> Linux x86_64 ASAN: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.linux-x86_64-asan.tar.bz2
>               Mac: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.mac.dmg
>             Win32: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win32.installer.exe
>             Win64: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-02-03-02-01-mozilla-central/firefox-31.0a1.en-US.win64-x86_64.installer.exe

Previous Nightly Build Information:

        ID: 20140401030203
 Changeset: 1417d180a1d8665b1a91b897d1cc4cc31e7980d4
   Version: 31.0a1
      TBPL: https://tbpl.mozilla.org/?rev=1417d180a1d8
       URL: https://ftp.mozilla.org/pub/mozilla.org/firefox/nightly/2014/04/2014-04-01-03-02-03-mozilla-central

Comment 14

[:Paolo Amadini]

(In reply to Justin Dolske [:Dolske] from comment #2)
> Let's just kill the _storage getter, and roll that code directly into init()
> [removing the try/catch around the __storage.init(), too] . You're now
> forcing creation of it there anyway.

It seems that creating the storage module in the "init" method caused a slight startup regression, as we access the database and the keyfiles:

https://tbpl.mozilla.org/php/getParsedLog.php?id=37083720&tree=Fx-Team

We decided to do that in order to start the asynchronous data loading sooner. If the asynchronous loading completes before a page needs to use the Logins API, then we don't need to fall back to the synchronous loading, thus improving the overall responsiveness after startup when there is a page containing form fields.

Dolske, what do you think we should do here?

Comment 15

[:Paolo Amadini]

Comment 14 was probably incorrect when it talked about startup time, because this is not what xperf is measuring. There is a different interpretation for the regression.

While previously we could start loading the saved passwords when the first page requiring that data was loaded (blocking the main thread), we now need to start loading passwords in the background to avoid triggering main-thread I/O later.

We trigger the load a few seconds after the browser starts, but since xperf is running a test that loads several pages, the I/O measurements are probably catching that operation.

Joel, do you concur with this analysis? What would be needed to update the xperf whitelist consequently? It seems to be stored in a separate repository.

I think this will occur with the JSON back-end as well, I've started a combined build to verify:

https://tbpl.mozilla.org/?tree=Try&rev=f823162229fc

Comment 16

[Joel Maher ( :jmaher ) (UTC -8)]

Thanks for looking into this, here is where the whitelist file is located at:
http://hg.mozilla.org/build/talos/file/e2dd770e2d4c/talos/xtalos/xperf_whitelist.json

I want to get a sign off from aklotz before adding stuff to that file, otherwise it is as simple as adding an entry to xperf_whitelist.json and we are good.

Comment 17

[:Paolo Amadini]

(In reply to :Paolo Amadini from comment #15)
> I think this will occur with the JSON back-end as well, I've started a
> combined build to verify:

Confirmed:

https://tbpl.mozilla.org/php/getParsedLog.php?id=37753474&tree=Try

This means we'd simply need to update the whitelist.

Comment 18

[:Paolo Amadini]

The Valgrind failure is in nsPK11Token, related to the crypto initialization change as well:

https://tbpl.mozilla.org/php/getParsedLog.php?id=37088945&tree=Fx-Team#error0

I'm not sure about how we could take this leak of 24 bytes into account.

Comment 19

[Andrew McCreight [:mccr8]]

If that's readily reproducible, you could file a PSM bug on the leak.

Comment 20

[Andrew McCreight [:mccr8]]

It sounds similar to bug 794358.

Comment 21

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to :Paolo Amadini from comment #15)
> Comment 14 was probably incorrect when it talked about startup time, because
> this is not what xperf is measuring. There is a different interpretation for
> the regression.
> 
> While previously we could start loading the saved passwords when the first
> page requiring that data was loaded (blocking the main thread), we now need
> to start loading passwords in the background to avoid triggering main-thread
> I/O later.
> 
> We trigger the load a few seconds after the browser starts, but since xperf
> is running a test that loads several pages, the I/O measurements are
> probably catching that operation.

I'd like to see a better justification for why we're loading the affected files on the main thread during startup. Can we mitigate that somehow?

Comment 22

[Justin Dolske [:Dolske]]

(In reply to :Paolo Amadini from comment #14)

> It seems that creating the storage module in the "init" method caused a
> slight startup regression, as we access the database and the keyfiles:
[...]
> Comment 14 was probably incorrect when it talked about startup time, because
> this is not what xperf is measuring. There is a different interpretation for
> the regression.

Not sure what the current state of things is given the differing comments.

Maybe we need to finally fix bug 856470 (and, specifically, the mysterious bug blocking it), so that we don't force-init login manager on startup. That would allow deferring the service until it's needed, or at least give us the option of controlling when to init it. I.E., right now we force the login manager to be created but defer the storage, we could instead defer the login manager and but force the storage creation with it.

Comment 23

[Nicholas Nethercote [inactive]]

(In reply to :Paolo Amadini from comment #18)
> The Valgrind failure is in nsPK11Token, related to the crypto initialization
> change as well:
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=37088945&tree=Fx-Team#error0
> 
> I'm not sure about how we could take this leak of 24 bytes into account.

What's the question you want me to answer?

If it's "why is it leaking", it would be better answered by somebody who understands the code.

It this leak is deliberate and you want to suppress it, there is a way to do that. The suppression details will be in the output, and you should look at the build/valgrind/*.sup files.

Comment 24

[:Paolo Amadini]

(In reply to Nicholas Nethercote [:njn] from comment #23)
> It this leak is deliberate and you want to suppress it, there is a way to do
> that. The suppression details will be in the output, and you should look at
> the build/valgrind/*.sup files.

Thanks, this was the part of the question for you!

Comment 25

[:Paolo Amadini]

(In reply to Justin Dolske [:Dolske] from comment #22)
> Not sure what the current state of things is given the differing comments.
> 
> Maybe we need to finally fix bug 856470 (and, specifically, the mysterious
> bug blocking it), so that we don't force-init login manager on startup.

I missed this bit. I now have enough information to proceed on this issue, thanks everyone for the prompt replies!

Comment 26

[:Paolo Amadini]

(In reply to Aaron Klotz [:aklotz] from comment #21)
> I'd like to see a better justification for why we're loading the affected
> files on the main thread during startup. Can we mitigate that somehow?

Landing bug 856470 would indeed allow this bug to land, since it removes the forced initialization:

https://tbpl.mozilla.org/?tree=Try&rev=f291c01e84d2

I've not executed a tryserver build to test the delayed initialization from bug 853549, which was actually ineffective due to the synchronous initialization.

Aaron, I've read the xperf test documentation on the wiki, which indicates that xperf runs the tp5 suite. However, it's not clear to me whether it measures I/O regressions during the entire test run, or on startup only:

https://wiki.mozilla.org/Buildbot/Talos/Tests#xperf

This would definitely change how the delayed initialization will affect the test results.

Comment 27

[(No longer employed by Mozilla) Aaron Klotz]

(In reply to :Paolo Amadini from comment #26)
> (In reply to Aaron Klotz [:aklotz] from comment #21)
> > I'd like to see a better justification for why we're loading the affected
> > files on the main thread during startup. Can we mitigate that somehow?
> 
> Landing bug 856470 would indeed allow this bug to land, since it removes the
> forced initialization:
> 
> https://tbpl.mozilla.org/?tree=Try&rev=f291c01e84d2

Am I interpreting this right: landing bug 856470 will eliminate the accesses are causing the xperf orange in this bug?

> Aaron, I've read the xperf test documentation on the wiki, which indicates
> that xperf runs the tp5 suite. However, it's not clear to me whether it
> measures I/O regressions during the entire test run, or on startup only:
> 
> https://wiki.mozilla.org/Buildbot/Talos/Tests#xperf
> 
> This would definitely change how the delayed initialization will affect the
> test results.

Currently only startup is in scope for the xperf test.

Comment 28

[:Paolo Amadini]

(In reply to Aaron Klotz [:aklotz] from comment #27)
> Am I interpreting this right: landing bug 856470 will eliminate the accesses
> are causing the xperf orange in this bug?

It makes it so that this bug does not cause the additional accesses.

> Currently only startup is in scope for the xperf test.

Thanks, I've updated the wiki accordingly.

Comment 29

[Jim Mathies [:jimm]]

Cleaning up some deps - both this and bug 956550 block bug 853549, which blocks e10m1.

Comment 30

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/f36bdf55548a

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/mozilla-central/rev/f36bdf55548a