Use mozIStorageAsyncConnection in migrators

RESOLVED FIXED in Firefox 53

Status

()

defect
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: mak, Assigned: TheLayman, Mentored)

Tracking

(Blocks 1 bug)

unspecified
Firefox 53
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [good first bug][lang=js])

Attachments

(1 attachment, 3 obsolete attachments)

All of the consumers here are async, so we should be able to convert them to the new async connection.
or use Sqlite.jsm
The only remaining call is here:
http://searchfox.org/mozilla-central/rev/f5c9e9a249637c9abd88754c8963ecb3838475cb/browser/components/migration/tests/unit/test_Chrome_passwords.js#150
it could be converted to Sqlite.jsm
Mentor: mak77
Whiteboard: [good first bug][lang=js]
Hi. Newbie here.
Can I work on this?
(In reply to George Veneel Dogga from comment #3)
> Hi. Newbie here.
> Can I work on this?

Sure you can. comment 2 is still valid, that test should be refactored to use Sqlite.jsm API instead of the existing calls to mozIStorageConnection.
You can see an example of that at:
http://searchfox.org/mozilla-central/rev/790b2cb423ea1ecb5746722d51633caec9bab95a/browser/components/migration/MigrationUtils.jsm#602
Assignee: nobody → georgeveneeldogga
Posted patch V1.patch (obsolete) — Splinter Review
Hey mak, I was on linux so the test was skipped. 
Please let me know about the errors. :)
Attachment #8827889 - Flags: review?(mak77)
Attachment #8827889 - Flags: feedback+
Comment on attachment 8827889 [details] [diff] [review]
V1.patch

Feedback and review should only be given by reviewers
Attachment #8827889 - Flags: feedback+
Comment on attachment 8827889 [details] [diff] [review]
V1.patch

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

