201381 - Auto-importing IE favorites with directory entries may take a very long time, CPU usage 100%

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Brad Town]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4a) Gecko/20030408 Phoenix/0.5+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.4a) Gecko/20030408 Phoenix/0.5+

When starting with a new profile, Mozilla imports IE Favorites.  If one of the
entries in Favorites is a directory entry (such as C:\) or a network entry (such
as \\nova\files), it recurses down through these to create bookmarks for
everything below there.  This can take an incredibly long time.

I checked bugs listed under bug 120814 (the Favorites tracking bug), but I
didn't see anything that addressed this specific problem.

Reproducible: Always

Steps to Reproduce:
1. Shut down Mozilla.
2. Remove or rename your profile directory.
3. Open Windows Explorer (not Internet Explorer) and navigate to C:\.
4. Create a favorite for C:\.
5. Start Mozilla.
Actual Results:  
Mozilla appears to hang while it recurses and creates bookmarks for every
directory below the directory favorite.

Expected Results:  
Mozilla should either not recurse when it's a directory entry (local or network)
or it should warn the user that it could take a very long time.

This problem can be seen in both Mozilla and Phoenix.

Comment 1

[Brad Town]

Changed summary to reflect CPU usage of 100%; added as blocker of 120814 (the
Favorites tracking bug).

There's a workaround -- before starting Mozilla or Phoenix, move everything from
your Favorites directory (typically located at %USERPROFILE%\Favorites) to a
temporary location.  Start Mozilla or Phoenix; it won't import Favorites because
it can't find them, so it which should start right away.  Move everything back
into your Favorites folder.

Comment 2

[Vincent]

dumb question, why does Phoenix automatically import IE favorites?  Why not let
the users choose when to import them?

Phoenix is using the IE dll "shfolder.dll" do the importing, is it a wise thing
to depend on IE dlls?

I have also notice that Phoenix will not start if the "favorite" key under the
"Shell Folders" in the registry does not exist.

Comment 3

[Niklas (dacovale) Bolmdahl]

in a tread on Mozillazine, (
http://www.mozillazine.org/forums/viewtopic.php?t=11129 )
there is a workaround for this problem, that doesn't involve moving or deleting
folders. The workaround is also described in bug 205769 which was errouneously
linked as the Favorites bug.
Is this the sollution needed?

(It's well past 1 AM here, and I'm tired. Please, someone who's in a better
timezone, take a look at this. I'd try to do it myself, but I'll forget it by
the time I wake up tomorrow)

Comment 4

[(not reading, please use [e-mail] instead)]

from bug #205769, a workaround:

user_pref("browser.bookmarks.added_static_root", true);

Comment 5

[(not reading, please use [e-mail] instead)]

samir, this is the bug that asa mentioned to us today.

> from bug #205769, a workaround:
> user_pref("browser.bookmarks.added_static_root", true);

I haven't tried this, so i should say a "potential" work around.

Comment 6

[(not reading, please use [e-mail] instead)]

if we really do hang / crash, I'd say this blocker 1.4 final

asa, what do you think?

Comment 7

[Brad Town]

The "edit prefs.js" workaround works for me.  However, Firebird (the Mozilla
flavor I use) doesn't crash for me -- if I wait long enough (several minutes),
it will eventually return control.  To use the workaround, I have to kill it
myself, then edit prefs.js.  (Note: If you use either workaround described in
this bug, you won't have your IE favorites imported.  Obvious, I know, but worth
noting.)

Comment 8

[Samir Gehani]

adt: nsbeta1+/adt2

Comment 9

[Jan Varga [:janv]]

Attachment: possible fix - doesn't work, wrong boolean operator

Comment 10

[Sean Su]

Attachment: possible fix #2

The first patch didn't work.  It actually made things more broken, where it no
longer imported .url files.  It did import subfolders, but with nothing in it.

This patch is a modification of Jan's patch that has been tested under Win32. 
It fixes this bug and not regress import.

Comment 11

[Sean Su]

Comment on attachment 124571 [details] [diff] [review]
possible fix #2

turns out this patch doesn't create a bookmark for shortcut to folders.

Comment 12

[timeless]

Comment on attachment 124537 [details] [diff] [review]
possible fix - doesn't work, wrong boolean operator

>+            if (!extension.Equals(NS_LITERAL_CSTRING("url"),
>+                                  nsCaseInsensitiveCStringComparator()) ||
Negations and Boolean algebra lead to problems

continue if (not equals .url) or (not equals .lnk)

one of the conditions will *always* be true.
If you change '||' to '&&' then things should work.

>+                !extension.Equals(NS_LITERAL_CSTRING("lnk"),
>+                                  nsCaseInsensitiveCStringComparator()))

Patch tested. Resulted in empty folder hierarchy, interesting, but not useful
:-)

