145858 - Imported NetPositive Bookmarks are not loaded properly anymore

Status: RESOLVED FIXED

(Core Graveyard :: RDF, defect)

Description

[Paul]

Something changed between teh 6th and the 8th of May, that broke the display of imported
Net+ bookmarks.  Here's a list of changes to cvs for that time perios:

http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=MozillaTinderboxAll&branch=
HEAD&branchtype=match&dir=&file=&filetype=match&who=&whotype=match&sortby=Date&hours=2&date
=explicit&mindate=5%2F6%2F2002&maxdate=5%2F8%2F2002&cvsroot=%2Fcvsroot

I believe it is something in the xpcom/io directory, but I have not narrowed it down yet.
It should not be hard to do so, however, so, I will hopefully have a fix soon.

Comment 1

[Sergei Dolgov]

Chris, this problems sees in your competence.
I noticed lot of your checkins in
nsBookmarksService 
and
nsFileSystemDataSource
in beginnin of May 2002, when Net+ bookmarks were broken.
I suspect that reason may be related to all this dance around  UC2/UTF-8

Comment 2

[Sergei Dolgov]

nsresult FileSystemDataSource::getNetPositiveURL in
rdf/datasource/src/nsFileSystemDataSource.cpp needs rewrote:
1)Check-in for XP_WIN
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/xpfe/components/bookmarks/src&command=DIFF_FRAMESET&file=nsBookmarksService.cpp&rev1=1.238&rev2=1.239&root=/cvsrootv
breaks BeOS functionality (they even didn't put if define(XP_WIN) in generic code!
2) Even if this check-in is rolle back, there is still problem in
getNetPositiveURL: 

constructor BFile   bf(nativeURI, B_READ_ONLY); gets path in Escaped form, when
it hsould be pure UTF-8 (native BeOS encoding).

Comment 3

[hacker formerly known as [e-mail]]

I'm confused by your previous post but to clarify the situation, my checkins to
those files were for a) a global obsolete macro cleanup and b) to checkin
Takashi Toyoshima's changes to actually use the netPositive import routines. 
Both of which occurred long before May 2002.  

It does look like the nsIFile changes triggered the problem though.  Cc'ing a
couple of people who would understand it better than I.  The changes that you
pointed out appear to only occur inside a XP_WIN ifdef so I'm confused as to how
they break BeOS.  Or did you mean that we need to make similar changes to the
BeOS routines to correct the problem?

Comment 4

[Sergei Dolgov]

hey Chris

1)i'm sorry. #endif before :ParseFavoritesFolder closed #if _MSC_VER >= 1200
case, not XP_WIN case, as i thought

2)yeah i mean't that there is need to check effect of Bug 142870 on BeOS. Seems
titanic work, but i have feeling that this is quite important for other BeOS bugs.

3)Coffee instead sleeping is evil - now i had working Net+ bookmarks, but
besides hacks  in FileSystemDataSource::getNetPositiveURL() i don't know now
what rollback certainly helped me to restore its functionality. Will try
diff-ing again this night/

Comment 5

[Sergei Dolgov]

Attachment: patch #1 - nsBookmarksService.cpp

sets useDynamicSystemBookmarks = PR_TRUE; by default in BeOS
It was broken by adding 
   if (bookmarksPrefs)
     bookmarksPrefs->GetBoolPref("import_system_favorites",
&useDynamicSystemBookmarks);
AFTER #ifdef XP_BEOS
see comments inside patch. Other soultion is follow bug 22642 and create item
in BookmarkManager->View aslo for BeOS build
patch for FileSystemDataSource::getNetPositiveURL() will follow

Comment 6

[Sergei Dolgov]

second notice about - nsBookmarksService.cpp - CRITICAL!
checking
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&subdir=mozilla/xpfe/components/bookmarks/src&command=DIFF_FRAMESET&file=nsBookmarksService.cpp&rev1=1.237&rev2=1.238&root=/cvsroot


for bug 129428 is main break reason - he breaks proper Net+ folder scanning by
assigning blindly file///: to all entries in NetPositive/Bookmarks folder, and
preventing from call to GetUrl->GetNetpositiveURL from BFile "META:url" attribute.
I fixed it for BeOS but need contact to above checkin author
Also dependency on bug 129428 must be set

Comment 7

[Sergei Dolgov]

Attachment: simplest patch

patch.

Comment 8

[Sergei Dolgov]

additional patch for FileSystemDataSource::getNetPositiveURL
is needed to fix handling of Net+ bookmarks files with non-ASCII-7 names.
(Unescape())

