Tagging a place: URI can lead to circular menus

VERIFIED FIXED in Firefox 3

Status

()

Firefox
Bookmarks & History
P2
critical
VERIFIED FIXED
11 years ago
9 years ago

People

(Reporter: Christiaan Hofman, Assigned: dietrich)

Tracking

Trunk
Firefox 3
Points:
---
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

11 years ago
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.

Updated

11 years ago
Component: Bookmarks → Places
QA Contact: bookmarks → places
Version: unspecified → Trunk
(Assignee)

Comment 1

11 years ago
Yes, a folder shortcut should not be allowed to be a descendent of the source folder.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: blocking-firefox3?
OS: Mac OS X → All
Priority: -- → P2
Hardware: Macintosh → All
Target Milestone: --- → Firefox 3
Not a common use case, to be sure, but one that needs to be avoided as the consequences are pretty disastrous.
Flags: blocking-firefox3? → blocking-firefox3+
(Reporter)

Comment 3

11 years ago
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!
(Reporter)

Comment 4

10 years ago
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

10 years ago
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. 
(Reporter)

Comment 6

10 years ago
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

10 years ago
(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? 
(Reporter)

Comment 8

10 years ago
> 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). 
(Reporter)

Comment 9

10 years ago
Sorry, I got a bit confused by the different bugs. So please ignore the first part of my comment #8.
(Reporter)

Comment 10

10 years ago
This issue should have definitely made the Known Issues for FF 3.0b5, if not downright be a showstopper.
(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?

(Reporter)

Comment 12

10 years ago
Yes, and it does for anyone using a PPC Mac, see bug 424769.
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.
(Reporter)

Comment 14

10 years ago
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.
(Assignee)

Comment 15

10 years ago
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?
(Assignee)

Comment 16

10 years ago
Created attachment 313420 [details] [diff] [review]
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.
Assignee: nobody → dietrich
Status: NEW → ASSIGNED
Attachment #313420 - Flags: review?(mano)
Comment on attachment 313420 [details] [diff] [review]
fix for tag containers

r=mano
Attachment #313420 - Flags: review?(mano) → review+
(Assignee)

Comment 18

10 years ago
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Summary: Places can lead to circular menus → Tagging a place: URI can lead to circular menus
(Assignee)

Updated

10 years ago
Blocks: 424769
Created attachment 317565 [details]
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

Updated

10 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
ah, dang.  it's recursively added.. That's bug 426870.  sorry.  what did this bug fix then?
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
(Assignee)

Comment 21

10 years ago
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.
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED

Updated

10 years ago
Status: RESOLVED → VERIFIED
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
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.