Closed
Bug 407821
Opened 17 years ago
Closed 10 years ago
Truncate tag to a maximum length (pick a length that gives us memory savings, somewhere over 100 chars seems fine)
Categories
(Firefox :: Bookmarks & History, defect, P3)
Firefox
Bookmarks & History
Tracking
()
RESOLVED
FIXED
Firefox 35
People
(Reporter: masa141421356, Assigned: yarik.sheptykin, Mentored)
References
Details
(4 keywords, Whiteboard: [good first bug])
Attachments
(5 files, 2 obsolete files)
39.03 KB,
image/png
|
Details | |
63.74 KB,
image/jpeg
|
Details | |
670 bytes,
patch
|
Details | Diff | Splinter Review | |
1.86 KB,
patch
|
Paolo
:
feedback+
|
Details | Diff | Splinter Review |
4.89 KB,
patch
|
Paolo
:
review+
|
Details | Diff | Splinter Review |
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007121009 Firefox/3.0b2pre Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9b2pre) Gecko/2007121009 Firefox/3.0b2pre If text in "Tags." of Places Organizer is too long, Places Organizer works wrong. 1.Too slow. 2.Text is not displayed correctly in list view and textbox. I think its length should be limited Reproducible: Always Steps to Reproduce: 1.Edit bookmark 2.Start editing "tags." 3.Select huge text in other application like notepad and copy to clipboard. 4.Paste it to "tags." about 100times. Actual Results: Length of "Tags." seems to be not limited. Expected Results: Length of "Tags." shouled be limited.
Reporter | ||
Comment 1•17 years ago
|
||
Text in tags is huge text (but it contains only one word).
Reporter | ||
Comment 2•17 years ago
|
||
I think it is not needed to support too long word in Tags.
Summary: Length of "Tags." in Places organizer should be limited. → Huge text in Tags breaks view of Places organizer
Comment 3•17 years ago
|
||
On OS X I get ellipses at the end of the column if the available width isn't enought to show all tags. You don't get such one? Could it be a encoding issue?
Reporter | ||
Comment 4•17 years ago
|
||
(In reply to comment #3) > On OS X I get ellipses at the end of the column if the available width isn't > enought to show all tags. You don't get such one? Could it be a encoding issue? > When length of tags is not huge ("Supercalifragilisticexpialidocious" etc.), ellipsis is displayed. This bug occurs only when text is very very long. I can't count length of tag in screenshot be cause textbox of tags is empty. But according to binary data of places.sqllite, it is about 6MB.
Reporter | ||
Comment 5•17 years ago
|
||
This bug contains two different issue. 1. list view issue (Ellipsis is not displayed) 2. empty textbox issue But list view issue not seems to be issue of Library. I change target of this bug to "empty textbox" issue.
Summary: Huge text in Tags breaks view of Places organizer → Huge text in Tags in Library causes empty textbox.
Comment 6•17 years ago
|
||
What does it make you think that it's not an issue within the library? Which textboxes are empty? Please be clear in your statements and don't transform a bug into handling a totally different issue.
Reporter | ||
Comment 7•17 years ago
|
||
I think , list view issue is "Core" issue. I already filed it as another bug. But it may be security issue. (Now , It is security-sensitive bug).
Comment 8•17 years ago
|
||
Anyway so please be clear about the issue you mentioned in comment 5. Which textboxes in which area of the Library do you mean?
Reporter | ||
Comment 9•17 years ago
|
||
Text of tags is not displayed at input file of tags.
Attachment #292871 -
Attachment is obsolete: true
Reporter | ||
Comment 10•17 years ago
|
||
(In reply to comment #8) > Anyway so please be clear about the issue you mentioned in comment 5. Which > textboxes in which area of the Library do you mean? > I mean textbox rounded red line in attachment 293489 [details].
Comment 11•17 years ago
|
||
Which folder inside the Library is it?
Reporter | ||
Comment 12•17 years ago
|
||
(In reply to comment #11) > Which folder inside the Library is it? > "All Bookmarks" -> "Bookmarks Menu" -> subfolder1 -> subfoldsr2 -> bookmarked RSS feed (live bookmark)
Comment 13•17 years ago
|
||
I believe that the text will be displayed but it will take a really long time to display. I can confirm this. As longer the tag content is, Firefox has a massive slowdown. Shall we limit the length of the Tag content to a specific count of characters? I don't think that users need more than somewhat 1024 characters.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: perf
Summary: Huge text in Tags in Library causes empty textbox. → Huge text in Tags slows down Firefox drastically
Version: unspecified → Trunk
Comment 14•17 years ago
|
||
The rendering issue is probably a dupe of bug 92193.
Comment 16•17 years ago
|
||
Even with the patch on bug 393870 there is no change in handling very long tag names. Even editing the tag name makes Firefox consuming nearly 100% cpu load. So it's not only based on querying or showing the tag. Is it a general problem in handling very long strings?
Summary: Huge text in Tags slows down Firefox drastically → Very long tag name slows down Firefox drastically
Comment 17•17 years ago
|
||
There is no change at all. With the latest build Firefox totally hangs. I'm not able to work with the Library. Even the auto complete for the location bar is drastically slow. Further the display of very long tags goes crazy. Here is a screen shot.
Updated•17 years ago
|
OS: Windows XP → All
Hardware: PC → All
Updated•17 years ago
|
Flags: blocking-firefox3?
Comment 18•17 years ago
|
||
What's the point of having a tag that's thousands of characters long? The point of a tag is to succinctly label item(s) so that later that tag can be quickly and easily entered into search to find those tagged item(s). There isn't anything about a 1000+ character tag that is quick, easy or succinct. I'd call this WONTFIX.
Comment 19•17 years ago
|
||
Maybe we should auto-truncate at a certain length when storing the tags? This would keep people from being prompted but also save us headaches.
Comment 20•17 years ago
|
||
(In reply to comment #19) > Maybe we should auto-truncate at a certain length when storing the tags? This > would keep people from being prompted but also save us headaches. Morphing.
Flags: wanted-firefox3+
Flags: blocking-firefox3?
Flags: blocking-firefox3-
Summary: Very long tag name slows down Firefox drastically → Truncate tag to a maximum length (pick a length that gives us memory savings, somewhere over 100 chars seems fine)
Updated•15 years ago
|
Priority: -- → P3
Whiteboard: [good first bug]
Comment 21•15 years ago
|
||
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
Comment 22•12 years ago
|
||
I reproduced the bug by following the steps in the description. The results are exactly those already citated: - the length of tags seems unlimited - and the browser slows down when you copy a lot of long text over and over Do you have any idea where I can look into the code to solve this problem? (I've never looked into Firefox source code so I have no idea where to search for it)
Comment 23•12 years ago
|
||
I think I understood where the problem lies. Exploring the source code for the Places Organizer I found the function that takes the string you give to the Tags Field and splits it into actual tags. The function is called "EIO__getTagsArrayFromTagField()" and it's defined in "/browser/base/content/places/editBookmarkOverlay.js". To fix the problem I think it will suffice to add an invocation of "substring(0, 100)" on the string representing the tags after the invocation of "trim()". I'll try to test if this works and make a patch as soon as I manage to compile the nightly build :-)
Comment 24•12 years ago
|
||
I'm sorry, the correct file in which the function is defined is "/browser/components/places/content/editBookmarkOverlay.js".
Comment 25•12 years ago
|
||
I reproduced the bug by adding a very long text to the Tags field. I also get this javascript timeout.
Comment 26•12 years ago
|
||
The solution I wrote doesn't work because it still doesn't prevent the user from inserting a very long string which will slow down the browser when processed. I managed to fix the problem by modifying the xul interface "editBookmarkOverlay.xul". I simply added the property maxlength="100" to the textbox for the tags field.
Comment 27•12 years ago
|
||
(In reply to Cristian Bellomo from comment #22) > I reproduced the bug by following the steps in the description. > The results are exactly those already citated: > - the length of tags seems unlimited > - and the browser slows down when you copy a lot of long text over and over This is a very good analysis. The second issue is related to the first, but can be considered a separate bug, but I agree that solving the first problem will actually prevent the second from happening in this case.
Comment 28•12 years ago
|
||
You may also look at the nsITaggingService interface implementation in order to validate the input at the back-end level. This would benefit other applications that use the tagging service as well: https://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsTaggingService.js#132
Comment 29•12 years ago
|
||
Comment 30•12 years ago
|
||
Assigning the bug to Cristian who provided a first patch to the issue.
Assignee: nobody → cristian.deepeye
Status: NEW → ASSIGNED
Whiteboard: [good first bug] → [good first bug][mentor=paolo]
Comment 31•12 years ago
|
||
Hi Cristian! Sorry for this week of silence, now I can help you with the procedures for solving the bug, as we discussed last week. I remember that with you and Marco, we've identified that we need the following interventions: 1. Prevent any user of the nsITaggingService API from adding tags that are longer than allowed, and raise an exception if this happens. That is implemented in nsTaggingService.js, and needs an xpcshell test. We should make the length a constant in nsITaggingService.idl. For example: https://mxr.mozilla.org/mozilla-central/source/xpcom/io/nsIFile.idl#36 2. Change the front-end code to filter out individual tags longer than the allowed length (determined using the constant). 3. Set a maxlength on the tags textbox, to avoid slowing down the browser. This can actually be longer than the maximum length of one tag. I'm not sure if we should use the constant (multiplied for instance by 10) or just hardcode the value in the XUL file. Marco, does all of the above sound right? Cristian, when you have the modifications for issue 1, you can just create a patch using these commands for proper formatting: https://developer.mozilla.org/en-US/docs/Creating_a_patch Then, please set the "feedback" flag to "?" and indicate my account (:paolo), similar to what is explained here for the review flag: https://developer.mozilla.org/en-US/docs/Developer_Guide/How_to_Submit_a_Patch
Comment 32•12 years ago
|
||
I have modified nsITaggingService as you said Paolo, but it fires up another problem: it recognizes correctly when you try to insert a too long tag and it will prevent you from doing it but it will also not insert all the other tags you write with it. We could solve this by modifying also the front-end so that it doesn't pass to the back-end a value that would throw an exception. However I wonder if this would be alright for other applications which use the tagging service, maybe they would suffer of similar problems in the front-end too. If this is alright I will write the test too (which I'm not sure exactly where an how I should implement it yet).
Attachment #676752 -
Flags: review?(paolo.mozmail)
Comment 33•12 years ago
|
||
Comment on attachment 676752 [details] [diff] [review] Intervention on nsTaggingService.js (In reply to Cristian Bellomo from comment #32) > However I wonder if this would be alright for other applications which use > the tagging service, maybe they would suffer of similar problems in the > front-end too. This is true, and it's fine. It's the design decision we made for the API. > If this is alright I will write the test too (which I'm not sure exactly > where an how I should implement it yet). You can add a function in test_tagging.js, and run the test using: ./mach xpcshell-test toolkit/components/places/tests/unit/test_tagging.js
Attachment #676752 -
Flags: review?(paolo.mozmail) → feedback+
Comment 34•12 years ago
|
||
I've added the addon-compat and dev-doc-needed keywords, whose purpose is to indicate that we're updating the API behavior, so API consumers can take action if needed.
Keywords: addon-compat,
dev-doc-needed
Comment 35•11 years ago
|
||
Can you confirm that you're still working on this bug?
Flags: needinfo?(cristian.deepeye)
Comment 36•11 years ago
|
||
(In reply to Mike Hoye [:mhoye] from comment #35) > Can you confirm that you're still working on this bug? Sorry, I wasn't able to work on this bug anymore. Please reassign it.
Flags: needinfo?(cristian.deepeye)
Comment 37•10 years ago
|
||
OK, throwing it back.
Assignee: cristian.deepeye → nobody
Status: ASSIGNED → NEW
Updated•10 years ago
|
Mentor: paolo.mozmail
Whiteboard: [good first bug][mentor=paolo] → [good first bug]
Comment 38•10 years ago
|
||
Hi, I would like to work on this bug.
Comment 39•10 years ago
|
||
(In reply to Ashutosh Dhundhara [:ashutoshd] from comment #38) > I would like to work on this bug. Welcome, thank you for your interest! I see you have started working on bug 1009799 already, so you probably already have a Firefox build, and can import patches and run tests on them. Have you already tried manually reproducing the issue from the description in comment 0? If so, have you tried applying the patches already present in this bug and see if they solve the problem? If I remember correctly, the remaining work is about writing the tests for the patch, but since some time has passed, it should also be verified whether the existing patches are still valid or need updating.
Assignee | ||
Comment 40•10 years ago
|
||
Hi, everyone! Here comes my first attempt to submit a patch to mozilla. First, I could reproduce the situation described in comment #0 with the trunk build. Firefox slows down and increasingly consumes more CPU and RAM when some long text is entered into the tags field in bookmarks. I applied the patches from Christian Bellomo to limit the length of a single tag. I also limited the max-length of the imput field to 100 characters as suggested in comment 29. I added a test against long tags to test_tagging.js. I could not completely address the point 3 in comment 31 because I don"t know how to place expressions into max-length of the XUL. I believe an expression similar to "10 * Ci.nsITaggingService.MAX_TAG_LENGTH" is needed. I can try to get it working if required but first would like to know if this the right way to go.
Attachment #8489874 -
Flags: review?(paolo.mozmail)
Comment 41•10 years ago
|
||
Comment on attachment 8489874 [details] [diff] [review] Limits the length of the bookmarks tag to 100 characters. bug407821.diff Review of attachment 8489874 [details] [diff] [review]: ----------------------------------------------------------------- This is a very good first patch, thank you! ::: browser/components/places/content/editBookmarkOverlay.xul @@ +149,5 @@ > tabscrolling="true" > showcommentcolumn="true" > observes="paneElementsBroadcaster" > + placeholder="&editBookmarkOverlay.tagsEmptyDesc.label;" > + maxlength="100" (In reply to Iaroslav Sheptykin from comment #40) > I could not completely address the point 3 in comment 31 because I don"t > know how to place expressions into max-length of the XUL. I believe an > expression similar to "10 * Ci.nsITaggingService.MAX_TAG_LENGTH" is needed. > I can try to get it working if required but first would like to know if this > the right way to go. To generate this value from code you'd need a setAttribute call when the dialog is created, but I think we can use the simple approach here and just hardcode the value (1000 instead of 100) in the maxlength attribute above. I made this change and started a tryserver build: https://tbpl.mozilla.org/?tree=Try&rev=5a077be98d51 If that succeeds, I can take care of the checkin.
Attachment #8489874 -
Flags: review?(paolo.mozmail) → review+
Assignee | ||
Comment 42•10 years ago
|
||
(In reply to :Paolo Amadini from comment #41) > If that succeeds, I can take care of the checkin. Great! I will be happy to work on any further issues if tests fail.
Comment 43•10 years ago
|
||
Tests look good, pushed the change to the fx-team integration branch: https://hg.mozilla.org/integration/fx-team/rev/f1d1f89e5bd5
Assignee | ||
Comment 44•10 years ago
|
||
(In reply to :Paolo Amadini from comment #43) > Tests look good, pushed the change to the fx-team integration branch: > > https://hg.mozilla.org/integration/fx-team/rev/f1d1f89e5bd5 Awesome! Thanks, Paolo!
Comment 45•10 years ago
|
||
Unfortunately I've had to reverted this for test failures: https://tbpl.mozilla.org/php/getParsedLog.php?id=48202636&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=48202042&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=48203036&tree=Fx-Team remote: https://hg.mozilla.org/integration/fx-team/rev/07d5640694f2
Comment 46•10 years ago
|
||
Also failures in other suites: https://tbpl.mozilla.org/php/getParsedLog.php?id=48202895&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=48203462&tree=Fx-Team https://tbpl.mozilla.org/php/getParsedLog.php?id=48203408&tree=Fx-Team It seems that nsITaggingService.idl was missing the UUID bump (try runs are clobbers). Paolo when reviewing please remember to check for UUID changes needed :-)
Comment 47•10 years ago
|
||
it doesn't need a UUID bump. if we have an hook that hook is wrong.
Comment 48•10 years ago
|
||
or something changed recently. We never needed UUID bumps for constants.
Comment 49•10 years ago
|
||
The build system now decides whether to rebuild .idl files based on the UUID. Just always bump the UUID, it's safer that way and can't harm anything except in beta.
Assignee | ||
Comment 50•10 years ago
|
||
I have no idea what you are talking about so I assume I cannot help here. If I can be of any help regarding the patch, please let me know.
Comment 51•10 years ago
|
||
(In reply to Iaroslav Sheptykin from comment #50) > I have no idea what you are talking about so I assume I cannot help here. If > I can be of any help regarding the patch, please let me know. please change the UUID in the nsITaggingService.idl file: [scriptable, uuid(f816b4df-f733-4dbd-964d-8bfc92a475b2)] <=== interface nsITaggingService : nsISupports { you can generate a new uuid using: ./mach uuid then attach an updated patch and needinfo me or paolo to push it to the Try server.
Updated•10 years ago
|
Assignee: nobody → yarik.sheptykin
Status: NEW → ASSIGNED
Assignee | ||
Comment 52•10 years ago
|
||
Alright, guys, I have updated the patch according to the comment 41 (regarding the length of the input field) and comment 51. The patch includes all previous interventions squashed together. It makes my other patch obsolete. The tests pass on my current setup.
Attachment #8489874 -
Attachment is obsolete: true
Attachment #8490732 -
Flags: review?(mak77)
Comment 53•10 years ago
|
||
Comment on attachment 8490732 [details] [diff] [review] Full patch for bug 407821 with updated UUID Review of attachment 8490732 [details] [diff] [review]: ----------------------------------------------------------------- Iaroslav, thanks for helping us with the paperwork! Firefox is a complex code base and sometimes things are non-obvious even for experienced developers, so I appreciate you taking the time to update the patch. I've changed the commit message to be more summary-like and include the reviewer's nickname, per our convention: "Discard bookmark tags exceeding maximum length of 100 characters. r=paolo" Here is a new tryserver build to verify everything is still in order, before the final landing: https://tbpl.mozilla.org/?tree=Try&rev=154216ec15e3
Attachment #8490732 -
Flags: review?(mak77) → review+
Comment 55•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/232b2eadb49a
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Assignee | ||
Comment 56•10 years ago
|
||
Awesome.
You need to log in
before you can comment on or make changes to this bug.
Description
•