Comment 9

[Sergei Dolgov]

Ben,
can you verify/confirm that patch
http://bugzilla.mozilla.org/attachment.cgi?id=87612&action=view
does not break anything for other platforms? 

Comment 10

[Paul]

Ok, this seems to work, but, BeOS will now not be able to support the
"import_system_favorites" preference.  Could we not default it to true,
in the default prefs file that is distributed with mozilla for BeOS?
Just an idea.

Comment 11

[Sergei Dolgov]

hm, problem is that this option in UI exists only in Win/Mac XUL-s/jar-s.
Until someone create proper UI module for BeOS, i prefer to forget about this
option in preferences :) Though, it may be set for BeOS by default in all.js or
BookmarkOverlay.js wherever else. Just in order to inform people about situation.

Comment 12

[Sergei Dolgov]

Attachment: i18n fix

Allows to get URL from bookmark files with non-ascii characters.

Comment 13

[Sergei Dolgov]

There is another problem now with N+ bookmarks.
Search don't work for those Net+ Imported bookmarks.
Dunno if it worked before break.
so, BookmarkService needs revisiting, but maybe problem is on JS code.
Paul, can you check it, and what do you think, will it be another bug?

Comment 14

[Paul]

Question:  Does the search work under the Windows version?  
If not, we probably don't need to worry about it.  Especially since they are
Net+ bookbark files, which are native files under BeOS, and cn be queried on the file
system :)

Comment 15

[Sergei Dolgov]

Do you mean search in "Imported IE Favorites"?
I cannot check it, because i haven't IE  here (all 3 machines with
Win98Lite/SLEEK). So no subject to import

Comment 16

[Sergei Dolgov]

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

Comment 17

[Sergei Dolgov]

Attachment: Restyled patch for nsBookmarkService

Comment 18

[Sergei Dolgov]

Comment on attachment 87821 [details] [diff] [review]
i18n fix

Not needed if we commit patch for bug 158092.
That's more general solution.

Comment 19

[Sergei Dolgov]

Attachment: mv tabs /dev/null

additional cleanup

Comment 20

[timeless]

Comment on attachment 91857 [details] [diff] [review]
mv tabs /dev/null

whitespace nightmares, don't worry, r=timeless
but this:
+	else
+	{
+#if defined(XP_BEOS)
should be
+    else
+    {
+#ifdef XP_BEOS
(the ifdef change is for consistency, the spaces are slightly more important)

Comment 21

[Asa Dotzler [:asa]]

Comment on attachment 91857 [details] [diff] [review]
mv tabs /dev/null

a=asa (on behalf of drivers) for checkin to 1.1

Comment 22

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

checked in

Comment 23

[Kai Lahmann (is there, where MNG is)]

v

Comment 24

[Robert John Churchill]

Attachment: GetSynthesizedType() must be able to return NULL on non-BeOS platforms.

Sorry, but the nsBookmarksService.cpp patch needs to be restructured slightly
(due to aggregation).  I'm re-opening this bug and attaching a slightly
modified fix... can someone with BeOS test to make sure that it doesn't regress
please?  Thanks!

Comment 25

[Robert John Churchill]

Re-opening...

Comment 26

[Sergei Dolgov]

No regression. It works. And i can say more - it seems that it works even
without  #ifdef XP_BEOS part now. Though, i tested this patch with original 1.1b
release source, so i will be very grateful if you point me to another patches
since 1.1b milestone in nsBookmarkService.cpp - it those exist.

Now i prefer to let this #ifdef XP_BEOS to stay in code - for safety reason.

Comment 27

[Robert John Churchill]

Sergei: Thanks for testing!  If it works without the #ifdef XP_BEOS section,
that section should be removed as the patch enables internet search & bookmark
find queries which are saved into bookmarks to work.  :)  Agreed?

Comment 28

[Sergei Dolgov]

Sorry Robert. Little confusing happened. It seems that patch was applied
incorectly in my old sources.
When i apllied it manually, i got crash when attempting to reach Imported
Bookmarks in Bookmark manager:
loading symbols
segment violation occurred
nsBookmarksService::GetSynthesizedType(nsIRDFResource *, nsIRDFNode **):
GetSynthesizedType__18nsBookmarksServiceP14nsIRDFResourcePP10nsIRDFNode:
+00b8  ece26df8:   *          0a8b    movl    (%edx), %ecx
mozilla-bin:sc
   frame         retaddr
fcffaf08   ece2741a  nsBookmarksService::GetTarget(nsIRDFResource *,
nsIRDFResource *, int, nsIRDFNode **) + 000001da
fcffafd4   ed89e3a4  CompositeDataSourceImpl::GetTarget(nsIRDFResource *,
nsIRDFResource *, int, nsIRDFNode **) + 000000b4
fcffb004   edc07361  nsTemplateRule::ComputeAssignmentFor(nsConflictSet &,
nsTemplateMatch *, int, Value *) const + 000000d1
fcffb064   edc064e0  nsTemplateMatch::GetAssignmentFor(nsConflictSet &, int,
Value *) + 00000040
fcffb08c   edc1ce32 
nsXULTemplateBuilder::SubstituteTextReplaceVariable(nsXULTemplateBuilder *,
nsAString const &, void *) + 00000172
fcffb16c   edc1c817 
ParseAttribute__20nsXULTemplateBuilderRC9nsAStringPFP20nsXULTemplateBuilderRC9nsAStringPv_vT2Pv
+ 00000547 

So we should wait until someone (timeless, arougthopher or cls) can confirm or
reject it on up-to-date source tree for trunk. I hardly can update and build
BeOS Mozilla before Monday. But if i do it faster - i definitely check this problem

Comment 29

[Sergei Dolgov]

my friend with 4-day old sources also got
Hunk #1 FAILED at 3077.
1 out of 1 hunk FAILED -- saving rejects to file nsBookmarksService.cpp.rej: 
***************
*** 3077,3103 ****
      // if we didn't match anything in the graph, synthesize its type
      // (which is either a bookmark or a bookmark folder, since everything
      // else is annotated)
      PRBool isContainer = PR_FALSE;
      (void)gRDFC->IsSeq(mInner, aNode, &isContainer);

      if (isContainer)
      {
        *aType =  kNC_Folder;
      }
-     else
      {
  #ifdef XP_BEOS
        //solution for BeOS - bookmarks are stored as file attributes.
-       PRBool isBookmarkedFlag = PR_FALSE;
-       rv = IsBookmarkedInternal(aNode, &isBookmarkedFlag);
-       if (!isBookmarkedFlag || NS_FAILED(rv) || (rv == NS_RDF_NO_VALUE))
-         *aType = kNC_URL;
-       else
- #endif
-        *aType = kNC_Bookmark;
      }
-     NS_ADDREF(*aType);
    }
    return(NS_OK);
  }

