1225798 - Fix error reporting of edge reading list migration failures

Status: VERIFIED FIXED

(Firefox :: Migration, defect)

Description

[:Gijs (he/him)]

+++ This bug was initially created as a clone of Bug #1225466 +++

There's a trivial mistake in the error handling here:

https://dxr.mozilla.org/mozilla-central/rev/a2f83cbe53ac4009afa4cb2b0b8f549289b23eeb/browser/components/migration/MSMigrationUtils.jsm#479

where the use of 'dbPath' should be 'dbFile.path'

We should also be reporting errors correctly so the parent migrator doesn't pretend all is fine and dandy when the reading list import failed.

I reproduced one issue with the migrator if the DB had outstanding changes in its log files that hadn't yet been reconciled into the DB, at which point the DB is in an "unclean shutdown" state.

I don't know how easy it is to fix that, but fixing it properly should probably be a different bug.

Comment 1

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1225798 - part 2: move reading list import to its own bookmarks migrator source, r?MattN

Bug 1225798 - improve error reporting of Edge reading list and favourites migration issues, r?MattN

Comment 2

[Matthew N. [:MattN]]

Comment on attachment 8688941 [details]
MozReview Request: Bug 1225798 - part 2: move reading list import to its own bookmarks migrator source, r?MattN

https://reviewboard.mozilla.org/r/25461/#review22997

::: browser/components/migration/MSMigrationUtils.jsm:377
(Diff revision 1)
>            yield MigrationUtils.createImportedBookmarksFolder(this.importedAppLabel, folderGuid);
>        }
> -      yield this._migrateFolder(this._favoritesFolder, folderGuid);
> +      yield this._migrateFolder(this._favoritesFolder, folderGuid).catch(ex => {
> +        Cu.reportError(ex);
> +        succeeded = false;
> +      });
>  
>        if (this._migrationType == MSMigrationUtils.MIGRATION_TYPE_EDGE) {
> -        yield this._migrateEdgeReadingList(PlacesUtils.bookmarks.menuGuid);
> +        yield this._migrateEdgeReadingList(PlacesUtils.bookmarks.menuGuid).catch(ex => {
> +          Cu.reportError(ex);
> +          succeeded = false;
> +        });
>        }
> -    }.bind(this)).then(() => aCallback(true),
> +    }.bind(this)).then(() => aCallback(succeeded),
>                          e => { Cu.reportError(e); aCallback(false) });

Reading list should have probably been it's own migrator of the bookmark type (we support multiple of each) so we didn't have to do this more complicated success/error handling as we could just throw and have the aCallback(false) used. 

I normally prefer to default to succeeded = false too which that would make easier.

I guess this is okay for now.

::: browser/components/migration/MSMigrationUtils.jsm:448
(Diff revision 1)
>        } catch (ex) {
>          Components.utils.reportError("Unable to import " + this.importedAppLabel + " favorite (" + entry.leafName + "): " + ex);
> +        succeeded = false;
> +      }

Please ensure that no errors are reported in relatively clean IE install perhaps due to some custom URL scheme being used.

Comment 3

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1225798 - part 1: add better error reporting for reading list importer's ESE handling, r?MattN

Bug 1225798 - part 1: add better error reporting for reading list importer's ESE handling, r?MattN

Comment 4

[:Gijs (he/him)]

Comment on attachment 8688941 [details]
MozReview Request: Bug 1225798 - part 2: move reading list import to its own bookmarks migrator source, r?MattN

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

Comment 5

[:Gijs (he/him)]

Comment on attachment 8688941 [details]
MozReview Request: Bug 1225798 - part 2: move reading list import to its own bookmarks migrator source, r?MattN

I ended up moving all the things anyway as suggested in comment #2 because of how the edge bookmarks have moved as well (bug 1226556). In part 1 I also added logging for unknown JET errors so we can find out what goes wrong on users' machines if they file bugs with NS_ERROR_FAILURE as the exception.

Comment 6

[Matthew N. [:MattN]]

Comment on attachment 8690980 [details]
MozReview Request: Bug 1225798 - part 1: add better error reporting for reading list importer's ESE handling, r?MattN

https://reviewboard.mozilla.org/r/26015/#review23407

::: browser/components/migration/nsEdgeReadingListExtractor.cpp:206
(Diff revision 1)
> +      consoleService->LogStringMessage(msg);

Seems like this should be reportError but I'm not sure if that's much harder.

Comment 7

[Matthew N. [:MattN]]

Comment on attachment 8688941 [details]
MozReview Request: Bug 1225798 - part 2: move reading list import to its own bookmarks migrator source, r?MattN

https://reviewboard.mozilla.org/r/25461/#review23535

I didn't get to this the other day because of the large code movement and RB doesn't help with that when it's between files…

::: browser/components/migration/EdgeProfileMigrator.js:84
(Diff revision 2)
> +function EdgeReadListMigrator() {
> +}
> +
> +EdgeReadListMigrator.prototype = {

Nit: why not `EdgeReadingListMigrator`?

::: browser/components/migration/EdgeProfileMigrator.js:95
(Diff revision 2)
> +    Cu.reportError("migrate called");

Nit: leftover debugging code

::: browser/components/migration/EdgeProfileMigrator.js:145
(Diff revision 2)
> +    }
> +  }),
> +  _migrateReadingListDB: Task.async(function*(dbFile, parentGuid) {
> +    dbFile.appendRelativePath("DBStore\\spartan.edb");

Nit: missing new line

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/2998cc7e851f
https://hg.mozilla.org/integration/fx-team/rev/80cc747d4e5e

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2998cc7e851f
https://hg.mozilla.org/mozilla-central/rev/80cc747d4e5e

Comment 10

[Xidorn Quan [:xidorn] UTC+11]

https://reviewboard.mozilla.org/r/26015/#review23755

::: browser/components/migration/nsEdgeReadingListExtractor.cpp:204
(Diff revision 1)
> +      wchar_t* msg = new wchar_t[80];

Isn't it a memory leak? I think you should be able to just use "wchar_t msg[80];" here.

Comment 11

[:Gijs (he/him)]

(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10)
> https://reviewboard.mozilla.org/r/26015/#review23755
> 
> ::: browser/components/migration/nsEdgeReadingListExtractor.cpp:204
> (Diff revision 1)
> > +      wchar_t* msg = new wchar_t[80];
> 
> Isn't it a memory leak? I think you should be able to just use "wchar_t
> msg[80];" here.

Seems like you're fixing this in bug 1228482.

Comment 12

[Cornel Ionce [:noni] [Hubs QA]]

Verified fixed using Firefox 45 beta 8 (build ID 20160221141421) on Windows 10 x64.