1229076 - Implement a generic JS-based module for getting data out of ESE databases using jsctypes and use it to import the reading list (instead of nsReadingListExtractor.cpp)

Status: RESOLVED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (he/him)]

To be used for the Edge reading list import and then the bookmarks one in bug 1226556.

Comment 1

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1229076 - use JS module for reading ESE database, r?MattN

Bug 1229076 - use JS module for reading ESE database

Comment 2

[:Gijs (he/him)]

Comment on attachment 8694887 [details]
MozReview Request: Bug 1229076 - use JS module for reading ESE database, r?MattN

Matt, any chance you have time to give me some feedback on this? I know you're heading to Orlando a bit earlier, not sure how much time you have.

Comment 3

[Matthew N. [:MattN]]

https://reviewboard.mozilla.org/r/26925/#review24713

f+

::: browser/components/migration/ESEDBReader.jsm:14
(Diff revision 1)
> +Cu.import("resource://gre/modules/devtools/Console.jsm");

Nit: you could use the prefix option from https://mxr.mozilla.org/mozilla-central/source/browser/components/uitour/UITour.jsm?rev=d9fd7783ba30#74

::: browser/components/migration/ESEDBReader.jsm:132
(Diff revision 1)
> +        console.error("Got error " + resultCode + " (" + rv.toSource() + ") calling " + methodName);

You mentioned about toString(10) instead

::: browser/components/migration/ESEDBReader.jsm:212
(Diff revision 1)
> +  gLibs.ese = ctypes.open('esent.dll');
> +  gLibs.kernel = ctypes.open('kernel32.dll');

Nit: double quotes