--- 3077,3104 ----
      // if we didn't match anything in the graph, synthesize its type
      // (which is either a bookmark or a bookmark folder, since everything
      // else is annotated)
      PRBool isContainer = PR_FALSE;
+     PRBool isBookmarkedFlag = PR_FALSE;
      (void)gRDFC->IsSeq(mInner, aNode, &isContainer);

      if (isContainer)
      {
        *aType =  kNC_Folder;
      }
+     else if (NS_SUCCEEDED(rv = IsBookmarkedInternal(aNode,
+       &isBookmarkedFlag)) && (isBookmarkedFlag == PR_TRUE))
      {
+        *aType = kNC_Bookmark;
+     }
  #ifdef XP_BEOS
+     else
+     {
        //solution for BeOS - bookmarks are stored as file attributes.
+       *aType = kNC_URL;
      }
+ #endif
+     NS_IF_ADDREF(*aType);
    }
    return(NS_OK);
  }
So question is:
is it trunk or branch patch?

Comment 30

[Sergei Dolgov]

Now it looks like this in CVS. someone wiped all if-else conditions together
with BeOS case:
nsresult
nsBookmarksService::GetSynthesizedType(nsIRDFResource *aNode, nsIRDFNode **aType)
{
  *aType = nsnull;
  nsresult rv = mInner->GetTarget(aNode, kRDF_type, PR_TRUE, aType);
  if (NS_FAILED(rv) || (rv == NS_RDF_NO_VALUE))
  {
    // if we didn't match anything in the graph, synthesize its type
    // (which is either a bookmark or a bookmark folder, since everything
    // else is annotated)
    PRBool isContainer = PR_FALSE;
    (void)gRDFC->IsSeq(mInner, aNode, &isContainer);

    *aType = isContainer ? kNC_Folder : kNC_Bookmark;
      NS_ADDREF(*aType);
  }
  return(NS_OK);
} 

Comment 31

[Robert John Churchill]

Try this  (with/without the XP_BEOS):