::: browser/components/migration/tests/unit/test_Chrome_passwords.js
@@ +81,3 @@
>  
>  function promiseSetPassword(login) {
>    return new Promise((resolve, reject) => {

The new execute method already returns a promise, so there should be no need to further  wrap its call into a new Promise constructor, you should just return dbConn.execute....

@@ +85,3 @@
>      let passwordValue = crypto.stringToArray(crypto.encryptData(login.password));
> +    let params={password_value: passwordValue, rowid: login.id };
> +    dbConn.execute(stmt,params);

This not mandatory clearly, just a coding style thing. I'd prefer to have the arguments inline, something like

return dbConn.execute(`UPDATE...
                       SET...
                       WHERE
                      `, { param1:...,
                           param2:... });
The backticks ` is a special char that indicates a template literal (https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals) that allows to go multi-line.

@@ +85,4 @@
>      let passwordValue = crypto.stringToArray(crypto.encryptData(login.password));
> +    let params={password_value: passwordValue, rowid: login.id };
> +    dbConn.execute(stmt,params);
> +    dbConn.close();

why are you closing the connection here? the old code was not doing that, it was only finalizing the statement. Sqlite.jsm finalizes the statement for you already.

@@ +129,5 @@
>  
>  add_task(function* setup() {
>    let loginDataFile = do_get_file("AppData/Local/Google/Chrome/User Data/Default/Login Data");
> +  let dbConn = yield Sqlite.openConnection({path: "loginDataFile"});
> +  //dbConn = Services.storage.openUnsharedDatabase(loginDataFile);

the path should be a real path to the file, not just a string.
loginDataFile is an instance of nsIFile: http://searchfox.org/mozilla-central/source/xpcom/io/nsIFile.idl
it has a path attribute, so it should be 
{ path: loginDataFile.path }

(clearly you should also remove the commented out code)

@@ +134,5 @@
>    registerFakePath("LocalAppData", do_get_file("AppData/Local/"));
>  
>    do_register_cleanup(() => {
>      Services.logins.removeAllLogins();
> +    dbConn.close();

this is asynchronous and returns a promise, thus you should also change
do_register_cleanup(() => {
to
do_register_cleanup(function* () {
and yield dbConn.close();
Attachment #8827889 - Flags: review?(mak77)
Before I forget, please add a commit message, the reviewer and yourself as patch author.
See https://developer.mozilla.org/en-US/docs/Mercurial/Using_Mercurial#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F
Posted patch V1.1 patch (obsolete) — Splinter Review
I hope I've added all the changes as you said. 
Please let me know about the errors :)
Attachment #8827889 - Attachment is obsolete: true
Attachment #8828122 - Flags: review?(mak77)
Comment on attachment 8828122 [details] [diff] [review]
V1.1 patch

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

The patch is still missing a commit message, the reviewer must be at the end of the commit message. The style is usually:
Bug 887876 - Use Sqlite.jsm in browser/components/migration. r=mak

::: browser/components/migration/tests/unit/test_Chrome_passwords.js
@@ +83,5 @@
>  
> +    let stmt = "UPDATE logins SET password_value = :password_value WHERE rowid = :rowid";
> +    let passwordValue = crypto.stringToArray(crypto.encryptData(login.password));
> +    let params={password_value: passwordValue, rowid: login.id };
> +    return dbConn.execute(stmt,params);

you didn't address my cosing style comment. It may also be ok like this, but please always add a whitespace after commas and around =, we have eslint support for that and coding style tests would fail.

@@ +132,3 @@
>    registerFakePath("LocalAppData", do_get_file("AppData/Local/"));
>  
> +  do_register_cleanup(function* finalize_cleanup() {

Sorry, I just checked and do_register_cleanup doesn't support generators :(
It supports promises though, so here you should
do_register_cleanup(() => {
  Services.logins.removeAllLogins();
  crypto.finalize();
  return dbConn.close();
});
Attachment #8828122 - Flags: review?(mak77)
Posted patch Proposed patch (obsolete) — Splinter Review
Attachment #8828122 - Attachment is obsolete: true
Attachment #8828349 - Flags: review?(mak77)
Comment on attachment 8828349 [details] [diff] [review]
Proposed patch

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

it looks good, pushing to Try now.
Attachment #8828349 - Flags: review?(mak77) → review+
Status: NEW → ASSIGNED
The ES test noticed a couple problems:

TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/migration/tests/unit/test_Chrome_passwords.js:133:7 | 'dbConn' is already declared in the upper scope. (no-shadow)

indeed the let dbConn is shadowing the var dbConn at the top, please just remove "let".

TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/migration/tests/unit/test_Chrome_passwords.js:133:22 | 'Sqlite' is not defined. (no-undef)

Ehr right, you must import the Sqlite.jsm module, but I'd suggest to do that in the header file:
http://searchfox.org/mozilla-central/source/browser/components/migration/tests/unit/head_migration.js
Here just add a:
XPCOMUtils.defineLazyModuleGetter(this, "Sqlite",
                                  "resource://gre/modules/Sqlite.jsm");
close to the other imports at the top.
This way all the tests in this folder will be able to use the Sqlite object without having to import it (the head file is loaded before each xpcshell test).

To understand the X failure, just click on it, then click on the [LOG] icon, then you should be able to see this:
10:58:05     INFO -  PROCESS | 5908 | A coding exception was thrown and uncaught in a Task.
10:58:05     INFO -  PROCESS | 5908 | Full message: ReferenceError: Sqlite is not defined
...
That is what we are already fixing.
Posted patch V2.0.patchSplinter Review
Attachment #8828349 - Attachment is obsolete: true
Attachment #8828773 - Flags: review?(mak77)
Comment on attachment 8828773 [details] [diff] [review]
V2.0.patch

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

Looks good. Let's try again :)
Attachment #8828773 - Flags: review?(mak77) → review+
Depends on: 1247602
I have confirmed that the test passes once Bug 1247602 is fixed, I'll push that patch today.
tested locally.
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6328f5d01711
Use Sqlite.jsm in browser/components/migration. r=mak
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6328f5d01711
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.