Notes to people thinking about the future:
1. if the bookmarks root is a drive, and the drive has a moderate amount of
directories/files then not doing the import lazily or on a thread will be bad.

2. xpcom results are not guaranteed on failure and should be checked

3. this code will run afoul real symlinks (called junctions, present in w2k+).
automated recursion is a risky proposition.

Comment 13

[Jan Varga [:janv]]

>If you change '||' to '&&' then things should work.
right, that's why I asked people to test it since I'm not able to build on
windows at the moment.

>import lazily or on a thread will be bad.
We do it lazily (look for nsIBookmarksService::readBookmarks), but that doesn't
help in this case.
Loading and importing of bookmarks on a thread is really good idea, I have a bug
for it, but that's quite risky change. We did many risky changes recently, so I
would rather wait until things settle down.

Actually, this bug is about spending CPU cycles by recursing to .lnk
directories/symlinks which are more likely not containing any .url files.
So in my opinion it's completely useless to recurse into these directories/symlinks.
Imagine you filed a bookmark pointing to C:\, current code will recurse into
this path and try to create bookmarks for all *.url files. Do you think it's
rational ?

Comment 14

[Sean Su]

I agree that we shouldn't traverse .lnk to folders.  The user created a link to
a folder.  That's what should be migrated, just the shortcut to the folder.

The original code doesn't migrate .lnk to files.  It looks like anything !.url
is skipped, so if we want to just fix the traversing problem, "possible fix #2"
should do the trick.

Comment 15

[Jan Varga [:janv]]

>The original code doesn't migrate .lnk to files.  It looks like anything !.url
>is skipped, so if we want to just fix the traversing problem, "possible fix #2"
>should do the trick.

I think that original code creates folders for .lnk files, because these files
are treated as directiories (but they are also treated as symlinks).
I'm not sure if it is possible to use ResolveShortcut() for both types of files
(.url and .lnk).

Comment 16

[Jan Varga [:janv]]

Attachment: possible fix #3

Comment 17

[Olivier Vit (just a reporter)]

*** Bug 203987 has been marked as a duplicate of this bug. ***

Comment 18

[Olivier Vit (just a reporter)]

I'm still having the issue with build 2003060208 installer-sea.exe

Removing the content of the Favorites folder or inserting
user_pref("browser.bookmarks.added_static_root", true); solves the issues, but
doesn't import any MSIE bookmarks (which is I guess a big issue)

Platform: Win 2000 SP3 and all additional updates, MS IE 5.01 SP2, Netscape 4.79

Comment 19

[Samir Gehani]

Over to Sean.

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 124630 [details] [diff] [review]
possible fix #3

Sean: is this patch what we want?  If so, let's move to get it in.  If not,
what's the path from here to what we need?

Comment 21

[Sean Su]

Comment on attachment 124571 [details] [diff] [review]
possible fix #2

I thought I had unmarked this patch as obsolete and requested r=,sr=

doing do for sure this time.  We want to go with this patch, not patch #3

Comment 22

[Sean Su]

Comment on attachment 124630 [details] [diff] [review]
possible fix #3

we don't want this patch.  it is more than what we need for 1.4, and it's not
fully working as well.

Comment 23

[Jan Varga [:janv]]

Comment on attachment 124571 [details] [diff] [review]
possible fix #2

I also think it's sufficient for 1.4
r=varga

Comment 24

[Jan Varga [:janv]]

Comment on attachment 124571 [details] [diff] [review]
possible fix #2

I also think it's sufficient for 1.4
r=varga

Comment 25

[jag (Peter Annema)]

Comment on attachment 124571 [details] [diff] [review]
possible fix #2

sr=jag

Comment 26

[Brendan Eich [:brendan]]

Doug, why should nsIFile::isDirectory and isSymlink both return true?  That
seems like a design botch to me, an artifact of over-use of stat vs. lstat in
the Unix code.  Maybe there's a better reason on Windows.  nsIFile.idl has no
doc comments before the predicate methods.

The minimal patch here looks ok for 1.4; I'll approve in a second.

/be

Comment 27

[Brendan Eich [:brendan]]

Comment on attachment 124571 [details] [diff] [review]
possible fix #2

a=brendan,rjesup for 1.4.

/be

Comment 28

[Doug Turner (:dougt)]

Brendan, an nsIFile can represent a location such as: a/b/<symlink>/c/

If you do not want to every include symlinks at all in your iteration, another
fix might just be to set the |followLinks| to false when the local file is
returned from the directory service provider.  

Comment 29

[Sean Su]

