Closed
Bug 706005
Opened 13 years ago
Closed 9 years ago
Chrome migrator should handle when databases are locked
Categories
(Firefox :: Migration, defect)
Firefox
Migration
Tracking
()
Tracking | Status | |
---|---|---|
firefox42 | --- | verified |
People
(Reporter: MattN, Assigned: rittme)
References
Details
(Keywords: uiwanted, Whiteboard: [strings])
Attachments
(1 file)
Currently, if Chrome is open and I import history from the running profile then it will fail without telling me or importing any history. The failure is because the sqlite history database is locked. Below is the error reported in the error console:
Error: [Exception... "Component returned failure code: 0x80630001 (NS_ERROR_STORAGE_BUSY) [nsINavHistoryService.runInBatchMode]" nsresult: "0x80630001 (NS_ERROR_STORAGE_BUSY)" location: "JS frame :: file:///Users/foo/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/components/ChromeProfileMigrator.js :: Chrome_migrateHistory :: line 306" data: no]
Source File: file:///Users/foo/mozilla-central/obj-ff-dbg/dist/NightlyDebug.app/Contents/MacOS/components/ChromeProfileMigrator.js
Line: 308
We should probably tell users to close the source browser if we detect an item we want to import from is locked.
Reporter | ||
Updated•13 years ago
|
Comment 1•13 years ago
|
||
Migration runs automatically during startup, so our options here are limited. What would happen:
1. Use uses chrome as its default browser and decides to download Fx.
2. Installer runs. Firefox is opened.
3. We ask him/her to close Chrome before he can use Firefox.
4. If that went well, we'll then ask him to also make Fx the default browser.
Somewhat violent, I would say. Isn't there any way we can deal with the locked database?
Comment 2•13 years ago
|
||
However, it's also bad that the current UI tells you History and Cookies were migrated even though it's notified for errors.
Comment 3•13 years ago
|
||
Two more notes:
1. When chrome is open, we also fail to read the "Local State" fail, in which chrome's profile are listed. However, this exception is swallowed, and we fallback to importing the "Default" profile, from which we fail to import history and cookies (sqlite dbs), but succeed in importing bookmarks (json file). If this exceptions wasn't swallowed the way it is, we would't try to import from Chrome at all.
2. So, given comment 1, one solution would be to avoid auto-migration from Chrome in this case, but offer migration from crhome (plus instruction to close it) if migration is manually invoked later. I filed bug 731144 to make this option a little bit more realistic.
Keywords: uiwanted
Updated•13 years ago
|
Severity: normal → major
Comment 4•13 years ago
|
||
Alex: this one is pretty important. To be clear, if Chrome runs along with the migration wizard (and that's very likely for someone who's going to migrate from Chrome), migration is guaranteed to fail.
Reporter | ||
Updated•10 years ago
|
Flags: firefox-backlog+
Updated•10 years ago
|
Points: --- → 13
Flags: qe-verify?
Reporter | ||
Comment 6•9 years ago
|
||
For now we're just going to proactively tell the user to close the source browser since it's better than nothing.
The current dialog looks like this (on Nightly not during the first run): http://grab.by/Jovm
A "Don't import anything" option appears as the last radio button during first run.
I think we can add a sentence (in bold) below the radio buttons telling the user to close the source browser before continuing. To be smart, we could have the text show up depending on the selected radio button so it won't show up for IE (Safari too?) or "Don't import anything" options where it isn't relevant.
Matej, what do you think of this:
"For best results, please ensure the selected browser is closed before continuing."
Thanks.
Assignee: nobody → bernardo
Status: NEW → ASSIGNED
Iteration: --- → 42.3 - Aug 10
Points: 13 → 3
Flags: qe-verify?
Flags: qe-verify+
Flags: needinfo?(matej)
Whiteboard: [strings]
Comment 7•9 years ago
|
||
Looks OK to me, just wondering if we need "For best results" at the beginning. It reads cleaner without it.
Flags: needinfo?(matej)
Comment 8•9 years ago
|
||
Yeah, I'd nix that part. It's not that you get better results, it's that it's _broken_ if you don't.
Assignee | ||
Comment 9•9 years ago
|
||
Bug 706005 - Chrome migrator notifies user to close browser. r=MattN
Attachment #8645253 -
Flags: review?(MattN+bmo)
Assignee | ||
Comment 10•9 years ago
|
||
Comment on attachment 8645253 [details]
MozReview Request: Bug 706005 - Chrome migrator notifies user to close browser. r=MattN
Bug 706005 - Chrome migrator notifies user to close browser. r=MattN
Reporter | ||
Comment 11•9 years ago
|
||
Comment on attachment 8645253 [details]
MozReview Request: Bug 706005 - Chrome migrator notifies user to close browser. r=MattN
https://reviewboard.mozilla.org/r/15481/#review13843
::: browser/components/migration/content/migration.js:88
(Diff revision 2)
>
> + group.addEventListener("click", function() {
I don't think "click" covers keyboard navigation. I think you may want "command" or if it's like HTML, maybe "change"? Please test this manually
::: browser/components/migration/content/migration.js:89
(Diff revision 2)
> + group.addEventListener("click", function() {
I think you also need to call this function when the page initially displays since Chrome can be pre-selected depending on the OS and other available browsers (you can mock this for testing). You'll want to name the function and call it from the event listener and on load or whatever wizards use.
Attachment #8645253 -
Flags: review?(MattN+bmo) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8645253 -
Flags: review+ → review?(MattN+bmo)
Assignee | ||
Comment 12•9 years ago
|
||
Comment on attachment 8645253 [details]
MozReview Request: Bug 706005 - Chrome migrator notifies user to close browser. r=MattN
Bug 706005 - Chrome migrator notifies user to close browser. r=MattN
Reporter | ||
Comment 14•9 years ago
|
||
Comment on attachment 8645253 [details]
MozReview Request: Bug 706005 - Chrome migrator notifies user to close browser. r=MattN
https://reviewboard.mozilla.org/r/15481/#review13865
I made some tweaks myself since we want this landed before string freeze:
::: browser/components/migration/content/migration.js:69
(Diff revision 3)
> + if (group.selectedItem.id == "chrome" ||
> + group.selectedItem.id == "canary" ||
> + group.selectedItem.id == "chromium") {
The front-end shouldn't have to know this much about the migrators so I exposed this as a property on each migrator instead. Take a look at what I landed.
::: browser/components/migration/content/migration.xul:54
(Diff revision 3)
> <label id="noSources" hidden="true">&noMigrationSources.label;</label>
> + <description class="header" id="closeSourceBrowser" hidden="true">&closeSourceBrowser.label;</description>
I added a flexiable spacer above this since it seemed too close to the radio group and attracted less attention since it was further from the next button.
::: browser/components/migration/content/migration.js:72
(Diff revision 3)
> + document.getElementById("closeSourceBrowser").hidden = false;
> + } else {
> + document.getElementById("closeSourceBrowser").hidden = true;
Hidden is supposed to be the same as visibility:hidden according to MDN but that didn't seem to be the case so I switched to visibility:hidden; so the layout wouldn't change in some case upon toggling options.
Attachment #8645253 -
Flags: review?(MattN+bmo) → review+
Comment 15•9 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox42:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Updated•9 years ago
|
QA Contact: florin.mezei
Comment 16•9 years ago
|
||
I was able to reproduce this issue on Firefox 41 RC (20150917150946) using Windows 7 64-bit.
Verified fixed on Firefox 42 Beta 1 (20150921151815) under Windows 7 64-bit, Ubuntu 14.04 32-bit and Mac Os X 10.10.4.
“Please ensure the selected browser is closed before continuing.” message is successfully displayed when Chrome radio button is selected.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•