User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030630 Mozilla Firebird/0.6 Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.5a) Gecko/20030630 Mozilla Firebird/0.6 Firebird allows you to name seperators (e.g., <HR NAME="Open Documentation"> ) You're allowed to use quotation marks in the name, which malforms the markup. Any new Firebird instances will freeze when trying to load the bookmark file. The quotations must be removed with a text editor. Also, use of <HR NAME='"Open Documentation"'> or the inverse does not get around the problem as it should. This problem doesn't exist with directories or bookmarks because the "title" is not an HTML attribute in those cases--it's content. Entities aren't allowed in the NAME or ID attribute in HTML, and aren't processed in the bookmark file despite it being invalid HTML. Striping the quotes will produce an unexepcted/inconsistent behavior for the user. Instead of <HR>, using <legend> and/or <fieldset> might be an alternative. I think the HTML dictionary-list-bookmark-file cludge is wearing just a wee bit thin. It might be about time to either define a solid XML bookmark format, and/or seperate files for bookmarks (the later of which is unrelated to this bug). Reproducible: Always Steps to Reproduce: 1. Open the Firebird bookmark manager. 2. Create a seperator 3. Right-click -> Properties, name it something with quotes like "The War" (with quotes). 4. Close Firebird and relaunch. Firebird should freeze. Strangely, Firebird will freeze after the "Make Firebird your default browser" dialog has been clicked through, not before. (5.) Recovery: Open your bookmarks.html and find the seperator, delete and/or remove extra quotation marks from name. Actual Results: Freeze with near 100% CPU usage. Expected Results: not freeze :P I think it should detect a parse error and attempt to correct the mark-up.

As an aside, bookmarks as XML is already bug 55057.

What does it write out, exactly? <hr name="the "best" browser"> ? That would indeed be malformed and a simple bug. Somebody forgot that you have to escape anything that you write into an HTML file. In content, <>& must be escaped, in attributes, " (and <>& ?) must be escaped. I don't remember the entity for ", &quote;? So, it would have to write out <hr name="the &quote;best&quote; browser"> If the parsers (in Mozilla, Netscape 4, Konqueror and any other importers) are any good, they can deal with that. > Instead of &lt;HR&gt;, using &lt;legend&gt; and/or &lt;fieldset&gt; might > be an No, not backwards-compatible. Reports seems reasonable. I don't use Firebird, so I haven't tested it, but confirming anyways, so that a developer sees it.

Replacing quote marks in bookmarks.html with &quot; does allow Firebird to start up, but then the Bookmark Manager displays the separator name as, eg &quot;separator&quot; instead of "separator"

> but then the Bookmark Manager displays the separator name as e.g. > &quot;separator&quot; Then the Bookmark Manager is broken as well :-) and needs to be fixed.

Apparently, the bookmarks parser is SHOULD convert (some) entities in the bookmarks.html file to their UTF characters - specifically &lt; &gt; &amp; &quot; and &#39; It is shown in BookmarkParser::Unescape(nsString &text). THIS IS A BUG!!!!! http://lxr.mozilla.org/mozilla/source/xpfe/components/bookmarks/src/nsBookmarksService.cpp#926 Fixing Unescape() may provide a pseuod-fix; that is, it will fix the reading error but any chages to bookmarks.html after loading will result in an error (since quotes are still written as " and not &quot;). Does anyone know of the corrosponding Escape() function in the bookmarks.html serializer? Is there a way of verifying these functions are being called and parsing correctly without adding debugging code? Note that escape and unescape functions could be added to the XUL bookmark reader/writer as a quick hack; however, the solution really should be in the cpp code.

This bug is still occurring. Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.7.10) Gecko/20050716 Firefox/1.0.6 - Build ID: 2005071606 The original post has an excellent description of the bug.

Confirmed for Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8) Gecko/20051107 Firefox/1.5

(In reply to comment #7) > Confirmed for > Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8) Gecko/20051107 Firefox/1.5 > Ditto, confirmed for same build as above

sorry for bugspam, long-overdue mass reassign of ancient QA contact bugs, filter on "beltznerLovesGoats" to get rid of this mass change

I just confirmed this in FireFox 1.5.0.6 on mac os x 10.4. I would like to to owner ship and try to resolve this issue.

Reassign per request.

FireFox 2.0b2 fixes the writing aspect so it no longer crashes on restart but it still displays &quot;separator&quot; instead of "separator" So the reading aspect still doesn't work. Do you when want me to work on a patch for the 1.5 branch?

You probably want to focus on FF2.0 and trunk, which may be very different ("Places"). Whether a fix would still be accepted for FF2.0 I don't know.

It looks like 3.0a1 resolves the issue by simply not allowing separators to have names. As the the 2.0 branch is now at rc1 status on the FTP site. I am going to hold off and see if the trunk keeps the present GUI. If the option to rename separators is restored I will look into this further. Leaving open for now as the GUI for the bookmark Manager is still in limbo according to the site. Also I can only readily check this for Mac OS X 10.4 other operating systems may have a different layout.

The current 3.0pre a2 behaves like the 2.0 branch no crash but the name does not display properly in the Bookmark Manager.

I have fixed this in the trunk build and will upload a patch file shortly.

Comment on attachment 261257 [details] [diff] [review] purposed fix for 2.0 branch. Sorry for the bug spam. Having a trouble locating the right person.

Comment on attachment 261257 [details] [diff] [review] purposed fix for 2.0 branch. requesting "r= "

Thanks for the patch, Michael. The code that that patch is changing is the old bookmarks code that's going to be replaced by Places code (mozilla/browser/components/places) on the trunk before the release, so unless you think this should be fixed on the branch I don't think it's really needed at this point.

>>so unless you think this should be fixed on the branch I don't think it's really needed at this point.<< But point is *imho*, that this Bug will also break import of corrupted Bookmark files into the new Places System.

That could be, but the code you're patching isn't used for bookmarks import into Places (it isn't even built when places is enabled).

I would like to have the 2.0 branch patched. I will check the new code. How do you build with places on?

Comment on attachment 261257 [details] [diff] [review] purposed fix for 2.0 branch. this should work for the 2.0 branch obsolete for the trunk

FYI I'm working with the NSPRPUB_PRE_4_2_CLIENT_BRANCH of code it has the new places coed but does not as yet use it in the default compile.

Should the Version and Target Milestone be updated if this bug is solely for the 2.0 branch now?

I am going to focus attention else where this is no longer a crashing bug. The completed fix is attached for anyone that cares to update to 2.0 branch.