::: browser/components/migration/ESEDBReader.jsm:469
(Diff revision 1)
> +  addUser() {
> +    this._users++;
> +  },
> +  removeUser() {
> +    this._users--;
> +    if (this._users <= 0) {

I got this confused with user access control for databases but I understand now that this is more like reference counting. How about \_referenceCount or \_acquisitions and acquire/release?

Comment 4

[Matthew N. [:MattN]]

Comment on attachment 8694887 [details]
MozReview Request: Bug 1229076 - use JS module for reading ESE database, r?MattN

This doesn't seem too bad with js-ctypes

Comment 5

[:Gijs (he/him)]

Comment on attachment 8694887 [details]
MozReview Request: Bug 1229076 - use JS module for reading ESE database, r?MattN

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/26925/diff/1-2/

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8694887 [details]
MozReview Request: Bug 1229076 - use JS module for reading ESE database, r?MattN

https://reviewboard.mozilla.org/r/26925/#review26027

A unit test to do a sanity check import of an (in-tree) ESE database would be good. You can use registerFakePath like some other migration tests, if necessary.

::: browser/components/migration/ESEDBReader.jsm:208
(Diff revisions 1 - 2)
>  function unloadLibraries() {
> +  log.error("Unloading");
>    if (gOpenDBs.size) {

Shouldn't this be on a different log level like log.debug or log.info and then add a maxLogLevelPref option to the console API constructor? We don't normally log non-errors by default in builds. Of course that would mean that people who want the logging in new profiles would have to edit prefs.js by hand but that's probably fine since I suspect errors with this module would be reproducible since it's not modifying the DBs.

::: browser/components/migration/ESEDBReader.jsm:235
(Diff revisions 1 - 2)
>  function ESEDB(rootPath, dbPath, logPath) {
> +  log.error("Created db");
>    this.rootPath = rootPath;

ditto

::: browser/components/migration/ESEDBReader.jsm:470
(Diff revisions 1 - 2)
>      if (this._opened) {
> +      log.error("close db");
>        ESE.FailSafeCloseDatabase(this._sessionId, this._dbId, 0);
> +      log.error("finished close db");

ditto

::: browser/components/migration/ESEDBReader.jsm:307
(Diff revision 2)
> +  },
> +  checkForColumn(tableName, columnName) {

Nit: new line between methods

::: browser/components/migration/ESEDBReader.jsm:320
(Diff revision 2)
> +  },
> +  tableItems: function*(tableName, columns) {

Nit: new line between methods

::: browser/components/migration/ESEDBReader.jsm:388
(Diff revision 2)
> +  },
> +  _convertResult(column, buffer, err) {

Nit: new line between methods here are other places.

Comment 7

[:Gijs (he/him)]

(In reply to Matthew N. [:MattN] (vacation Jan. 1 – 10) from comment #6)
> Comment on attachment 8694887 [details]
> MozReview Request: Bug 1229076 - use JS module for reading ESE database,
> r?MattN
> 
> https://reviewboard.mozilla.org/r/26925/#review26027
> 
> A unit test to do a sanity check import of an (in-tree) ESE database would
> be good. You can use registerFakePath like some other migration tests, if
> necessary.

Two questions:
- Should this (ie having a test) block landing?
- ESE databases start at something ridiculous like 2MB, even if they're almost empty. I don't really want to check that into the tree. I could try to create and populate one from the test, using similar ctypes madness, but it would take a bit to get that right. It might also be possible to use some compression (e.g. check in a zipfile and unzip it in the test). I don't know that we have a way to fetch artifacts from somewhere else. What would you prefer?

Comment 8

[Matthew N. [:MattN]]

(In reply to :Gijs Kruitbosch from comment #7)
> Two questions:
> - Should this (ie having a test) block landing?

No, I was leaving it up to you to figure out what is best (do it here if trivial, postpoone to follow-up or don't do it if it's not worth it)

> - ESE databases start at something ridiculous like 2MB, even if they're
> almost empty. I don't really want to check that into the tree. I could try
> to create and populate one from the test, using similar ctypes madness, but
> it would take a bit to get that right. It might also be possible to use some
> compression (e.g. check in a zipfile and unzip it in the test). I don't know
> that we have a way to fetch artifacts from somewhere else. What would you
> prefer?

Given that, I'd say definitely don't block on a test. Maybe file follow-up to add sanity checks for the consumers? Or perhaps we can watch telemetry closer instead?

Comment 9

[Pulsebot]

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

Comment 10

[Pulsebot]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5e328358e3ec
https://hg.mozilla.org/mozilla-central/rev/807916112384

Comment 12

[:Gijs (he/him)]

Comment on attachment 8694887 [details]
MozReview Request: Bug 1229076 - use JS module for reading ESE database, r?MattN

Required for bug 1226556, so asking for beta uplift

Approval Request Comment
[Feature/regressing bug #]: Windows 10 update broke this, feature being importing bookmarks from Edge. We also realized that our importing of reading list was deficient in that it worked half or less of the time.
[User impact if declined]: No (or very much outdated) bookmarks imported from Edge; broken reading list import some of the time
[Describe test coverage new/current, TreeHerder]: nope. :-(
[Risks and why]: low risk. Not because the code is perfect, but because what we're improving upon is "it doesn't work". Improving "it doesn't work" is not hard. The code itself is limited to the Edge importer, so it's hard to imagine it would have an impact outside that.
[String/UUID change made/needed]: no

Comment 13

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #12)
> Comment on attachment 8694887 [details]
> MozReview Request: Bug 1229076 - use JS module for reading ESE database,
> r?MattN
> 
> Required for bug 1226556, so asking for beta uplift
> 
> Approval Request Comment
> [Feature/regressing bug #]: Windows 10 update broke this, feature being
> importing bookmarks from Edge. We also realized that our importing of
> reading list was deficient in that it worked half or less of the time.
> [User impact if declined]: No (or very much outdated) bookmarks imported
> from Edge; broken reading list import some of the time
> [Describe test coverage new/current, TreeHerder]: nope. :-(
> [Risks and why]: low risk. Not because the code is perfect, but because what
> we're improving upon is "it doesn't work". Improving "it doesn't work" is
> not hard. The code itself is limited to the Edge importer, so it's hard to
> imagine it would have an impact outside that.
> [String/UUID change made/needed]: no

For completeness' sake: this patch removes an interface. If necessary we could land it without removing the old interface, which is now unused in Firefox. I don't think it's necessary - I checked add-on MXR and nobody seems to use it (which is not very surprising, tbh).

Comment 14

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8694887 [details]
MozReview Request: Bug 1229076 - use JS module for reading ESE database, r?MattN

Gijs, I love your "Risks and why" section :)
Taking it to improve the situation.

Should be in 45 beta 5.

Comment 15

[Carsten Book [:Tomcat]]

running into 
remote: ************************** ERROR ****************************
remote: 
remote: *** IDL file browser/components/migration/nsIEdgeReadingListExtractor.idl altered in changeset ce6701c42892***
remote: 
remote: Changes to IDL files in this repo require you to provide binary change approval in your top comment in the form of ba=... (or, more accurately, ba\S*=...)
remote: This is to ensure that UUID changes (or method changes missing corresponding UUID change) are caught early, before release.
remote: 
remote: *************************************************************

can you take a look, thanks!

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Pretty sure sylvestre signed off on the binary changes with his approval, so went ahead and landed these:

https://hg.mozilla.org/releases/mozilla-beta/rev/01e4f115b078
https://hg.mozilla.org/releases/mozilla-beta/rev/dc19c61b3591

Comment 17

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

Set bug 1226556 for verification, which would also cover this.

Comment 18

[Mayur Patil]

[bugday-20160323]

Status: RESOLVED,FIXED -> UNVERIFIED

Comments:
STR: Not clear.
Developer specific testing

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel	beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS				Windows 7 SP1 x86_64

Expected Results: 
Developer specific testing

Actual Results: 
As expected