Closed Bug 895025 Opened 7 years ago Closed 6 years ago

Unable to import bookmarks from IE when they contain a .lnk to a folder

Categories

(Firefox :: Migration, defect)

22 Branch
All
Windows 7
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 30
Tracking Status
firefox28 --- wontfix
firefox29 --- verified
firefox30 --- verified

People

(Reporter: n9wys, Assigned: ryou)

References

Details

Attachments

(2 files, 5 obsolete files)

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:22.0) Gecko/20100101 Firefox/22.0 (Beta/Release)
Build ID: 20130618035212

Steps to reproduce:

Recently downloaded Firefox...  Ran the Import Bookmarks tool, per instructions.  Bookmarks did NOT completely import - only got ONE folder.  Ran tool a second time, same result. 


Actual results:

Fortunately, I had also installed Google Chrome - I ran Import Bookmarks tool, for bookmarks in Chrome, and was able to import ALL bookmarks this way.


Expected results:

Firefox should have imported bookmarks directly from IE 10
Component: Untriaged → Places
Product: Firefox → Toolkit
Assignee: nobody → ryou
Attached patch v.1 Patch (obsolete) — Splinter Review
I've found out that an exception is thrown every time Firefox imports a .lnk file under the Favorite folder (where IE stores bookmarks) and calls isDirectory() on the corresponding nsIFile object, which stops the import process.

Remove all .lnk files under the Favorite folder and import again. If successful this time, it's probably caused by the above reason.

In this situation, test if the nsIFile object represents a .lnk file by comparing its path and target attribute before calling isDirectory() and no exception would be thrown.
Attachment #8360948 - Flags: review?(mak77)
Comment on attachment 8360948 [details] [diff] [review]
v.1 Patch

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

Thank you, before setting the checkin-needed keyword, please correct the commit message so that's in the correct form:
Bug 895025 - Unable to import bookmarks from IE when they contain a .lnk to a folder. r=mak

