Use mozIStorageAsyncConnection in migrators

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Migration
RESOLVED FIXED
4 years ago
5 months 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)

(Reporter)

Description

4 years ago
All of the consumers here are async, so we should be able to convert them to the new async connection.
(Reporter)

Comment 1

2 years ago
or use Sqlite.jsm
(Reporter)

Comment 2

8 months ago
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@bonardo.net
Whiteboard: [good first bug][lang=js]
(Assignee)

Comment 3

5 months ago
Hi. Newbie here.
Can I work on this?
(Reporter)

Comment 4

5 months ago
(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
(Assignee)

Comment 5

5 months ago
Created attachment 8827889 [details] [diff] [review]
V1.patch

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+
(Reporter)

Comment 6

5 months ago
Comment on attachment 8827889 [details] [diff] [review]
V1.patch

Feedback and review should only be given by reviewers
Attachment #8827889 - Flags: feedback+
(Reporter)

Comment 7

5 months ago
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)
(Reporter)

Comment 8

5 months ago
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
(Assignee)

Comment 9

5 months ago
Created attachment 8828122 [details] [diff] [review]
V1.1 patch



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)
(Reporter)

Comment 10

5 months ago
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)
(Assignee)

Comment 11

5 months ago
Created attachment 8828349 [details] [diff] [review]
Proposed patch
Attachment #8828122 - Attachment is obsolete: true
Attachment #8828349 - Flags: review?(mak77)
(Reporter)

Comment 12

5 months ago
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+
(Reporter)

Comment 13

5 months ago
 https://treeherder.mozilla.org/#/jobs?repo=try&revision=08bff13589b0f475368f76daee27250a1408b555
(Assignee)

Updated

5 months ago
Status: NEW → ASSIGNED
(Reporter)

Comment 14

5 months ago
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.
(Assignee)

Comment 15

5 months ago
Created attachment 8828773 [details] [diff] [review]
V2.0.patch
Attachment #8828349 - Attachment is obsolete: true
Attachment #8828773 - Flags: review?(mak77)
(Reporter)

Comment 16

5 months ago
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+
(Reporter)

Comment 17

5 months ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a74ed456e6261050fe8135b4def69299c516479d
(Reporter)

Updated

5 months ago
Depends on: 1247602
(Reporter)

Comment 18

5 months ago
I have confirmed that the test passes once Bug 1247602 is fixed, I'll push that patch today.
(Reporter)

Comment 19

5 months ago
tested locally.
Keywords: checkin-needed

Comment 20

5 months ago
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

Comment 21

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/6328f5d01711
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.