424769 - Smart bookmarks Recent Tags menu contains self reference (PPC Mac)

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P2)

Description

[Christiaan Hofman]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032304 Minefield/3.0b5pre
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9b5pre) Gecko/2008032304 Minefield/3.0b5pre

The Smart Bookmarks menu really is running crazy. In particular the Recent Tags item always has a "(no title)" submenu, which really gets out of bounds. Submenus go infinitely deep, and some submenus refer back to the Smart Bookmarks. This leads to circular menus and a serious hang on Mac OS X (see bug 400291). 

Reproducible: Always

Steps to Reproduce:
1. Click Smart Bookmarks, 
2. Select Recent Tags
3. Select (no title)
4. Select Bookmarks Toolbar
5. Select Smart Bookmarks
6. Select Recent Tags
7. ...

Actual Results:  
I can go on as long as I want

Expected Results:  
Submenus should never go to a very deep level. And certainly not be self referring. 

Moreover, I fail to see the relevance of the whole (no title) submenu of the Recent Tags, and in fact what they have to do with Recent Tags. It should just be removed. This is very bad UI. For the rest the Smart Bookmarks is a great addition.

Comment 1

[Christiaan Hofman]

It seems to me that this is unintended. It also apparently does not appear on some computers (apparently it appears on Macbooks, but not on Powerbooks). So let me list what I see for the Recent Tags hierarchy:

Recent Tags
  (no title)
    Tags
      ...
    Unfiled Bookmarks
      ...
    Bookmarks Toolbar
      ...
    Bookmarks Menu
      ...
    (no title)
      ...
    (no title)
      ...

Submenus go on foir a while (infinitely). I'm pretty sure that the content of the Tags submenu should actually be the content of the Recent Tags menu. It contains submenus for each recently added tag, containing the bookmarks with that tag. All the rest of the (sub)menus shouldn't be there. The location for the Recent Tags item is place:type=6&sort=14&maxResults=10 which should be correct. 

Comment 2

[Christiaan Hofman]

Raised to critical, as it leads to a never ending hang on Mac OS X 10.5. 

Comment 3

[Christiaan Hofman]

Changed component to Places, as I believe this is caused by a bug in following the query. The menus being wrong is only a result from that.

Comment 4

[Marcia Knous [:marcia]]

Confirming using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008032804 Minefield/3.0pre. Nominating since we should have parity with what shows appears Intel Mac. I am testing on a PPC Mac G5 Dual 1.8 GHz.

Comment 5

[Christiaan Hofman]

Steven Michaud in the bug 400291 tracker suggested this might be due to a big-endian/little-endian issue in accessing 32 bit integers somewhere. Seems plausible to me. 

And I'd say having parity between different Macs is about the least relevant reason to nominate this, there are issues with this that are much more critical (compare bug 400291 and bug 424884). 

Comment 6

[Christiaan Hofman]

This issue should have definitely made the Known Issues for FF 3.0b5, if not downright be a showstopper.

Comment 7

[Mike Connor [:mconnor]]

er, that seems really broken.  Can you first back up your current bookmarks and then reset browser.places.createdSmartBookmarks and see if the recreated smart bookmarks exhibit this behaviour?

Also, the bug title has PPC Mac, but a comment says it happens on macbooks, not powerbooks...

Comment 8

[Christiaan Hofman]

> er, that seems really broken.  Can you first back up your current bookmarks and
> then reset browser.places.createdSmartBookmarks and see if the recreated smart
> bookmarks exhibit this behaviour?

That's the problem: it's in the Smart Bookmarks menu generated by FF/Minefield. You can also compare the location I gave above, and see it's the one generated by FF. 

> Also, the bug title has PPC Mac, but a comment says it happens on macbooks, not
> powerbooks...

Sorry, that's a typo. It should be reversed. 

Comment 9

[Dietrich Ayala (:dietrich)]

From comment #0, it seems that you've tagged the Toolbar folder in the left pane of the organizer. Can you click on that folder, and check the details pane to see if is tagged?

The "no title" tag container is mysterious though. Bug 424884 will have fixed the infinite menu portion of this.

However, we should not be able to get to a state where a tag container has no title.

Comment 10

[Dietrich Ayala (:dietrich)]

Marcia, did you confirm this with a new profile? If you can reproduce with a new profile, please attach the places.sqlite file.

Comment 11

[Marcia Knous [:marcia]]

Attachment: places.sqlie file from a New profile

Here is my file from the latest nightly, using a fresh profile. Note you don't have to do anything to get the Recent Tags to show the "No Title" it happens out of the box.

Comment 12

[Christiaan Hofman]

