424884 - Tagging a place: URI can lead to circular menus

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

Adding certain place: bookmarks can lead to bookmark menus which are circular, i.e. they can contain themselves as a submenu. 

Reproducible: Always

Steps to Reproduce:
1. Add a new bookmark with location place:folder=2 to the bookmarks
2.
3.
Actual Results:  
The new bookmark is added as a folder, containing the full bookmarks menu, including the item itself

Expected Results:  
Menus should never contain themselves, not even after a few steps. Circular /infinite menus are dangerous. 

In Mac OS X 10.5, circular/infinite menus lead to a serious hang, see bug 400291.

The places syntax is seriously broken.

Comment 1

[Dietrich Ayala (:dietrich)]

Yes, a folder shortcut should not be allowed to be a descendent of the source folder.

Comment 2

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

Not a common use case, to be sure, but one that needs to be avoided as the consequences are pretty disastrous.

Comment 3

[Christiaan Hofman]

I'm not so sure it's not a common use case. Perhaps not the specific example, but it shows it is very easy to create circular folders. If it happens with such a trivial example, it can also easily happen with complex queries. I am sure there are many useful (and common) cases having this problem. In fact, Minefield creates one for me automatically!

Comment 4

[Christiaan Hofman]

Also adding excludeQueries=1 does not change anything, that option does not seem to have any effect. Of course even if this would work, this problem still should not occur in any situation.

Comment 5

[Bo Yang]

I have some more tests on this bug. 
If I delete the new created folder in the Bookmarks Organizer, it does not disappear at all. Only when I restart the Bookmarks Organizer, it disappear. But if I create the folder in the bookmarks menu, it does not dsiappear always. I think this means that Places rebuild process has broken to some degree by this folder, too. 

Also, if I create a bookmark with Location place:folder=20 (Note that, there is no 20 folder at all), so I point the folder to a non-existing folder location. And then the bookmark menu can not show at all. 

I think the "place:" location should be used internally only by the Firefox or extensions. And the users should has no way to set such a location in the bookmarks system. And in this way, I will try to validate the Location input text and submit a patch.

Or, if we want the user use the "place:" kind of location, I think figure out a new patch will be more difficult here. Because we must make sure any location did not point to a non-existing folder or its ancestors. 

Comment 6

[Christiaan Hofman]

Bo, these are all *different* bugs with places. I think it's better to not pollute this one more with those. That's also why I opened several other bug reports related to these very issues.

Comment 7

[Bo Yang]

(In reply to comment #6)
> Bo, these are all *different* bugs with places. I think it's better to not
> pollute this one more with those. That's also why I opened several other bug
> reports related to these very issues.
> 

I think, at least the "can't delete" bug is the same bug of this one. It is another behavior of this bug. Just because of the circular folder reference, the created folder can't be deleted. 
And add a non-existing location should be another bug, maybe! 

And, whether the "place:" kind of location can be used by users? 

Comment 8

[Christiaan Hofman]

> I think, at least the "can't delete" bug is the same bug of this one. It is
> another behavior of this bug. Just because of the circular folder reference,
> the created folder can't be deleted. 

The "can't delete" bug is definitely not the same bug (and that one is not caused by circular references). It makes this one worse, but is certainly not the cause. 

Essentially, bug 424769 is the cause of the hang in the Help menu. But that bug is more specific than this one. Moreover, it does not explain the original crasher, which should still be present AFAIK, it's just hidden by the hang. 

> And add a non-existing location should be another bug, maybe! 

Perhaps, but I'm not sure I agree. I'd rather say it should be handled more gracefully. Validation to this detail would only lead to a lot of pain.

> And, whether the "place:" kind of location can be used by users? 

I did not file a bug report on that, you can do that. I'm also not sure it's a bug. Note that places is still in development, and as long as this is the case I think it's good that users can test it out and find bugs (or work around them, as I now do for this one). 

Comment 9

[Christiaan Hofman]

Sorry, I got a bit confused by the different bugs. So please ignore the first part of my comment #8.

Comment 10

[Christiaan Hofman]

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

Comment 11

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

(In reply to comment #10)
> This issue should have definitely made the Known Issues for FF 3.0b5, if not
> downright be a showstopper.

Is there a way for this to happen other than manually editing places queries?

Comment 12

[Christiaan Hofman]

Yes, and it does for anyone using a PPC Mac, see bug 424769.

Comment 13

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

Yes, and that bug is a blocker because the "no title" entry there is being misgenerated.

Let me be more clear: through normal UI actions such as dragging, dropping and creating saved searches, is there any way to create circular menus other than manually editing the queries?

I'm not disagreeing that this is a significant problem, I'm trying to get an understanding of how the problem can and will arise.

Comment 14

[Christiaan Hofman]

That I don't know. I guess you can tag the Recent Tags item, and it would show up in the Recent Tags. But I cannot test this, as the Recent Tags item doesn't work for me.

Comment 15

[Dietrich Ayala (:dietrich)]

Beltzner: Yes, you can do this by tagging the Recent Tags item, as Christian suggested. I haven't yet found other ways to create circular menus without manually manipulating place: URIs.

The fix for this specific example is to set expandQueries = false for tag containers.

Maybe we should also disallow tagging of place: URIs. Is there a valid use-case for doing this?

Comment 16

[Dietrich Ayala (:dietrich)]

Attachment: fix for tag containers

This excludes queries from being displayed in tag containers. Using expandQueries=0 allows the query to show up as an empty folder in the tag container, which is useless and will just confuse users who happen to tag a place URI somehow.

Comment 17

[Asaf Romano (gone)]

Comment on attachment 313420 [details] [diff] [review]
fix for tag containers

r=mano

Comment 18

[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.284; previous revision: 1.283
done

In the spirit of one-checkin-per-bug, I'm morphing this bug to cover the tag container case. I've filed bug 426870 for fixing the general place: URI case.

Christiaan: apologies for missing the second "a" in your name in comment #15.

Comment 19

[Tracy Walker [:tracy]]

Attachment: screenshot of fooo created by location: places:folder=2

This bug appears to still be present.  Using Library > Organize > New Bookmark to create a bookmark Name: fooo and Location: places:folder=2  I get the double nested Bookmarks Menu inside the newly created fooo folder

Comment 20

[Tracy Walker [:tracy]]

ah, dang.  it's recursively added.. That's bug 426870.  sorry.  what did this bug fix then?

Comment 21

[Dietrich Ayala (:dietrich)]

see comments #15 and #16

STR:

1. bookmark a place: URI like you did in comment #19
2. add a tag to that bookmark

Expected:

The bookmark does not show up in the tag container in the Library for that tag.

Comment 22

[Tracy Walker [:tracy]]

in that case, this is not fixed. I changed an existing untagged bookmark's location to places:folder=2, then tagged it.  The bookmark in question appears in the tag container with location places:folder=2

reopening

Comment 23

[Tracy Walker [:tracy]]

from irc: :
tracy: the bookmark showing is fine
[12:59p] dietrich:
as long as it's not a container
[12:59p] dietrich:
it's exactly what you created: a bookmark w/ a place: URI

Comment 24

[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