patch #2 checked in to 1.4 branch only. leaving bug open for better fix to land
on trunk.

Comment 30

[Brendan Eich [:brendan]]

>Brendan, an nsIFile can represent a location such as: a/b/<symlink>/c/

So?  That doesn't require isDirectory and isSymlink to return true for the same
given nsIFile instance, which is what I was asking about.

>If you do not want to every include symlinks at all in your iteration, another
>fix might just be to set the |followLinks| to false when the local file is
>returned from the directory service provider.  

No, followLinks is different.  The question is, when categorizing a file by its
type, can it be both a symlink and a directory (or another type of file)?  The
Unix answer is "no", but that requires you to use lstat consistently when typing
files.

Whatever the answer, we need a cross-platform solution that's documented by doc
comments in nsIFile.idl.  Is there a bug on file yet?

/be

Comment 31

[Doug Turner (:dougt)]

>So?  That doesn't require isDirectory and isSymlink to return true for the same
given nsIFile instance, which is what I was asking about.

On windows and the mac, it does.

>No, followLinks is different.

it isn't.

>Whatever the answer, we need a cross-platform solution that's documented by doc
comments in nsIFile.idl.  Is there a bug on file yet?

nope.  I think that the solution is to rip out this wannabe-like-unix automatic
"symlink" resolution on nonunix platforms.  That is, do not allow symlinks to be
in a non-termial position on these platforms.

Comment 32

[Brendan Eich [:brendan]]

>> So?  That doesn't require isDirectory and isSymlink to return true for the
>> same given nsIFile instance, which is what I was asking about.
> 
> On windows and the mac, it does.

Why?  A directory is not a shortcut on Windows, or a symlink on OS X.  The two
are different file types.  There is no confusion.

> >No, followLinks is different.
> 
> it isn't.

We are talking about nsIFile methods such as isDirectory and isSymlink, not
about nsILocalFile's followLinks attribute.
 
>> Whatever the answer, we need a cross-platform solution that's documented by
>> doc comments in nsIFile.idl.  Is there a bug on file yet?
> 
> nope.  I think that the solution is to rip out this wannabe-like-unix
> automatic "symlink" resolution on nonunix platforms.  That is, do not allow
> symlinks to be in a non-termial position on these platforms.

Why not?  How does that help this bug?  It has nothing to do with file type
being unique according to nsIFile's methods.

It would break a lot of client code that goes back before mozilla1.0.

/be

Comment 33

[Doug Turner (:dougt)]

Windows (when i said mac above i meant classic, and now realize that this
platform doesn't matter anymore) doesn't implicitly handle symlinks.  Consider
a/<symlink>/directory

On unix:
isDirectory True
isSymlink   False

on Windows:
isDirectory: True
isSymlink:   True

The "symlink" on windows is really just a special file.  When it is hit, you
manually have to do something with it.  NSPR, libc, and most of the WIN api's
will not properly handle the the "symlink" file.  So, on windows, clients need
to know if they are dealing with a path that has a .lnk or .url embedded.  The
symlink predicate returns true if *any* of the nodes in the file path are
symlinks not just the terminal.

This is different than on unix iirc for obvious reasons.

Lets not polute this bug with futher chatter about this topic.  lets create a
new bug about the platform inconsistencies regarding symlinks.

Comment 34

[Brendan Eich [:brendan]]

By all means, let's have a new bug.  Will you file it?  Just so we know what we
are talking about, though, let me ask one question here:

> Consider a/<symlink>/directory

Do you mean an nsIFile instance for the path /a/<symlink>/directory (\ for / and
c: in front on Windows, as needed)?  That nsIFile unambiguously refers to a
directory, not a symlink.  The fact that there's a symlink non-terminal pathname
component doesn't affect the type of the leaf, I hope.

/be

Comment 35

[Doug Turner (:dougt)]

it does affect the type of the leaf, and that is the problem.

Comment 36

[Sean Su]

Attachment: patch v4.0

This patch is for trunk only.  I got the link path to be of the right format,
"file:///...".	Also was able to remove the ".lnk" extension.

This patch is a modified version of Jan's "possible fix #3".

Comment 37

[Jan Varga [:janv]]

Comment on attachment 125347 [details] [diff] [review]
patch v4.0

So it turns out that we need another method to resolve lnk files.
I like this patch, nice job Sean!
r=varga

Comment 38

[Doug Turner (:dougt)]

Comment on attachment 125347 [details] [diff] [review]
patch v4.0

the local file does provide you with this support.

have you look at using the |nativeTarget|

Comment 39

[Chris Petersen]

Verified on the 2003-06-11-05 win32 branch.

Comment 40

[Sean Su]

Attachment: patch v4.1

I didn't know about nativeTarget.  This patch uses it now.

Comment 41

[Brendan Eich [:brendan]]

Comment on attachment 125465 [details] [diff] [review]
patch v4.1

Be aware that the implementations of nsIFile do not agree on whether currFile
can be both a directory and a symlink (see bug 208739).  We're likely to fix
this in a way that restores file type uniqueness.  So this patch might do
better to ask simply "is currFile a symlink", then get the target, then
consider what to do based on target type.

/be

Comment 42

[Olivier Vit (just a reporter)]

works for me with
http://ftp.mozilla.org/pub/mozilla/nightly/latest-1.4/mozilla-win32-installer-sea.exe
june 12th

Congratulations !

(but doesn't work with trunk build 1.5a june 12th, I guess it's normal?)

last comment: couldn't the dialog box for default/non default browser close
itself before the favorites get processed? - when clicking NO)