(In reply to comment #9)
> From comment #0, it seems that you've tagged the Toolbar folder in the left
> pane of the organizer. Can you click on that folder, and check the details pane
> to see if is tagged?
> 

No and no. This has nothing to do with anything *I* did, it is generated this way by FF. This is confirmed by several users on PPC macs. 

> The "no title" tag container is mysterious though. Bug 424884 will have fixed
> the infinite menu portion of this.
> 

That's incorrect, this is not fixed by adding excludeQueries (I said this somewhere else, either in this bug or a related one).

> However, we should not be able to get to a state where a tag container has no
> title.
> 

The point is that the whole Recent Tags menu is garbage (on PPC macs). The generated content has no relationship with tags whatsoever. So the question is not whether the query is right or not, but rather the parsing code has some more serious bug, probably involving a big-endian/little-endian issue or something.

> Marcia, did you confirm this with a new profile? If you can reproduce with a
> new profile, please attach the places.sqlite file.

No point in checking this, as I've done that lots of times now, as well as other users. And it's not relevant to the problem, because it's not the query that's wrong.

Comment 13

[Christiaan Hofman]

(In reply to comment #12)
> (In reply to comment #9)
 
> > The "no title" tag container is mysterious though. Bug 424884 will have fixed
> > the infinite menu portion of this.
> > 
> 
> That's incorrect, this is not fixed by adding excludeQueries (I said this
> somewhere else, either in this bug or a related one).
> 

The infinite menu indeed seems to have gone. But the menu is still a mess, and the repetitive inclusion of large menus (such as the bookmarks menu) leads to a serious slow down in showing the Help menu.

Comment 14

[Marco Bonardo [:mak]]

we have a new query for tag containers (at least for the smart query, temporary we have the fix in the Library only for new profiles). This new query is a real query, so should not allow anything created inside it that is not created through the tagging service.
So please, try this with nightly with fix for bug 419731 and a NEW profile, and report if still an issue.

Comment 15

[Christiaan Hofman]

(In reply to comment #14)
> we have a new query for tag containers (at least for the smart query, temporary
> we have the fix in the Library only for new profiles). This new query is a real
> query, so should not allow anything created inside it that is not created
> through the tagging service.
> So please, try this with nightly with fix for bug 419731 and a NEW profile, and
> report if still an issue.
> 

Here's my result with the latest nightly Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041104 Minefield/3.0pre, and a new profile. The Recent Tags item still has the same query (place:type=6&sort=14&maxResults=10) and is still the same mess (though without the self reference, as in my Comment # 13). 

Comment 16

[Marco Bonardo [:mak]]

yes, query has not changed, still subqueries (aka tag containers) are no more folders. Results should come out directly from sqlite and simply filtered out to remove dupes. Do you see results that have not been tagged at all, or missing results?

Comment 17

[Christiaan Hofman]

(In reply to comment #16)
> yes, query has not changed, still subqueries (aka tag containers) are no more
> folders. Results should come out directly from sqlite and simply filtered out
> to remove dupes. Do you see results that have not been tagged at all, or
> missing results?
> 

As I said, what I see is the mess I've described in Comment #1. Not a list of tagged items or anything. This has not changed. So even though the mess is not a critical mess anymore, it's still a mess.

Comment 18

[Marco Bonardo [:mak]]

from there feels like your system is returning the library left pane folder instead of the tag root :\

Comment 19

[Marco Bonardo [:mak]]

i've done some testing in a pearPC emulated machine, i can use minefield and i see the (no title) folder under Recent Tags. I'm not still able to compile there but it's a first step. 

From first analysis it appear a problem in SelectAsTag:

  nsNavHistory* history = nsNavHistory::GetHistoryService();
  NS_ENSURE_STATE(history);

  // This allows sorting by date fields what is not possible with
  // other history queries.
  mHasDateColumns = PR_TRUE; 

  mQueryString = nsPrintfCString(2048,
    "SELECT null, 'place:folder=' || id || '&queryType=%d&type=%ld', "
      "title, null, null, null, null, null, null, dateAdded, lastModified "
    "FROM   moz_bookmarks "
    "WHERE  parent = %ld",
    nsINavHistoryQueryOptions::QUERY_TYPE_BOOKMARKS,
    nsINavHistoryQueryOptions::RESULTS_AS_TAG_CONTENTS,
    history->GetTagsFolder());

for some reason on PPC the WHERE clause becomes WHERE parent = 0, insted of WHERE parent = 4, so it selects the bookmarks root that has no title, and is containing anything because RESULTS_AS_TAG_CONTENTS can't find tagged items inside.

i don't know if that's a problem with history->GetTagsFolder() or rather with nsPrintfCString. If someone can compile on ppc i would try changing this code and see results. Clearly i can't be sure of this, but the db is correct and RESULTS_AS_TAG_CONTENTS is returning correct items, everything other works fine, so it's the only code i can think of actually.

Comment 20

[Christiaan Hofman]

Seems to confirm a short/long endian problem. It seems GetTagsFolder returns an Int64, while ints on PPC are always Int32. That's always dangerous. I'd say it should help to cast history->GetTagsFolder() to a long? That is what Apple says you should do for %ld (for NSInteger though, but that is a similar beast). 

Comment 21

[Christiaan Hofman]

Another solution might be to use an %lld specifier instead of %ld, because a long long is (almost) always an Int64, while a long may often be an Int32 (and is for sure on a PPC Mac). 

Comment 22

[Marco Bonardo [:mak]]

(In reply to comment #21)
> Another solution might be to use an %lld specifier instead of %ld, because a
> long long is (almost) always an Int64, while a long may often be an Int32 (and
> is for sure on a PPC Mac). 

lol, i was about posting the same solution :) Do we have a PPC tryserver?!

Comment 23

[Marco Bonardo [:mak]]

Attachment: patch

let's see if this is enough

Comment 24

[Marco Bonardo [:mak]]

notice i've checked if we had this problem elsewhere, but other %ld (for example result type, query type) are unsigned short ints so they should not have any problem

Comment 25

[Dietrich Ayala (:dietrich)]

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

r=me, good catch.

Comment 26

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a1.9=beltzner

Comment 27

[Dietrich Ayala (:dietrich)]

Checking in toolkit/components/places/src/nsNavHistory.cpp;
/cvsroot/mozilla/toolkit/components/places/src/nsNavHistory.cpp,v  <--  nsNavHistory.cpp
new revision: 1.293; previous revision: 1.292
done

Comment 28

[Dietrich Ayala (:dietrich)]

once the mac hourly ppc build is ready, can someone with ppc test it?

Comment 29

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041513 Minefield/3.0pre, which is the hourly build. Final verification will occur with tomorrow's build.

Comment 30

[Dietrich Ayala (:dietrich)]

thanks Marcia!

Comment 31

[Christiaan Hofman]

I'm sorry I have to break the party, this problem has not been fixed completely in today's nightly. The mess has gone for the most part. But I still see a weird duplication of the Recent Tags menu. That is, the Recent Tags menu consists of a menu containing one item: a Recent tags menu. Only then do I see the expected (empty) tags menu. That does not seem right to me. So I think one level up there may be a similar problem.

Comment 32

[Christiaan Hofman]

And on a new profile the second Recent Tags is replaced again with (no title). So I get:

Recent Tags > (no title) > (Empty)

Also, the (Empty) item remains, even after I add some tags (and restart). 

Comment 33

[Marcia Knous [:marcia]]

Christiaan: Did you test the hourly build (in the tinderbox directory?). If so, I did not see what you see using a new profile. Tomorrow's build will have the fix dietrich landed.

(In reply to comment #32)
> And on a new profile the second Recent Tags is replaced again with (no title).
> So I get:
> 
> Recent Tags > (no title) > (Empty)
> 
> Also, the (Empty) item remains, even after I add some tags (and restart). 
> 

Comment 34

[Marco Bonardo [:mak]]

for me instead this is fixed, i've tested now with 20080415_1409_firefox-3.0pre.en-US.mac.dmg and it's correctly showing tags (while before the patch i was seeing what Christiaan is reporting in comment #32). will try with a new profile and deleting all tags, but the issue appear fixed for now.

Comment 35

[Christiaan Hofman]

(In reply to comment #33)
> Christiaan: Did you test the hourly build (in the tinderbox directory?). If so,

Guess not, as I don't know what tinderbox is.

> I did not see what you see using a new profile. Tomorrow's build will have the
> fix dietrich landed.

I'm using the latest nightly build, and it does look different from before. 

> 
> (In reply to comment #32)
> > And on a new profile the second Recent Tags is replaced again with (no title).
> > So I get:
> > 
> > Recent Tags > (no title) > (Empty)
> > 
> > Also, the (Empty) item remains, even after I add some tags (and restart). 
> > 
> 

Comment 36

[Marco Bonardo [:mak]]

tested old profile with tags -> correct
tested old profile while removing tags (till no tag) -> correct
tested new profile without tags -> correct
tested new profile while adding tags -> correct

i'm marking again as fixed since we have 2 tested ppc environment working (and they are clearly different since mine is an emulator while marcia's is a real one), please wait for tomorrow nightly build, then feel free to reopen (but i'm optimistic)

Comment 37

[Marcia Knous [:marcia]]

verified fixed using Mozilla/5.0 (Macintosh; U; PPC Mac OS X 10.5; en-US; rv:1.9pre) Gecko/2008041604 Minefield/3.0pre. I tried the combination of steps in Comment 36 to verify.

Comment 38

[Gervase Markham [:gerv]]

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv