Closed Bug 1225798 Opened 4 years ago Closed 4 years ago

Fix error reporting of edge reading list migration failures

Categories

(Firefox :: Migration, defect)

All
Windows 10
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 45
Tracking Status
firefox45 --- verified

People

(Reporter: Gijs, Assigned: Gijs)

References

(Blocks 2 open bugs)

Details

Attachments

(2 files)

+++ 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.
Depends on: 1225800
No longer depends on: 1225800
Blocks: 1225466
No longer depends on: 1225466
Bug 1225798 - improve error reporting of Edge reading list and favourites migration issues, r?MattN
Attachment #8688941 - Flags: review?(MattN+bmo)
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.
Attachment #8688941 - Flags: review?(MattN+bmo) → review+
Bug 1225798 - part 1: add better error reporting for reading list importer's ESE handling, r?MattN
Attachment #8690980 - Flags: review?(MattN+bmo)
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/
Attachment #8688941 - Attachment description: MozReview Request: Bug 1225798 - improve error reporting of Edge reading list and favourites migration issues, r?MattN → MozReview Request: Bug 1225798 - part 2: move reading list import to its own bookmarks migrator source, r?MattN
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.
Attachment #8688941 - Flags: review+ → review?(MattN+bmo)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
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.
Attachment #8690980 - Flags: review?(MattN+bmo) → review+
Attachment #8688941 - Flags: review?(MattN+bmo) → review+
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
https://hg.mozilla.org/mozilla-central/rev/2998cc7e851f
https://hg.mozilla.org/mozilla-central/rev/80cc747d4e5e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Depends on: 1228482
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.
(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.
Flags: qe-verify+
Verified fixed using Firefox 45 beta 8 (build ID 20160221141421) on Windows 10 x64.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.