Comment 43

[Sean Su]

Attachment: patch v4.2

patch takes into account Brendan's comment #41.

Comment 44

[Sean Su]

Attachment: patch v4.3

minor cleanup given conversation with Jan.

Comment 45

[Jan Varga [:janv]]

Comment on attachment 125798 [details] [diff] [review]
patch v4.3

r=varga

Comment 46

[jag (Peter Annema)]

Comment on attachment 125798 [details] [diff] [review]
patch v4.3

+	       nsAutoString ext;
+	       ext.Assign(Substring(bookmarkName, extpos,
bookmarkName.Length()));

Shouldn't that be |..., extpos, bookmarkName.Length() - extpos|?

And no need to assign into an nsAutoString, you can do

if (Substring(bookmarkName, expos, bookmarkName.Length() -
expos).Equals(NS_LITERAL_STRING(".lnk"), nsCaseInsensitiveStringComparator()))
    bookmarkName.Truncate(extpos);


Actually, how about this?

NS_NAMED_LITERAL_STRING(lnkExt, ".lnk");
PRUint32 lnkExtLength = lnkExt.Length();
PRUint32 lnkExtStart = bookmarkName.Length() - lnkExtLength;
if (lnkExtStart > 0 &&	  Substring(bookmarkName, lnkExtStart, lnkExtLength)   
	 .Equals(lnkExt, nsCaseInsensitiveStringComparator()))
    bookmarkName.Truncate(lnkExtStart);

Comment 47

[Sean Su]

Attachment: patch v4.4

changed the .lnk extension truncation code to what jag suggests.

Comment 48

[Brendan Eich [:brendan]]

Comment on attachment 126059 [details] [diff] [review]
patch v4.4

>+            // Look for and strip out the .lnk extension.
>+            NS_NAMED_LITERAL_STRING(lnkExt, ".lnk");
>+            PRUint32 lnkExtLength = lnkExt.Length();
>+            PRUint32 lnkExtStart = bookmarkName.Length() - lnkExtLength;
>+            if (lnkExtStart > 0 &&

What if bookmarkName.Length() is less than 4?  Is that impossible?

You want signed integral type for lnkExtStart, and I don't see a need for
lnkExtLength, myself.

/be

Comment 49

[Sean Su]

Attachment: patch v4.5

> What if bookmarkName.Length() is less than 4?  Is that impossible?

I can't think of a case where it would be < 4, but I'm sure it's not
impossible.  It also wouldn't hurt to use a signed type.

> You want signed integral type for lnkExtStart, and I don't see a need for
> lnkExtLength, myself.

changed lnkExtStart to a signed integral type and removed lnkExtLength.

Comment 50

[jag (Peter Annema)]

Comment on attachment 126082 [details] [diff] [review]
patch v4.5

"whoops" on the PRUint32 vs PRInt32.

+	     if (lnkExtStart > 0 && Substring(bookmarkName, lnkExtStart,
lnkExt.Length())   

+		    .Equals(lnkExt, nsCaseInsensitiveStringComparator()))


That looks a bit odd with the blank line.

Btw, I changed |StringBeginsWith| and |StringEndsWith| to take a
StringComparator parameter, so you could now write this as:

	    if (lnkExtStart > 0 &&
		StringEndsWith(bookmarkName, lnkExt,			       
    nsCaseInsensitiveStringComparator()))
		bookmarkName.Truncate(bookmarkName.Length() - lnkExt.Length());

sr=jag with that change.

Comment 51

[jag (Peter Annema)]

Comment on attachment 126082 [details] [diff] [review]
patch v4.5

Erh, minus the |lnkExtStart > 0 &&| part, StringEndsWith deals with that for
you.

Comment 52

[Sean Su]

Attachment: patch v4.6

updated patch.

Comment 53

[Sean Su]

patch v4.6 tested and checked in to trunk.