https://wiki.mozilla.org/Mobile/Projects/Java-side_replacement_for_nsILoginManager Engineering summary: Gecko shouldn't be storing passwords on Android, because doing so requires us to be using sqlite, and that conflicts with Bug 853549 and future improvements. See the project page (and Bug 853549 Comment 45 onwards) for more discussion. This might turn into a meta bug.

I really don't want to do this. I have no desire to keep re-implementing platform features on Android, and would rather we found a db format that allowed us concurrent access. We don't have the man power to constantly re-invent the wheel for every feature. But to be a bit more specific, we would likely be implementing nsILoginManagerStorage for Android: http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/nsILoginManagerStorage.idl The project page makes that clearer as well. We can (pretty easily?) continue to use Desktop's old sqlite version until we've figured something out. I don't think anyone else is blocked on us. We're just missing out on all the improvements/bug fixes they're making. And we're paying a maintenance cost for two implementations of nsILoginManagerStorage. Both of those minuses stay in place even if we write an Android implementation.

I understand your feeling on this. (In reply to Wesley Johnston (:wesj) from comment #1) > I really don't want to do this. I have no desire to keep re-implementing > platform features on Android, and would rather we found a db format that > allowed us concurrent access. We don't have the man power to constantly > re-invent the wheel for every feature. I'd totally agree with you if we were talking about something like password auto-fill. But this isn't really a feature. It's just storage, and the interface exists precisely to allow swappable storage. People already reimplement this interface in add-ons (e.g., for the Mac Keychain). Maybe this is a different way of looking at this: perhaps the path of least resistance -- the "DB format that allows us concurrent access" -- is to establish the pattern of reimplementing storage interfaces on top of Android primitives, rather than constantly pushing to get access to bits of Gecko from outside Gecko? After all, the thing that's always running is Java-land, and it has both relational and file-based storage, with the joy of persistent caches and real threads, and syncing for free! The alternative is that we (continue to) maintain custom CPs that open the same databases (expensive, fragile, b0rked in the case of Gecko-side caching), or have to do a similar thing with some kind of files-and-locks system. What we'd be doing here is implementing a storage interface on top of a Java CP, instead of on top of a DB -- much like we do for history access from Gecko, but in a less ad hoc way. That seems easier than the CP we have now, no? > And we're paying a maintenance cost for two implementations of > nsILoginManagerStorage. Both of those minuses stay in place even if we write > an Android implementation. We're kinda already paying that price by maintaining the content provider. We have two different systems accessing the same storage; that's going to come with a price.

A replacement nsILoginManagerStorage doesn't sound too bad, or at least seems like a plausible least-worst solution. It's fairly basic code and hasn't changed much, other than stuff to deal with issues around sqlite. Of course the downside of having a separate implementation is that Android wouldn't benefit from mainline improvements, and you may have to deal with your own performance issues. (Such as the exact one bug 853549 is trying to fix.) [Alternatively, it might make sense to split the Android storage at a lower level, eg by replacing 853549's LoginStore.jsm somehow?] One passing thought would be fix the concurrency issue by having the CP delegate to Gecko when it's running (and deal with the JSON itself when Gecko isn't). But that's probably just the worst of both worlds, and has some ugly extra edge cases.

(In reply to Justin Dolske [:Dolske] from comment #3) > A replacement nsILoginManagerStorage doesn't sound too bad, or at least > seems like a plausible least-worst solution. It's fairly basic code and > hasn't changed much, other than stuff to deal with issues around sqlite. Of > course the downside of having a separate implementation is that Android > wouldn't benefit from mainline improvements, and you may have to deal with > your own performance issues. (Such as the exact one bug 853549 is trying to > fix.) I agree. An Android impl of nsILoginManagerStorage might be OK. Also note that in Firefox for Android, the current nsILoginManagerStorage is not on the main-thread, it's in a background thread. It doesn't hurt the UI, but could still cause Gecko to stall.

I whiteboarded this at Whistler. Very happy to mentor; this is going to catch up with us soon.

Barbara/Ally, perhaps this should be tracked as part of upcoming login manager work. This could be a good time to pay down some technical debt.

+1 I added it to Aha under the Login improvements.

That's certainly worth considering, trade off would mean very few user visible features this round.

+1 from me. This is not an easy project, but it's a valuable one. Bug 853549 split the backend implementations into JSON (Desktop) and SQLite (Android only). We'd follow that model and write some JS <-> Java messaging, or better yet -- JNI, to talk to a regular Android ContentProvider that could initially be backed by the existing SQLite database. Later, we can swap out the passwords ContentProvider store with a lighter-weight interface.

Here's a more thorough breakdown of this. There are two possible directions: SQL-first and JSON-first. SQL-first is approximately Comment 10. JSON-first is: 1. (a) Implement a JSON storage layer behind PasswordsProvider, stripping out all of the SQLiteBridge code and NSS involvement in that. The provider must be thread-safe. Sebastian recently did a bit of thread-safe-JSON-storage for his font downloading work; check in with him. This will leave Sync pointing at that storage and Gecko pointing at the legacy SQLite DB. This is pretty easy to test and nicely self contained, and Sync's tests should run against it and pass. 1. (b) Crypto: use javax.crypto; we have plenty of examples in the tree. You can do much better than the one round of PBKDF2 that we use right now. Get sec review. 1. (c) Eliminate the process affinity from the manifest. This immediately saves us a big chunk of memory and makes everything else faster. 2. (a) Implement the nsILoginManagerStorage interface in Gecko, talking over the bridge or JNI (talk to Jim Chen) to PasswordsProvider. This gets Gecko using our storage. Remove the SQLite nsILMS from the build, and perhaps from the tree -- desktop doesn't use it. We can run tests against this, too. 2. (b) Make sure that about:logins and other UI stuff hasn't regressed in functionality or performance. At this point we're feature complete for most users. 3. Pure-Java Master Password. We should already have all the strings we need, and this is relatively straightforward. 4. Rev the storage layer in (1) to tackle Bug 1098501, and otherwise reap the benefits.

From bug 1213688 comment 9, the benefits of fixing this bug would be: * Syncing of passwords (ignoring MP altogether) would actually function. Right now it seems to be pretty broken even for non-MP users. * Much better starting point to implement Bug 1098501, which would bring password conflict resolution up to parity with iOS. * Better perf: no need for IPC, eliminate sqlite for storage of passwords. * Better memory footprint: no need for a second Android process. This will be at least 8MB, could be *huge*. * More secure password storage: we can discard the limitations of the existing login manager storage. * Password storage code that's actually maintained: the sqlite nsILoginManagerStorage implementation is deprecated and only used by Fennec. * Opportunity to use better protection technologies -- master password equivalent, or something more usable.

Bug 946857: Part 1 : Android Login provider r?nalexander,rnewman This is WIP for Sql first approach. Yet to do : * migrate moz_disabled-hosts * doCrypto

Comment on attachment 8694989 [details] MozReview Request: Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander https://reviewboard.mozilla.org/r/26945/#review24361 ::: mobile/android/base/AndroidManifest.xml.in:467 (Diff revision 1) > + <provider android:name="org.mozilla.gecko.db.LoginProvider" Let's not `export` this. In fact, we should add a pre: patch to not `export` any of these. ::: mobile/android/base/db/BrowserContract.java:492 (Diff revision 1) > + public static final class Login implements CommonColumns { nit: the pattern is to use plurals, so `Logins`, `DeletedLogins`, etc. ::: mobile/android/base/db/BrowserContract.java:498 (Diff revision 1) > + public static final String TABLE_LOGIN = "moz_Login"; Let's keep the table names as they were: `moz_logins`, `moz_deleted_logins`, `moz_disabled_hosts`. https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/db/PasswordsProvider.java#30 ::: mobile/android/base/db/BrowserContract.java:505 (Diff revision 1) > + public static final String ENCRYPTED_USERNAME = "encryptedUsername"; This seems mis-leading. I'll have to read to get a better understanding. ::: mobile/android/base/db/BrowserContract.java:514 (Diff revision 1) > + // This needs to be kept in sync with the types defined in toolkit/components/passwordmgr/nsILoginManagerCrypto.idl#45 I think this is no longer needed. (It's definitely not going to be accurate. Right?) ::: mobile/android/base/db/BrowserContract.java:522 (Diff revision 1) > + public static final Uri CONTENT_URI = Uri.withAppendedPath(LOGIN_AUTHORITY_URI, "deleted-logins"); Let's go _ throughout. (I think - is rare.) Although now I see I'm just wrong. Sigh. https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=withAppendedPath&redirect=true ::: mobile/android/base/db/BrowserDatabaseHelper.java:54 (Diff revision 1) > + static final String TABLE_LOGIN = BrowserContract.Login.TABLE_LOGIN; nit: plurals here too. ::: mobile/android/base/db/BrowserDatabaseHelper.java:355 (Diff revision 1) > + BrowserContract.Login.GUID + " TEXT," + GUID should be NOT NULL and UNIQUE. ::: mobile/android/base/db/BrowserDatabaseHelper.java:356 (Diff revision 1) > + BrowserContract.Login.ENC_TYPE + " INTEGER, " + Should these all be `INTEGER NOT NULL`? ::: mobile/android/base/db/BrowserDatabaseHelper.java:369 (Diff revision 1) > + db.execSQL("CREATE INDEX " + LoginProvider.INDEX_LOGIN_ENC_TYPE + I assume these are from the JS storage? I find it hard to think we'll need this one. Not clear on all of the others, either. ::: mobile/android/base/db/BrowserDatabaseHelper.java:385 (Diff revision 1) > + BrowserContract.DeletedLogin.GUID + " TEXT," + My guess is we'll want an index on GUID here. And GUID should be `TEXT NOT NULL` and `UNIQUE`. ::: mobile/android/base/db/BrowserDatabaseHelper.java:386 (Diff revision 1) > + BrowserContract.DeletedLogin.TIME_DELETED + " INTEGER" + `NOT NULL`? ::: mobile/android/base/db/BrowserDatabaseHelper.java:397 (Diff revision 1) > + BrowserContract.DisabledHosts.HOSTNAME + " TEXT UNIQUE ON CONFLICT REPLACE" + `NOT NULL`? ::: mobile/android/base/db/BrowserDatabaseHelper.java:1117 (Diff revision 1) > + migrateLogin(); nit: plurals here. ::: mobile/android/base/db/BrowserDatabaseHelper.java:1125 (Diff revision 1) > + for (cursor.moveToFirst(); !cursor.isAfterLast(); cursor.moveToNext()) { Huh, does this work? I don't see this pattern elsewhere: https://dxr.mozilla.org/mozilla-central/search?tree=mozilla-central&q=cursor.moveToFirst&redirect=true ::: mobile/android/base/db/BrowserDatabaseHelper.java:1130 (Diff revision 1) > + } catch (Exception e) { We want to best effort each login, so `catch` individually. Here and below. ::: mobile/android/base/db/LoginProvider.java:31 (Diff revision 1) > + private static final int LOGIN = 100; nit: plurals. ::: mobile/android/base/db/LoginProvider.java:49 (Diff revision 1) > + protected static final String INDEX_LOGIN_HOSTNAME = "login_hostname_index"; I feel like these should be public, for use in the creation/migration code. And... of course you're using them. Never mind :) ::: mobile/android/base/db/LoginProvider.java:134 (Diff revision 1) > + if (id >= 0) nit: always include braces. ::: mobile/android/base/db/LoginProvider.java:338 (Diff revision 1) > + values.put(DeletedLogin.GUID, System.currentTimeMillis()); Careful. Tests will winkle these errors out. ::: mobile/android/base/db/LoginProvider.java:343 (Diff revision 1) > + private void setupDefaultValues(ContentValues values, Uri uri) throws IllegalArgumentException { nit: extra newline. ::: mobile/android/base/db/LoginProvider.java:363 (Diff revision 1) > + DBUtils.replaceKey(values, null, Login.HOSTNAME, ""); Hmm. In general, I don't want to have empty string defaults like this. ::: mobile/android/base/db/LoginProvider.java:370 (Diff revision 1) > + DBUtils.replaceKey(values, null, Login.ENC_TYPE, "0"); Do we have to use strings for longs? This is funky. ::: mobile/android/base/db/LoginProvider.java:387 (Diff revision 1) > + private void sanitize(final ContentValues values, final SQLiteDatabase db, final boolean isInsert) { Can we find a better name for this, and give a description of what `sanitize` and `deSanitize` do? I thought you were just going to update some fields in the `ContentValues` instance. ::: mobile/android/base/db/LoginProvider.java:388 (Diff revision 1) > + if (isInsert && values.containsKey(Login.GUID)) { Yeah, this definitely doesn't look right. This deletion should happen in a transaction with the insertion. ::: mobile/android/base/moz.build:227 (Diff revision 1) > + 'db/LoginProvider.java', nit: plural here, too -- `LoginsProvider`. ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testLoginProvider.java:27 (Diff revision 1) > +public class testLoginProvider extends ContentProviderTest { These tests are great. I'd like to build out this suite to be more comprehensive, however. (Possibly as follow-ups that you can mentor, as you see fit.) In particular, I want to see random GUID generation working; I want to see creation time getting set; and modification time getting set and updated. I'd also like to see some failure cases: inserting malformed GUIDs, deleting unknown GUID, deleting the same GUID twice, etc. This is a *solid* patch. Bravo! I have some asks for more tests, and one substantive issue (around sanitize deleting items). But after that it's just nits, which IntelliJ should make easy work. I'm impressed.

Comment on attachment 8694989 [details] MozReview Request: Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander Clearing rnewman's review for now. We'll have a few targeted questions for him.

One concern I have is that currently adding a new field to the password storage is very expensive in terms of development time, even if we don't need to index the field in the database at all. As far as I can tell from a quick scan of the code, this patch does not solve this issue, and even makes it worse by requiring writing Java code, together with SQL schema migration code. As a benchmark, imagine this patch lands. What code changes do we need to implement if want to add a synced "Notes" field to all saved passwords in Desktop and Android? (The above is just an example, we don't need a notes field right now. We've however discussed more important new fields like a boolean telling whether the password can be autofilled on HTTP sites, but since there is a very high cost to doing that, we've spent a lot of time discussing hacks to improperly reuse existing fields or even use the presence of multiple, possibly misaligned records to indicate the same information so we don't need to change the schema. The same goes for metadata that helps syncing more robustly, as far as I can remember.) Another question. Does the final design on Android still require the use of a SQL database because we need concurrent access from multiple processes? I remember at some point we discussed using IPC for the purpose, and having a single process (that is not the Gecko process) responsible for I/O. Is there a page where the final architecture is documented? It seems the following page mentioned in the bug description is out of date, in fact Desktop performance issues have been resolved already: https://wiki.mozilla.org/Mobile/Projects/Java-side_replacement_for_nsILoginManager

(In reply to :Paolo Amadini from comment #16) > One concern I have is that currently adding a new field to the password > storage is very expensive in terms of development time, even if we don't > need to index the field in the database at all. As far as I can tell from a > quick scan of the code, this patch does not solve this issue, and even makes > it worse by requiring writing Java code, together with SQL schema migration > code. This is correct, but this ticket isn't trying to make that easier. This patch makes it no harder than it already is: Gecko-side additions require SQL schema changes. I suppose you could say it's worse in that after landing we'll require an Android engineer who understands Android paradigms; but I argue that the existing situation is worst possible: we have some evidence that the store is completely broken for many users, and we don't get Desktop JS engineering support. Making that easier on Android is strictly a future project: we'd also like to use a lighter-weight store than SQL! But it's much easier for us to just transition existing SQL across the JS-Java barrier, and then switch the ContentProvider implementation (to an AtomicFile full of JSON, say), than to implmeent a a JSON store on the Gecko side and have it be accessible to non-Gecko code. (That's madness.) > As a benchmark, imagine this patch lands. What code changes do we need to > implement if want to add a synced "Notes" field to all saved passwords in > Desktop and Android? 1) Rev IDL files 2) Rev JSON Desktop backend 3) Rev Android SQL schema 4) Rev Android JS backend that interfaces to ContentProvider > (The above is just an example, we don't need a notes field right now. We've > however discussed more important new fields like a boolean telling whether > the password can be autofilled on HTTP sites, but since there is a very high > cost to doing that, we've spent a lot of time discussing hacks to improperly > reuse existing fields or even use the presence of multiple, possibly > misaligned records to indicate the same information so we don't need to > change the schema. The same goes for metadata that helps syncing more > robustly, as far as I can remember.) rnewman filed to add some of this metadata for Sync to the Android implementation, so we have it in the DB, but it hasn't happened: Bug 1098501. It's strictly easier for mobile team to arrange this if we manage the store like any other store; right now, it's a Special Snowflake. > Another question. Does the final design on Android still require the use of > a SQL database because we need concurrent access from multiple processes? I > remember at some point we discussed using IPC for the purpose, and having a > single process (that is not the Gecko process) responsible for I/O. The existing implementation requires concurrent access from Gecko and non-Gecko, and I believe that we needed a separate process to work around having two different versions of SQLite. The proposed implementation should not require a separate process. (We have data stores just liked, accessed concurrently from Gecko and non-Gecko, without issue.) > Is there a page where the final architecture is documented? It seems the > following page mentioned in the bug description is out of date, in fact > Desktop performance issues have been resolved already: No, but rnewman and I have broken this down pretty clearly in https://bugzilla.mozilla.org/show_bug.cgi?id=946857#c11. We can do more documentation here if it's necessary. As an aside, it may be true that "Desktop performance issues have been resolved already", but the synchronous API is still a significant problem. We think we have a way to handle that, but we haven't implemented it fully.

(In reply to Nick Alexander :nalexander from comment #17) Okay. Without entering into detailed discussions, I just wanted to make sure that the concern about the difficulty of updating the schema is still acknowledged and there is a path to simplify that in the future, which seems to be the case. > As an aside, it may be true that "Desktop performance issues have been > resolved already", but the synchronous API is still a significant problem. Can you elaborate on this? If the Desktop team can help with simplifying things for you it would be good to know, even though we may not be able to find more people to work on the Login Manager right now.

(In reply to Nick Alexander :nalexander from comment #17) > This is correct, but this ticket isn't trying to make that easier. This > patch makes it no harder than it already is: Gecko-side additions require > SQL schema changes. It's worth bearing in mind that the current Gecko-side SQL storage that we use is *only* used by Android, and will be deleted as soon as we stop using it; desktop uses the JSON backend. We already have two backends and effectively support one of them ourselves. > The existing implementation requires concurrent access from Gecko and > non-Gecko, and I believe that we needed a separate process to work around > having two different versions of SQLite. Not just SQLite, but also some NSS limitations, IIRC. > The proposed implementation should not require a separate process. Yup, that's part of the goal. We need to access this store from both Gecko and Android. Right now we concurrently access the DB from a separate process with NSS loaded. This is turrible. The proposal is to have the Android side store passwords in encrypted JSON; no NSS, no Gecko, no extra process, no nothin'. Then we'd touch the CP over one of our two bridges from an nsILoginManagerStorage implementation. The Java side is always around, so Gecko can talk to it whenever it wants. > As an aside, it may be true that "Desktop performance issues have been > resolved already", but the synchronous API is still a significant problem. > We think we have a way to handle that, but we haven't implemented it fully. Desktop has fixed most of its perf issues by no longer using the SQLite backend, as I understand it. Fennec still uses that backend, and we want to stop. The only sane way to stop, given the constraints, is to do what we're doing here.

(In reply to :Paolo Amadini from comment #18) > Okay. Without entering into detailed discussions, I just wanted to make sure > that the concern about the difficulty of updating the schema is still > acknowledged and there is a path to simplify that in the future, which seems > to be the case. Yup, and thank you for the attention! When we have a single component that owns storage, and switch to JSON as a backend, we should have a pretty easy time of this. Sync will remain a bottleneck for changes, but that's always the case.

(In reply to Richard Newman [:rnewman] from comment #19) > We already have two backends and effectively support one of them ourselves. Both backends have been getting the same support in that authors changing storage change both backends at the same time so I don't think this is true.

Comment on attachment 8694989 [details] MozReview Request: Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26945/diff/1-2/

Review commit: https://reviewboard.mozilla.org/r/28999/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/28999/

Review commit: https://reviewboard.mozilla.org/r/29001/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/29001/

Comment on attachment 8694989 [details] MozReview Request: Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander https://reviewboard.mozilla.org/r/26945/#review25865 Looks fine by me. A test that we can query would be nice. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:257 (Diff revision 2) > + public static final String ID = "id"; This "id" is determined by the underlying Gecko-side, right? ::: mobile/android/base/java/org/mozilla/gecko/db/PasswordsProvider.java:34 (Diff revision 2) > + static final String TABLE_DISABLED_HOSTS = "moz_disabledHosts"; Gah! Who named these things? ::: mobile/android/base/java/org/mozilla/gecko/db/PasswordsProvider.java:93 (Diff revision 2) > + DISABLED_HOSTS_PROJECTION_MAP.put(GeckoDisabledHosts.HOSTNAME, GeckoDisabledHosts.HOSTNAME); Is there a reason to not expose the underlying ID? Is your intention to drop the ID when you migrate? ::: mobile/android/base/java/org/mozilla/gecko/db/PasswordsProvider.java:232 (Diff revision 2) > + if (!values.containsKey(GeckoDisabledHosts.HOSTNAME)) { nit: "Must provide a hostname for a disabled host."

Comment on attachment 8701605 [details] MozReview Request: Bug 946857 - Part 1 : Android Login provider r?nalexander https://reviewboard.mozilla.org/r/28999/#review25867 This is looking pretty good. I'm going to mark this r+ but expect to do some testing (especially of the JNI interface) myself before we land it. Bravo! ::: mobile/android/base/AndroidManifest.xml.in:431 (Diff revision 1) > + android:permission="@ANDROID_PACKAGE_NAME@.permissions.PASSWORD_PROVIDER" I recently dropped such permissions, so this'll need a rebase. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:508 (Diff revision 1) > + public static final String TABLE_LOGINS = "moz_logins"; I know I said otherwise before, but let's just call this "logins". ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserContract.java:541 (Diff revision 1) > + public static final Uri CONTENT_URI = Uri.withAppendedPath(LOGIN_AUTHORITY_URI, "disabled-hosts"); nit: newline in between, just to keep the existing style. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:363 (Diff revision 1) > + nit: drop extra newline. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:382 (Diff revision 1) > + // Table for each disabled hosts. nit: // Table for each disabled host. ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:1183 (Diff revision 1) > + Log.e(LOGTAG, "Exception occurred while migrating moz_deleted-logins for values : " + contentValues, e); No need to print the CVs here. Important thing is to say that you're *ignoring* the exception, so make it `Log.w`, like: ``` Log.w(LOGTAG, "Ignoring exception caught while migrating deleted logins.", e); ``` ::: mobile/android/base/java/org/mozilla/gecko/db/LoginsProvider.java:331 (Diff revision 1) > + private String doCrypto(String initialValue, Boolean encrypt) { Have you tested this with a non-trivial thing, like Base64-encoding and decoding? ::: mobile/android/base/java/org/mozilla/gecko/db/LoginsProvider.java:335 (Diff revision 1) > + private void storeDeletedLoginForGUIDInTranscation(final String guid, final SQLiteDatabase db) { For these "InTransaction" methods, add JavaDoc explaining what the expected conditions are: /** * Caller is responsible for invoking this method inside a transaction. */ Also, I think you need to return something from `db.insert` (or a boolean flag) to allow the caller to know to abort the transaction.

Comment on attachment 8701606 [details] MozReview Request: Bug 946857 - part 2: JNI based password manager storage r?nalexander,jchen https://reviewboard.mozilla.org/r/29001/#review25869 ::: mobile/android/base/java/org/mozilla/gecko/db/LocalLoginsAccessor.java:81 (Diff revision 1) > + cursor = getContext().getContentResolver().query(disabledHostsUriWithProfile, null, Can we do a "LIMIT 1" here? ::: mobile/android/base/java/org/mozilla/gecko/db/LoginsAccessor.java:13 (Diff revision 1) > +public interface LoginsAccessor { This needs some class comments to explain what it's for and how it fits into your new architecture. ::: toolkit/components/passwordmgr/storage-fennecStorage.js:19 (Diff revision 1) > +function LoginManagerStorage_mozStorage() { }; `mozStorage`? Shouldn't this be `fennecStorage`? ::: toolkit/components/passwordmgr/storage-fennecStorage.js:51 (Diff revision 1) > + * Database initialization are done in Android, so return immediately. I know what you mean, but be clear that they're done "in Android before the Gecko thread starts". ::: toolkit/components/passwordmgr/storage-fennecStorage.js:65 (Diff revision 1) > + return Promise.resolve(); This seems... odd. Explain why doing nothing works. ::: toolkit/components/passwordmgr/storage-fennecStorage.js:92 (Diff revision 1) > + JNI.UnloadClasses(my_jenv); I feel like this pattern is probably wrong: you should maintain the JNI environment (since you're always on the same (main JavaScript) thread). jchen will know more. ::: toolkit/components/passwordmgr/storage-fennecStorage.js:106 (Diff revision 1) > + if (!this._isGuidUnique(loginClone.guid)) nit: Android-team JavaScript style is to always brace (with {}), no matter what. (Other teams do differently.) ::: toolkit/components/passwordmgr/storage-fennecStorage.js:163 (Diff revision 1) > + let cb = Async.makeSyncCallback(); Hmm. I wonder how this will perform in the wild. We'll have to experiment and get some perspective from some knowledgeable JS folks. ::: toolkit/components/passwordmgr/storage-fennecStorage.js:224 (Diff revision 1) > + _modifyLoginHelper(newLogin, cb); This pattern, with the helper out-of-line at the top, is a little hard to read. Can we make this a little helper, so that it reads... ``` waitForSync(() => { use newLogin, return a promise }) ``` and create the `cb`, and evaluate it automatically after the promise resolves? I can help with this clean-up. ::: toolkit/components/passwordmgr/storage-fennecStorage.js:498 (Diff revision 1) > + var jHostName = loginClone.hostname ? JNI.NewString(my_jenv, loginClone.hostname) : this._SIG.NULL; I don't really understand how the `JNI.NewString` references get garbage collected. Can you explain? ::: toolkit/components/passwordmgr/storage-fennecStorage.js:775 (Diff revision 1) > + // Allocate a local reference frame with enough space (1 String read). Should we be using this pattern more generally? vivek: I've managed to give this a first reading but not a thorough reading. I'd like to see two things: 1) improving the async callback pattern, so that the `cb` management is automatic and based on the promise resolving. (It's important that `cb` get invoked even when the promise is rejected, too.) 2) making the JNI environment management "global", rather than per-function. Get everything prepped at initialization and then tear it down when appropriate, rather than registering everything each query. Could you flag jchen for feedback here too? He knows much more than I do about this stuff. Thanks!

https://reviewboard.mozilla.org/r/26945/#review25879 ::: mobile/android/base/java/org/mozilla/gecko/db/PasswordsProvider.java:93 (Diff revision 2) > + DISABLED_HOSTS_PROJECTION_MAP.put(GeckoDisabledHosts.HOSTNAME, GeckoDisabledHosts.HOSTNAME); Yes, It was my intention to drop the ID as Android CP automatically assign an _ID. I will remove the ID field declaration from GeckoDisabledHosts in BrowserContract.

Comment on attachment 8701606 [details] MozReview Request: Bug 946857 - part 2: JNI based password manager storage r?nalexander,jchen jchen, Can you please review the JNI based password manager that I was working on.

https://reviewboard.mozilla.org/r/29001/#review25881 ::: toolkit/components/passwordmgr/storage-fennecStorage.js:498 (Diff revision 1) > + var jHostName = loginClone.hostname ? JNI.NewString(my_jenv, loginClone.hostname) : this._SIG.NULL; My bad, this is a leak. I will fix this.

Comment on attachment 8694989 [details] MozReview Request: Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26945/diff/2-3/

Comment on attachment 8701605 [details] MozReview Request: Bug 946857 - Part 1 : Android Login provider r?nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28999/diff/1-2/

Comment on attachment 8701606 [details] MozReview Request: Bug 946857 - part 2: JNI based password manager storage r?nalexander,jchen Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29001/diff/1-2/

Comment on attachment 8701606 [details] MozReview Request: Bug 946857 - part 2: JNI based password manager storage r?nalexander,jchen Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29001/diff/2-3/

Comment on attachment 8701606 [details] MozReview Request: Bug 946857 - part 2: JNI based password manager storage r?nalexander,jchen https://reviewboard.mozilla.org/r/29001/#review26051 For now, I think this approach works. JNI.jsm isn't great for doing lots of JNI work like this, so maybe eventually we should have a C++ component that simply passes all calls to Java. ::: toolkit/components/passwordmgr/storage-fennecStorage.js:110 (Diff revision 3) > + my_jenv.contents.contents.PushLocalFrame(my_jenv, 13); I see 26 NewString calls in `_populateContentValues`. Is that right? 13 for the names and 13 for the values. ::: toolkit/components/passwordmgr/storage-fennecStorage.js:601 (Diff revision 3) > + let selectionArgs = selectionParams.length ? StringArray.new(selectionParams.length) : this._SIG.NULL; You need `PushLocalFrame`/`PopLocalFrame` here because the `StringArray.new` and the `JNI.NewString` calls creates new references. ::: toolkit/components/passwordmgr/storage-fennecStorage.js:753 (Diff revision 3) > + let jHostName = hostname ? JNI.NewString(my_jenv, hostname) : this._SIG.NULL Need `PushLocalFrame`/`PopLocalFrame` here.

https://reviewboard.mozilla.org/r/29001/#review26397 ::: toolkit/components/passwordmgr/storage-fennecStorage.js:500 (Diff revision 3) > + var jHttpRealm = loginClone.httpRealm ? JNI.NewString(my_jenv, loginClone.httpRealm) : this._SIG.NULL; How should we handle empty js string? Is the approach here reasonable where empty js string are mapped to Java null. @Nick/jchen: Can you please advice me on the correct approach

Please advice on the above comment

(In reply to Vivek Balakrishnan[:vivek] from comment #39) > https://reviewboard.mozilla.org/r/29001/#review26397 > > ::: toolkit/components/passwordmgr/storage-fennecStorage.js:500 > (Diff revision 3) > > + var jHttpRealm = loginClone.httpRealm ? JNI.NewString(my_jenv, loginClone.httpRealm) : this._SIG.NULL; > > How should we handle empty js string? Is the approach here reasonable where > empty js string are mapped to Java null. In general, "it depends" -- on the input data and the underlying schema. Here, I see a helper to convert to JSON at https://dxr.mozilla.org/mozilla-central/source/addon-sdk/source/lib/sdk/passwords/utils.js#58 that treats some things that evaluate to false (like an empty string "") as null and some other things as the empty string itself. So I think your approach is reasonable.

Comment on attachment 8694989 [details] MozReview Request: Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26945/diff/3-4/

Comment on attachment 8701605 [details] MozReview Request: Bug 946857 - Part 1 : Android Login provider r?nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28999/diff/2-3/

Comment on attachment 8701606 [details] MozReview Request: Bug 946857 - part 2: JNI based password manager storage r?nalexander,jchen Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29001/diff/3-4/

Review commit: https://reviewboard.mozilla.org/r/29705/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/29705/

Comment on attachment 8701606 [details] MozReview Request: Bug 946857 - part 2: JNI based password manager storage r?nalexander,jchen https://reviewboard.mozilla.org/r/29001/#review26547 LGTM if you add the `PopLocalFrame` call back. ::: toolkit/components/passwordmgr/storage-fennecStorage.js (Diff revisions 3 - 4) > - my_jenv.contents.contents.PopLocalFrame(my_jenv, null); I think you accidentally deleted the `PopLocalFrame` call here.

Comment on attachment 8701606 [details] MozReview Request: Bug 946857 - part 2: JNI based password manager storage r?nalexander,jchen Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29001/diff/4-5/

Review commit: https://reviewboard.mozilla.org/r/29727/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/29727/

Comment on attachment 8694989 [details] MozReview Request: Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26945/diff/4-5/

* Added MOZ_ANDROID_LOGINS_STORAGE_INIT build flag to initialize the CP and create db schema * Added query param to skip login decryption Review commit: https://reviewboard.mozilla.org/r/31057/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31057/

Patch changes: Added MOZ_ANDROID_LOGINS_STORAGE_USE build flag Exposed an accessor for LoginsProvider New passwordmgr storage that uses JNI to store data through LoginsProvider CP Fixed getLoginsSavedEnabled() return type mis-match. Fixed potential memory leak in fennecStorage.js Test toolkit/components/passwordmgr changes Yet to do: Use LoginsProvider with Sync Password RepoSessions Review commit: https://reviewboard.mozilla.org/r/31059/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/31059/

Comment on attachment 8694989 [details] MozReview Request: Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26945/diff/5-6/

Comment on attachment 8708461 [details] MozReview Request: Bug 946857 - Part 2: Add Android LoginsProvider. r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31057/diff/1-2/

Comment on attachment 8708462 [details] MozReview Request: Bug 946857 - part 2: JNI based password manager storage r?nalexander,jchen Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31059/diff/1-2/

Comment on attachment 8694989 [details] MozReview Request: Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26945/diff/6-7/

Comment on attachment 8708461 [details] MozReview Request: Bug 946857 - Part 2: Add Android LoginsProvider. r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31057/diff/2-3/

Comment on attachment 8708462 [details] MozReview Request: Bug 946857 - part 2: JNI based password manager storage r?nalexander,jchen Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31059/diff/2-3/

* PasswordsProvider conditionally swapped with LoginsProvider for sync based on build flag * Fixed timing issue in passwordRepository tests. Review commit: https://reviewboard.mozilla.org/r/33833/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/33833/

Comment on attachment 8708462 [details] MozReview Request: Bug 946857 - part 2: JNI based password manager storage r?nalexander,jchen https://reviewboard.mozilla.org/r/31059/#review30825

Comment on attachment 8694989 [details] MozReview Request: Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26945/diff/7-8/

Comment on attachment 8708461 [details] MozReview Request: Bug 946857 - Part 2: Add Android LoginsProvider. r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31057/diff/3-4/

Comment on attachment 8708462 [details] MozReview Request: Bug 946857 - part 2: JNI based password manager storage r?nalexander,jchen Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31059/diff/3-4/

Comment on attachment 8716519 [details] MozReview Request: Bug 946857 - part 3: Sync password repo related changes for logins storage r?nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33833/diff/1-2/

@Nick, The patches are split as below. Pre -> Added GeckoDisabledHost related changes to query disabledHosts from sqlite (gecko). Part 1 -> Added new Android side Content Provider, this requires bumping db base version. Part 2 -> JNI and fennecStorage related changes, migrate data from gecko sqlite to android CP Part 3 -> Android sync related changes I have introduced two build "MOZ_ANDROID_LOGINS_STORAGE_INIT" and "MOZ_ANDROID_LOGINS_STORAGE_USE". The "MOZ_ANDROID_LOGINS_STORAGE_INIT" flag is introduced in patch part 1 and will be turned on by default. The "MOZ_ANDROID_LOGINS_STORAGE_USE" flag introduced in patch part 2 will be turned on later. The "MOZ_ANDROID_LOGINS_STORAGE_USE" flips the implementation for nsLoginManager and for sync passwordReposession. As discussed over irc, pre & part 1 patch will be landed first and then part 2 & part 3 changes will landed. The flags will be turned on when the respective changes are landed. The idea behind this approach is to validate that new CP does not break anything before taking it to use. Also, the login manager/sync can be reverted back to use old password provider/gecko sqlite by flipping off the "MOZ_ANDROID_LOGINS_STORAGE_USE" flag. There is a risk associated with the above approach. The logins stored after flipping "MOZ_ANDROID_LOGINS_STORAGE_USE" is not available in gecko side. In other words, passwords migration from gecko is done only once i.e when part 2 patch gets landed. Please comment, if the above approach is reasonable.

https://reviewboard.mozilla.org/r/26945/#review31279 Just a few nits here. ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testPasswordProvider.java:25 (Diff revision 8) > + * - insert a disabled host nit: "inserts", and "queries for". ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testPasswordProvider.java:73 (Diff revision 8) > + ContentValues values = new ContentValues(); nit: no newline between. ::: mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testPasswordProvider.java:77 (Diff revision 8) > + // Attempt to insert into the db nit: full sentence, so add the trailing period.

Comment on attachment 8694989 [details] MozReview Request: Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26945/diff/8-9/

Comment on attachment 8708461 [details] MozReview Request: Bug 946857 - Part 2: Add Android LoginsProvider. r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31057/diff/4-5/

Comment on attachment 8708462 [details] MozReview Request: Bug 946857 - part 2: JNI based password manager storage r?nalexander,jchen Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31059/diff/4-5/

Comment on attachment 8716519 [details] MozReview Request: Bug 946857 - part 3: Sync password repo related changes for logins storage r?nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/33833/diff/2-3/

Added a simple robocop test to verify that it is possible to query disabledHosts with PasswordProviders. Review commit: https://reviewboard.mozilla.org/r/35033/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35033/

LoginsProvider is an all-Android implementation of PasswordsProvider. PasswordsProvider is an SQLite database backed by the version of SQLite that ships with Gecko. It is concurrently accessed from Gecko and it runs with a special lifecycle that includes a separate heavy-weight process. Eventually we'll migrate the Gecko-side passwords interface to use the new Android-side LoginsProvider, but for now we just want to get the new provider landed and the tests running. Review commit: https://reviewboard.mozilla.org/r/35035/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35035/

Review commit: https://reviewboard.mozilla.org/r/35037/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/35037/

vivek: OK, I have gone over part 1 in detail, and pushed some review nits. I had some pre: nits above; please address them and rename the pre to part 1. Then, if you are happy with my review comments, fold that into part 2. Those two are ready to land. We'll break the other stuff into new tickets. I made the following significant changes: * no build flag. Disabling the build flag would not have worked as you wrote it; it just would have failed tests. * I removed the AES encryption. We won't use a static key or anything like that, so no sense using anything more than NullCipher right now. * This patch was very sloppy about null vs. "". TextUtils.isEmpty is almost never correct for database code! Empty strings should never be special. I have the following concerns but I don't want to block this further: * It's not obvious that you're handling the profile path in the content URI correctly. I /think/ you are, since you inherit from the PerProfile* classes; but the test has additional magic around fake contexts which make it non-obvious. We can address this as needed. * You've duplicated the PasswordsProvider Sync interface, which is /not/ the same as all of the other Android-side providers. That is, you don't respect the "sync" parameter in the URI, and handle deletions accordingly. I expect we'll need to update the Sync password implementation to unify, but I don't know when that will be needed. * The copy-pasted MatrixCursor rewriting almost certainly does not do the right thing when it comes to notifications of changes. I didn't try to test this (e.g., like https://dxr.mozilla.org/mozilla-central/source/mobile/android/tests/browser/robocop/src/org/mozilla/gecko/tests/testBrowserProvider.java#1728). I think the existing PasswordsProvider is bad, so this is no worse; but we'll want to test it as we transition.

vivek: we've accumulated a lot of review requests; if you're happy with the above, please abandon them or however you do that in MozReview. Or mark them r+/not up for review as appropriate. We'll come back to the JS work in another ticket. I think after the above you should be largely unblocked for Bug 1247999 \o/

https://reviewboard.mozilla.org/r/35035/#review32391 @Nick: I will push my changes as you asked for with the following changes ::: mobile/android/base/java/org/mozilla/gecko/db/BrowserDatabaseHelper.java:332 (Diff revision 1) > + private boolean didCreateLoginsTable = false; This boolean check is unnecessary. The user either has database in version 28 with logins table setup or has db version prior to that. In the later case, the user is migrated to version 28. Bug 1219323 also did something similar to remove the checks for didCreateTabsTable and didCreateReadingListTable

Comment on attachment 8694989 [details] MozReview Request: Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26945/diff/9-10/

Comment on attachment 8708461 [details] MozReview Request: Bug 946857 - Part 2: Add Android LoginsProvider. r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31057/diff/5-6/

Comment on attachment 8708461 [details] MozReview Request: Bug 946857 - Part 2: Add Android LoginsProvider. r=nalexander Review will continue in other tickets.

Comment on attachment 8694989 [details] MozReview Request: Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26945/diff/10-11/

Comment on attachment 8708461 [details] MozReview Request: Bug 946857 - Part 2: Add Android LoginsProvider. r=nalexander Review request updated; see interdiff: https://reviewboard.mozilla.org/r/31057/diff/6-7/

https://hg.mozilla.org/integration/fx-team/rev/ca822e985ba5baae25a400b0031199a0561cfc9e Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander https://hg.mozilla.org/integration/fx-team/rev/9e4c8d1ffd3c23944c9a0741b3f4e64ea240b832 Bug 946857 - Part 2: Add Android LoginsProvider. r=nalexander

I had to back this out for testBrowserDatabaseHelperUpgrades failures: https://treeherder.mozilla.org/logviewer.html#?job_id=7737743&repo=fx-team https://hg.mozilla.org/integration/fx-team/rev/c3b966f02d76

(In reply to Wes Kocher (:KWierso) from comment #89) > I had to back this out for testBrowserDatabaseHelperUpgrades failures: > > https://treeherder.mozilla.org/logviewer.html#?job_id=7737743&repo=fx-team > > https://hg.mozilla.org/integration/fx-team/rev/c3b966f02d76 Vivek: mcomella just added the testBrowserDatabaseHelperUpgrades in: https://hg.mozilla.org/mozilla-central/rev/6bf5e3d73e4a This means you'll need to add a v29.db, which is created using a version of fennec built _without_ your DB upgrade code (even just a nightly from today is probably good enough). We're actually investigating some issues with this test in Bug 1252978, it's possible that a the DB has to be created on a device running Android 2.3 - I'm doing some testing in that area right now, I'll hopefully have more information later today, and I can probably give you a v29.db (that's been created on a 2.3 device) to stuff into your DB upgrade commit. (The new test just needs a new DB for the previous DB version every time the DB is upgraded.)

https://hg.mozilla.org/integration/fx-team/rev/b5774ec699c9678535e082d16238a331073e272c Bug 946857 - Part 1: Expose disabled hosts through Password CP r=nalexander https://hg.mozilla.org/integration/fx-team/rev/2c6222ddc0330ef5b0473363889a46110a8ade07 Bug 946857 - Part 2: Add Android LoginsProvider. r=nalexander

https://hg.mozilla.org/mozilla-central/rev/b5774ec699c9 https://hg.mozilla.org/mozilla-central/rev/2c6222ddc033

Comment on attachment 8708461 [details] MozReview Request: Bug 946857 - Part 2: Add Android LoginsProvider. r=nalexander https://reviewboard.mozilla.org/r/31057/#review42399 I'm removing myself from this review. I'm embarrased that I didn't get to it for months and months, for which I apologize. In principle there is nothing wrong with this code; I expect it to be landed in more or less this form. Unfortunately, I think there needs to be a significant effort to test the JS interface, and I haven't manufactured the time to look at that testing effort. That's what needs to happen in order for this to land.