Last Comment Bug 834034 - Migrate persdict.dat when resetting Firefox
: Migrate persdict.dat when resetting Firefox
Status: RESOLVED FIXED
: user-doc-complete
Product: Firefox
Classification: Client Software
Component: Migration (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: Firefox 23
Assigned To: :Gijs Kruitbosch
:
: Matthew N. [:MattN] (PM me if requests are blocking you)
Mentors:
Depends on:
Blocks: reset-firefox
  Show dependency treegraph
 
Reported: 2013-01-23 15:17 PST by Mardeg
Modified: 2014-01-24 00:30 PST (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Migrate the file (1.50 KB, patch)
2013-04-05 06:11 PDT, :Gijs Kruitbosch
MattN+bmo: review+
Details | Diff | Splinter Review

Description Mardeg 2013-01-23 15:17:25 PST
From a user experience point of view someone doing a reset could lose years of custom spellings they added. I've looked for serious problems that point to persdict.dat as the cause and haven't found any myself, but others can comment if they know any.
Comment 1 Florian Bender 2013-02-28 06:13:01 PST
Should this copy the file, move the file, or re-write (i. e. read & write database entries directly) the file? It may make a difference if the file is corrupt.
Comment 2 Tyler Downer [:Tyler] 2013-03-18 17:43:29 PDT
I would suggest in this case (and any where we transfer user data) we verify and re-write the file to help avoid any issues with corruption.
Comment 3 Matthew N. [:MattN] (PM me if requests are blocking you) 2013-03-19 17:04:25 PDT
(In reply to Tyler Downer [:Tyler] from comment #2)
> I would suggest in this case (and any where we transfer user data) we verify
> and re-write the file to help avoid any issues with corruption.

If we're adding the ability to verify the integrity of a file then we should do this whenever we read the file (or on idle for a long-running verification) and do the re-write fix then.  There's no need to wait for a user to reset in order to repair known data issues.  We already do this type of thing with places.sqlite and sessionstore.js as an example.  Reset should be saved for fixing cases where it's not straightforward to write code to identify problems.

It would be fine to run such verification code before migrating the data to the new profile in case the regular checks aren't running for some reason. In this case the file format is simple and the file gets re-written on save so I think the chances of corruption are unlikely.  If there are known dictionary corruption issues, file a bug to detect such issues and repair them automatically.
Comment 4 :Gijs Kruitbosch 2013-04-05 06:11:18 PDT
Created attachment 733826 [details] [diff] [review]
Migrate the file

Just migrating the file is pretty straightforward. Looking at the personal dictionary implementation (which by its own comments is "braindead"), it seems like it'd be hard to corrupt it in a meaningful way, as it's just a UTF-8 encoded newline-separated list of words. ( http://hg.mozilla.org/mozilla-central/annotate/55f9e3e3dae7/extensions/spellcheck/src/mozPersonalDictionary.cpp#l143 ). So, not doing any verification/checking seems best.
Comment 5 :Gijs Kruitbosch 2013-04-05 06:12:30 PDT
Matt, asking feedback because I'm not entirely sure who to ask for review, and I guess that because of Fx reset, you own it now. :-)
Comment 6 :Gijs Kruitbosch 2013-04-05 06:13:33 PDT
Ahem, sorry for bugspam, but I forgot: do we need to update the UI to specifically call out that we save the user's personal dictionary? I'm not decided, but I guess at a minimum we should correct this SUMO article if/when this lands: https://support.mozilla.org/en-US/kb/reset-firefox-easily-fix-most-problems
Comment 7 Matthew N. [:MattN] (PM me if requests are blocking you) 2013-04-07 23:37:04 PDT
Comment on attachment 733826 [details] [diff] [review]
Migrate the file

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

(In reply to :Gijs Kruitbosch from comment #4)
>  So, not doing any verification/checking seems best.

I agree in this case. If the file format wasn't so simple, I would probably lean towards WONTFIX for the bug (for the same reason as #2 below).

(In reply to :Gijs Kruitbosch from comment #6)
> do we need to update the UI to
> specifically call out that we save the user's personal dictionary? I'm not
> decided, but I guess at a minimum we should correct this SUMO article
> if/when this lands:
> https://support.mozilla.org/en-US/kb/reset-firefox-easily-fix-most-problems

I don't think it's necessary to list it in the dialog for two reasons:
1) If there is too much to read, people won't read it
2) I think it's unlikely that someone would cancel a reset because they thought their personal dictionary wouldn't be migrated.

We should update the 64_firefox string in migration.properties though.  I'm tempted to change it back to "Other Data" since I don't think it was necessary to call out bookmark backups in the first place and because every change to these strings requires adding a slightly hacky mapping in MU_getLocalizedString.  r=me with the string change.

Updating the SUMO article makes sense to me.  I'll add the user-doc-needed keyword to the bug.
Comment 8 :Gijs Kruitbosch 2013-04-08 14:18:39 PDT
(In reply to Matthew N. [:MattN] from comment #7)
> r=me with the string change.


Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/35179d807373
Comment 9 Ed Morley [:emorley] 2013-04-09 02:28:49 PDT
https://hg.mozilla.org/mozilla-central/rev/35179d807373
Comment 10 :Gijs Kruitbosch 2014-01-24 00:30:10 PST
(I just noticed that the personal dictionary is now listed as something that will be saved)

Note You need to log in before you can comment on or make changes to this bug.