nsresult
nsBookmarksService::GetSynthesizedType(nsIRDFResource *aNode, nsIRDFNode **aType)
{
  *aType = nsnull;
  nsresult rv = mInner->GetTarget(aNode, kRDF_type, PR_TRUE, aType);
  if (NS_FAILED(rv) || (rv == NS_RDF_NO_VALUE))
  {
    // if we didn't match anything in the graph, synthesize its type
    // (which is either a bookmark or a bookmark folder, since everything
    // else is annotated)
    PRBool isContainer = PR_FALSE;
    PRBool isBookmarkedFlag = PR_FALSE;
    (void)gRDFC->IsSeq(mInner, aNode, &isContainer);

    if (isContainer)
    {
      *aType =  kNC_Folder;
    }
    else if (NS_SUCCEEDED(rv = IsBookmarkedInternal(aNode,
      &isBookmarkedFlag)) && (isBookmarkedFlag == PR_TRUE))
    {
       *aType = kNC_Bookmark;	
    }
#ifdef XP_BEOS
    else
    {
      //solution for BeOS - bookmarks are stored as file attributes. 
      *aType = kNC_URL;
    }
#endif
    NS_IF_ADDREF(*aType);
  }
  return(NS_OK);
}

Comment 32

[Sergei Dolgov]

Hello Robert.
I succeed with update and build sooner than expected. Here is my test results.
I put in proposed code debug statements:
--------------------------
    else if (NS_SUCCEEDED(rv = IsBookmarkedInternal(aNode,
      &isBookmarkedFlag)) && (isBookmarkedFlag == PR_TRUE))
    {
fprintf(stdout,"kNC_Bookmark\n");
       *aType = kNC_Bookmark;	
    }
#ifdef XP_BEOS
    else
    {
      //solution for BeOS - bookmarks are stored as file attributes. 
fprintf(stdout,"BeOS - kNC_URL\n");      
      *aType = kNC_URL;
    }
#endif
fflush(stdout);
    NS_IF_ADDREF(*aType);
-----------------------------
And always when i reach Imported NetPositive bookmarks, i get in stdout:

BeOS - kNC_URL 

It means, that second condition fails for BeOS and if we ecxlude XP_BEOS case,
we'll get: *aType = nsnull; and, as result, NS_IF_ADDREF(null);
For some reason it worked, but seems wrong and i got crash on exit with some
hashtable, when excluded XP_BEOS case.
So we should save this as fallback option, though, it seems not preventing
"internet search & bookmark find queries which are saved into bookmarks to
work." - as Mozilla saves those queries in its own format, so it'll be under
first or second conditions.

Comment 33

[Robert John Churchill]

OK, sounds fine to me.  Let's get reviews of the code (with #ifdef XP_BEOS) in
comment 31 so the fix can be checked in.

Comment 34

[Robert John Churchill]

BTW:  "NS_IF_ADDREF(null);"  is fine, as it addrefs only if the in-param is
non-null.

Comment 35

[Sergei Dolgov]

if there will be appliable patch, i can review it from BeOS POV.

Comment 36

[Robert John Churchill]

Sergei:  the last attached patch to this bug is applicable to the tip.

Comment 37

[Sergei Dolgov]

Comment on attachment 95650 [details] [diff] [review]
GetSynthesizedType() must be able to return NULL on non-BeOS platforms.

ok
r= [e-mail]

Comment 38

[Robert John Churchill]

Still need an sr= on this one.

Comment 39

[Darin Fisher]

Comment on attachment 95650 [details] [diff] [review]
GetSynthesizedType() must be able to return NULL on non-BeOS platforms.

sr=darin

Comment 40

[Robert John Churchill]

Thank, Darin and Sergei.  Fix checked in.

Comment 41

[Sergei Dolgov]

Broken again

Comment 42

[Doug Turner (:dougt)]

probably due to my latest changes. :-/  sorry.

Comment 43

[Sergei Dolgov]

nsFileSystemDataSource.cpp
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsFileSystemDataSource.cpp&root=/cvsroot&subdir=mozilla/rdf/datasource/src&command=DIFF_FRAMESET&rev1=1.119&rev2=1.120

nsXPIDLCString nativeURI;  declared but never and nowhere assigned for XP_BEOS

Comment 44

[Sergei Dolgov]

tested with last cls fixes. No help. Because we still use nativeURI without
assignment of any real value

Comment 45

[Sergei Dolgov]

Attachment: patch - restores N+ bookmark import

fixed initialisation of BFile object and tests for NetPositive bookmarks path

Comment 46

[Sergei Dolgov]

Attachment: patch - added get()

same as previous - added get() to obtain char * from string.

Comment 47

[Sergei Dolgov]

Comment on attachment 118069 [details] [diff] [review]
patch - added get()

review request

Comment 48

[Sergei Dolgov]

fixed