238136 - [w95/w95a][nt 3.51] Bookmarks don't work

Status: RESOLVED FIXED

(SeaMonkey :: Bookmarks & History, defect)

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

nsBookmarksService.cpp:5221 NS_ENSURE_TRUE failed
http://lxr.mozilla.org/seamonkey/source/xpfe/components/bookmarks/src/nsBookmarksService.cpp#5221

Comment 1

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch

Comment 2

[[e-mail]]

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

Comment 3

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 144428 [details] [diff] [review]
patch

to clarify - I have tested the patch now :) it works on both win2k (importing
system favorites works), and nt 3.51 (bookmarks work)

Comment 4

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: diff -w version, FOR REVIEW ONLY

this is probably easier to review

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 144428 [details] [diff] [review]
patch

oops, this only compiles on windows

Comment 6

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v1.1

Comment 7

[Christian :Biesinger (don't email me, ping me on IRC)]

Attachment: patch v1.1 -w FOR REVIEW ONLY

Comment 8

[[e-mail]]

In fact there is a logic error in nsBookmarksService.cpp - as you know system
favourites are imported lazily, and the code in ImportSystemBookmarks already
checks that the favourites folder exists (w95/w95a systems don't have IE3).
Unfortunately as we have seen the code in LoadBookmarks does not.

Comment 9

[Christian :Biesinger (don't email me, ping me on IRC)]

Comment on attachment 144476 [details] [diff] [review]
patch v1.1

ok, this patch obviously doesn't work.

Comment 10

[[e-mail]]

Attachment: Possible patch

I couldn't figure out a neat way to do line 5300... suggestions?

Comment 11

[Christian :Biesinger (don't email me, ping me on IRC)]

nice, the patch looks good

what's so special about line 5300?

Comment 12

[Jan Varga [:janv]]

Comment on attachment 144661 [details] [diff] [review]
Possible patch

r=varga
So, you basically changed the behaviour to not exit with an error when there's
no system bookmarks folder on windows.

by line 5300 Neil probably meant this:
+#ifdef XP_WIN
+    if (systemBookmarksFolder)
+#endif
     if (!addedStaticRoot)
     {

Comment 13

[Christian :Biesinger (don't email me, ping me on IRC)]

-> patch author

Comment 14

[jag (Peter Annema)]

Comment on attachment 144661 [details] [diff] [review]
Possible patch

>Index: xpfe/components/bookmarks/src/nsBookmarksService.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/xpfe/components/bookmarks/src/nsBookmarksService.cpp,v
>retrieving revision 1.313
>diff -p -u -r1.313 nsBookmarksService.cpp
>--- xpfe/components/bookmarks/src/nsBookmarksService.cpp	9 Mar 2004 19:33:48 -0000	1.313
>+++ xpfe/components/bookmarks/src/nsBookmarksService.cpp	24 Mar 2004 14:09:30 -0000
>@@ -5213,29 +5213,31 @@ nsBookmarksService::LoadBookmarks()
> #endif
> 
> #if defined(XP_WIN) || defined(XP_BEOS)
>-    nsCOMPtr<nsIRDFResource> systemFavoritesFolder;
>     nsCOMPtr<nsIFile> systemBookmarksFolder;
> 
> #if defined(XP_WIN)
>     rv = NS_GetSpecialDirectory(NS_WIN_FAVORITES_DIR, getter_AddRefs(systemBookmarksFolder));
>-    NS_ENSURE_SUCCESS(rv, rv);
> #elif defined(XP_BEOS)
>     rv = NS_GetSpecialDirectory(NS_BEOS_SETTINGS_DIR, getter_AddRefs(systemBookmarksFolder));
>-    NS_ENSURE_SUCCESS(rv, rv);
>-    rv = systemBookmarksFolder->AppendNative(NS_LITERAL_CSTRING("NetPositive"));
>-    NS_ENSURE_SUCCESS(rv, rv);
>+
>+    if (NS_SUCCEEDED(rv))
>+        rv = systemBookmarksFolder->AppendNative(NS_LITERAL_CSTRING("NetPositive"));
>    
>-    rv = systemBookmarksFolder->AppendNative(NS_LITERAL_CSTRING("Bookmarks"));
>-    NS_ENSURE_SUCCESS(rv, rv);
>+    if (NS_SUCCEEDED(rv))
>+        rv = systemBookmarksFolder->AppendNative(NS_LITERAL_CSTRING("Bookmarks"));
>
> #endif
> 
>     nsCOMPtr<nsIURI> bookmarksURI;
>     rv = NS_NewFileURI(getter_AddRefs(bookmarksURI), systemBookmarksFolder);

So by removing the above NS_ENSURE_SUCCESSes, at this point you might be
constructing a new file URI from an "empty" nsIFile. I'm not sure what happens
in that case. A quick glance suggests the .spec will be "file:///", pointing to
the root directory. Not sure that's what you want. I doubt
NS_GetSpecialDirectory would fail, but ... I'd suggest adding |if
(NS_SUCCEEDED(rv))| to the NS_NewFileURI line.

>@@ -5245,7 +5247,8 @@ nsBookmarksService::LoadBookmarks()
> #if defined(XP_MAC) || defined(XP_MACOSX)
>             parser.SetIEFavoritesRoot(nsCString(kURINC_IEFavoritesRoot));
> #elif defined(XP_WIN) || defined(XP_BEOS)
>-            parser.SetIEFavoritesRoot(bookmarksURICString);
>+            if (systemFolderResource)
>+                parser.SetIEFavoritesRoot(bookmarksURICString);

This had me puzzled for a while. I guess this code would be a bit more clear to
me if it read:

    if (!bookmarksURICString.Empty())
	parset.SetIEFavoritesRoot(bookmarksURICString);

Or you could add a #if defined(XP_MAC) || defined(XP_MACOSX) section a little
higher up with |nsCAutoString bookmarksURICString(kURINC_IEFavoritesRoot);| and
get rid of these #ifdefs.

>@@ -5292,6 +5295,9 @@ nsBookmarksService::LoadBookmarks()
> 
>     // Add the root that System bookmarks are imported into as real bookmarks. This is 
>     // only done once. 
>+#ifdef XP_WIN
>+    if (systemBookmarksFolder)
>+#endif
>     if (!addedStaticRoot)
>     {

And this could also check for !bookmarksURICString.Empty() and get rid of the
#ifdef.

>         nsCOMPtr<nsIRDFContainer> rootContainer(do_CreateInstance(kRDFContainerCID, &rv));
>@@ -5329,8 +5335,6 @@ nsBookmarksService::LoadBookmarks()
> 
>             nsCOMPtr<nsIRDFResource> systemFolderResource;
> #if defined(XP_WIN)
>-            rv = gRDF->GetResource(bookmarksURICString,
>-                                   getter_AddRefs(systemFolderResource));
> #elif defined(XP_MAC) || defined(XP_MACOSX)
>             rv = gRDF->GetResource(NS_LITERAL_CSTRING(kURINC_IEFavoritesRoot),
>                                    getter_AddRefs(systemFolderResource));

Hmm, did you mean to remove this? This will leave systemFolderResource a null
pointer for XP_WIN (it hides the one you added above), in which case I'm
guessing the RemoveElement call below this will fail. If this was supposed to
stay, now that bookmarksURICString == kURINC_IEFavoritesRoot for the XPMAC(OSX)
cases, you could get rid of the #ifdefs. Ah, which makes it pretty much be the
systemFolderResource code you moved higher up into that XP_WIN|BEOS only
section. Move that out and I guess you could get rid of that code here.

Ugh, so much code is really the same in this file, but pretends to be
different. We should really aim to get rid of more #ifdefs.

Comment 15

[jag (Peter Annema)]

Heh... *blush* s/\.Empty/.IsEmpty/g

Comment 16

[jag (Peter Annema)]

Attachment: Patch based on my comments, not for review yet

Comment 17

[jag (Peter Annema)]

Comment on attachment 153277 [details] [diff] [review]
Patch based on my comments, not for review yet

>+            if (!bookmarksURICString.IsEmpty()) {
>+                parser.SetIEFavoritesRoot(bookmarksURICString);
>+                parser.ParserFoundIEFavoritesRoot(&foundIERoot);
>+            }

I'm just guessing that if I don't SetIEFavoritesRoot,
ParserFoundIEFavoritesRoot will just return false. Fair assumption? Or should
we always check?

>@@ -5328,21 +5336,13 @@
>         // only to add confusion. 
>         if (!useDynamicSystemBookmarks)
>         {
>+            // XXXjag could this reuse rootContainer?
>             nsCOMPtr<nsIRDFContainer> container(do_CreateInstance(kRDFContainerCID, &rv));

I don't think we actually need to create a new rdf container here, we could
just reuse the one created right above this, right?

Comment 18

[[e-mail]]

Attachment: Addressing some review comments

OK, this simplifies things a bit, and fixes the issues that jag found.
I didn't want to "simplify" things not directly touched by my patch.

Comment 19

[jag (Peter Annema)]

Comment on attachment 154974 [details] [diff] [review]
Addressing some review comments

sr=jag

Comment 20

[[e-mail]]

Fix checked in, with Jan's r= over IRC.