Closed Bug 407821 Opened 12 years ago Closed 5 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)

defect

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)

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.
Attached image Screenshot (obsolete) —
Text in tags is huge text (but it contains only one word).
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
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?
(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.
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.
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.
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).
Anyway so please be clear about the issue you mentioned in comment 5. Which textboxes in which area of the Library do you mean? 
Attached image screenshot
Text of tags is not displayed at input file of tags.
Attachment #292871 - Attachment is obsolete: true
(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].
Which folder inside the Library is it?
(In reply to comment #11)
> Which folder inside the Library is it?
> 

"All Bookmarks" -> "Bookmarks Menu" 
-> subfolder1 -> subfoldsr2 -> bookmarked RSS feed (live bookmark)

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
The rendering issue is probably a dupe of bug 92193.
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
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.
OS: Windows XP → All
Hardware: PC → All
Flags: blocking-firefox3?
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.
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.
(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)
Priority: -- → P3
Whiteboard: [good first bug]
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
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)
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 :-)
I'm sorry, the correct file in which the function is defined is "/browser/components/places/content/editBookmarkOverlay.js".
I reproduced the bug by adding a very long text to the Tags field.

I also get this javascript timeout.
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.
(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.
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
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]
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
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 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+
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.
Can you confirm that you're still working on this bug?
Flags: needinfo?(cristian.deepeye)
(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)
OK, throwing it back.
Assignee: cristian.deepeye → nobody
Status: ASSIGNED → NEW
Mentor: paolo.mozmail
Whiteboard: [good first bug][mentor=paolo] → [good first bug]
Hi,
I would like to work on this bug.
(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.
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 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+
(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.
Tests look good, pushed the change to the fx-team integration branch:

https://hg.mozilla.org/integration/fx-team/rev/f1d1f89e5bd5
(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!
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 :-)
it doesn't need a UUID bump. if we have an hook that hook is wrong.
or something changed recently. We never needed UUID bumps for constants.
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.
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.
(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.
Assignee: nobody → yarik.sheptykin
Status: NEW → ASSIGNED
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 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+
https://hg.mozilla.org/mozilla-central/rev/232b2eadb49a
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 35
Awesome.
Depends on: 1122941
You need to log in before you can comment on or make changes to this bug.