::: browser/components/migration/src/IEProfileMigrator.js
@@ +227,4 @@
>  
>        // Make sure that entry.path == entry.target to not follow .lnk folder
>        // shortcuts which could lead to infinite cycles.
> +      if (entry.path == entry.target && entry.isDirectory()) {

Please replace "entry.path == entry.target" with !entry.isSymLink(), at the time we wrote this code it was not working properly for .lnk files, but it should have been fixed in the meanwhile.
If you could test that manually it would be much appreciated, since we don't have valid automated tests for these migrations.

Could you also please post the steps to create a lnk file that causes isDirectory to throw? Since that looks like a bug in the platform code as well.
Attachment #8360948 - Flags: review?(mak77) → review+
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Summary: Unable to Import Bookmarks from IE 10.0.7 to Firefox 22.0 → Unable to import bookmarks from IE when they contain a .lnk to a folder
Attached file CreateLNK.vbs
(In reply to Marco Bonardo [:mak] from comment #2)
> Comment on attachment 8360948 [details] [diff] [review]
> v.1 Patch
> 
> Review of attachment 8360948 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thank you, before setting the checkin-needed keyword, please correct the
> commit message so that's in the correct form:
> Bug 895025 - Unable to import bookmarks from IE when they contain a .lnk to
> a folder. r=mak
> 

Done.

> ::: browser/components/migration/src/IEProfileMigrator.js
> @@ +227,4 @@
> >  
> >        // Make sure that entry.path == entry.target to not follow .lnk folder
> >        // shortcuts which could lead to infinite cycles.
> > +      if (entry.path == entry.target && entry.isDirectory()) {
> 
> Please replace "entry.path == entry.target" with !entry.isSymLink(), at the
> time we wrote this code it was not working properly for .lnk files, but it
> should have been fixed in the meanwhile.
> If you could test that manually it would be much appreciated, since we don't
> have valid automated tests for these migrations.
> 

Actually "!entry.isSymlink()" also throws an exception as I test it:

[Exception... "Component returned failure code: 0x80070057 (NS_ERROR_ILLEGAL_VALUE) [nsINavBookmarksService.runInBatchMode]"  nsresult: "0x80070057 (NS_ERROR_ILLEGAL_VALUE)" location: "JS frame :: resource://app/components/IEProfileMigrator.js :: B_migrate :: line 217"  data: no]

As "entry.path == entry.target" works fine, I suggest keeping it this way currently.

> Could you also please post the steps to create a lnk file that causes
> isDirectory to throw? Since that looks like a bug in the platform code as
> well.

Run CreateLNK.vbs in attachment and D:\MyShortcut.LNK will be created, which links to http://www.google.com. Put it under the Favorite folder and isDirectory() would throw.

Plus, here is the exception that "entry.isDirectory()" throws:

[Exception... "Component returned failure code: 0x80520012 (NS_ERROR_FILE_NOT_FOUND) [nsINavBookmarksService.runInBatchMode]"  nsresult: "0x80520012 (NS_ERROR_FILE_NOT_FOUND)"  location: "JS frame :: resource://app/components/IEProfileMigrator.js :: B_migrate :: line 217"  data: no]
Attached patch v.1 Patch (obsolete) — Splinter Review
Attachment #8361481 - Attachment is obsolete: true
Attachment #8361483 - Flags: review?(mak77)
Comment on attachment 8361483 [details] [diff] [review]
v.1 Patch

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

::: browser/components/migration/src/IEProfileMigrator.js
@@ +227,4 @@
>  
>        // Make sure that entry.path == entry.target to not follow .lnk folder
>        // shortcuts which could lead to infinite cycles.
> +      if (entry.path == entry.target && entry.isDirectory()) {

Ok, I verified what's up, basically isSymlink fails when trying to resolve the target path, it gets an empty string out of ResolveShortcut(); and then it passes that empty string to GetFileInfo that properly throws...
The behavior is strange, though I don't know if isSymLink is expected to return properly only for "real" symlinks, it may just be late to fix its behavior and expect consumers to be ready for such a change.

So, the fix is fine, but please add a further comment above the if that states:
// Don't use isSymlink(), since it would throw for invalid
// lnk files pointing to URLs or to unresolvable paths.

Also, since other stuff may throw in the wild and we should not stop the import due to that, please wrap everything after "let entry = entries.getNext().QueryInterface(Ci.nsIFile);" in a
try {
  ...
} catch(ex) {
  Components.utils.reportError("Unable to import IE favorite (" + entry.leafName + "): " + ex);
}

So we may get bug reports telling us which favorite is broken and have it attached to the bug report more easily.
Attachment #8361483 - Flags: review?(mak77) → feedback+
Attached patch v.1 Patch (obsolete) — Splinter Review
(In reply to Marco Bonardo [:mak] from comment #6)
> Comment on attachment 8361483 [details] [diff] [review]
> v.1 Patch
> 
> Review of attachment 8361483 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/migration/src/IEProfileMigrator.js
> @@ +227,4 @@
> >  
> >        // Make sure that entry.path == entry.target to not follow .lnk folder
> >        // shortcuts which could lead to infinite cycles.
> > +      if (entry.path == entry.target && entry.isDirectory()) {
> 
> Ok, I verified what's up, basically isSymlink fails when trying to resolve
> the target path, it gets an empty string out of ResolveShortcut(); and then
> it passes that empty string to GetFileInfo that properly throws...
> The behavior is strange, though I don't know if isSymLink is expected to
> return properly only for "real" symlinks, it may just be late to fix its
> behavior and expect consumers to be ready for such a change.
> 
> So, the fix is fine, but please add a further comment above the if that
> states:
> // Don't use isSymlink(), since it would throw for invalid
> // lnk files pointing to URLs or to unresolvable paths.
> 

Done.

> Also, since other stuff may throw in the wild and we should not stop the
> import due to that, please wrap everything after "let entry =
> entries.getNext().QueryInterface(Ci.nsIFile);" in a
> try {
>   ...
> } catch(ex) {
>   Components.utils.reportError("Unable to import IE favorite (" +
> entry.leafName + "): " + ex);
> }
> 
> So we may get bug reports telling us which favorite is broken and have it
> attached to the bug report more easily.

Done.

Talking about other stuff throwing, some .url shortcuts have been found throwing too, even with this patch applied.

I will file a new bug after further verification.
Attachment #8361483 - Attachment is obsolete: true
Attachment #8364988 - Flags: review?(mak77)
Comment on attachment 8364988 [details] [diff] [review]
v.1 Patch

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

::: browser/components/migration/src/IEProfileMigrator.js
@@ +223,5 @@
>      // Until we support it, bookmarks are imported in alphabetical order.
>      let entries = aSourceFolder.directoryEntries;
> +    try {
> +      while (entries.hasMoreElements()) {
> +        let entry = entries.getNext().QueryInterface(Ci.nsIFile);

The try should be *inside* the loop, basically we only want to skip single entries, not the whole folder.

while (entries.hasMoreElements()) {
  let entry = entries.getNext().QueryInterface(Ci.nsIFile);
  try {
    ...
  } catch (ex) {
    Components.utils.reportError("Unable to import IE favorite (" + entry.leafName + "): " + ex);
  }
Attachment #8364988 - Flags: review?(mak77)
(In reply to Marco Bonardo [:mak] from comment #8)
> Comment on attachment 8364988 [details] [diff] [review]
> v.1 Patch
> 
> Review of attachment 8364988 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/components/migration/src/IEProfileMigrator.js
> @@ +223,5 @@
> >      // Until we support it, bookmarks are imported in alphabetical order.
> >      let entries = aSourceFolder.directoryEntries;
> > +    try {
> > +      while (entries.hasMoreElements()) {
> > +        let entry = entries.getNext().QueryInterface(Ci.nsIFile);
> 
> The try should be *inside* the loop, basically we only want to skip single
> entries, not the whole folder.
> 
> while (entries.hasMoreElements()) {
>   let entry = entries.getNext().QueryInterface(Ci.nsIFile);
>   try {
>     ...
>   } catch (ex) {
>     Components.utils.reportError("Unable to import IE favorite (" +
> entry.leafName + "): " + ex);
>   }

Done.
Attachment #8364988 - Attachment is obsolete: true
Attachment #8365009 - Flags: review?(mak77)
Comment on attachment 8365009 [details] [diff] [review]
0001-Bug-895025-Unable-to-import-bookmarks-from-IE-when-t.patch

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

r=me with the following addressed, then you can just set checkin-needed keyword. Thanks!

Your commit message seems to still contain [PATCH] before "bug ..."

::: browser/components/migration/src/IEProfileMigrator.js
@@ +223,5 @@
>      // Until we support it, bookmarks are imported in alphabetical order.
>      let entries = aSourceFolder.directoryEntries;
>      while (entries.hasMoreElements()) {
> +      try {
> +        let entry = entries.getNext().QueryInterface(Ci.nsIFile);

please move this out of the try, otherwise the catch cannot see the scoped entry variable
Attachment #8365009 - Flags: review?(mak77) → review+
Attached patch v.1 PatchSplinter Review
(In reply to Marco Bonardo [:mak] from comment #10)
> Comment on attachment 8365009 [details] [diff] [review]
> 0001-Bug-895025-Unable-to-import-bookmarks-from-IE-when-t.patch
> 
> Review of attachment 8365009 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the following addressed, then you can just set checkin-needed
> keyword. Thanks!
> 
> Your commit message seems to still contain [PATCH] before "bug ..."
> 
> ::: browser/components/migration/src/IEProfileMigrator.js
> @@ +223,5 @@
> >      // Until we support it, bookmarks are imported in alphabetical order.
> >      let entries = aSourceFolder.directoryEntries;
> >      while (entries.hasMoreElements()) {
> > +      try {
> > +        let entry = entries.getNext().QueryInterface(Ci.nsIFile);
> 
> please move this out of the try, otherwise the catch cannot see the scoped
> entry variable

You're right. Thanks. Done.
Attachment #8365009 - Attachment is obsolete: true
Attachment #8365083 - Flags: review?(mak77)
Comment on attachment 8365083 [details] [diff] [review]
v.1 Patch

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

Thanks!
Attachment #8365083 - Flags: review?(mak77) → review+
This is ready to land
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/059179513db9
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla29
Comment on attachment 8365083 [details] [diff] [review]
v.1 Patch

I think it's worth to uplift this to help users migrating to Firefox from IE, it's a nice gain at a low price

[Approval Request Comment]
Bug caused by (feature/regressing bug #): new IE migrator
User impact if declined: we are not properly migrating bookmarks from IE if they contain invalid entries
Testing completed (on m-c, etc.): m-c
Risk to taking this patch (and alterntives if risky): the patch is trivial and should have no risks
String or IDL/UUID changes made by this patch: none
Attachment #8365083 - Flags: approval-mozilla-aurora?
Attachment #8365083 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Target Milestone: mozilla29 → mozilla30
Component: Places → Migration
Product: Toolkit → Firefox
Hardware: x86_64 → All
Target Milestone: mozilla30 → Firefox 30
Keywords: verifyme
Blocks: 975981
I was able to confirm the fix for this issue on Windows 7 64-bit [1], using a set of bookmarks created in IE10 and .lnk file generated according to Comment 3 with:
- the latest Beta (Build ID: 20140318013849),
- the latest Aurora (Build ID: 20140324004000).

1. Mozilla/5.0 (Windows NT 6.1; WOW64; rv:30.0) Gecko/20100101 Firefox/30.0
Status: RESOLVED → VERIFIED
Keywords: verifyme
You need to log in before you can comment on or make changes to this bug.