407821 - Truncate tag to a maximum length (pick a length that gives us memory savings, somewhere over 100 chars seems fine)

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Masahiro YAMADA]

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.

Comment 1

[Masahiro YAMADA]

Attachment: Screenshot

Text in tags is huge text (but it contains only one word).

Comment 2

[Masahiro YAMADA]

I think it is not needed to support too long word in Tags.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 4

[Masahiro YAMADA]

(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.

Comment 5

[Masahiro YAMADA]

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.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 7

[Masahiro YAMADA]

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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Anyway so please be clear about the issue you mentioned in comment 5. Which textboxes in which area of the Library do you mean? 

Comment 9

[Masahiro YAMADA]

Attachment: screenshot

Text of tags is not displayed at input file of tags.

Comment 10

[Masahiro YAMADA]

(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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Which folder inside the Library is it?

Comment 12

[Masahiro YAMADA]

(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

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 14

[Mats Palmgren (inactive)]

The rendering issue is probably a dupe of bug 92193.

Comment 15

[Asaf Romano (gone)]

The patch on bug 393870 may help here.

Comment 16

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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?

Comment 17

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Errorneous display of very long tag

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.

Comment 18

[Tracy Walker [:tracy]]

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

[Al Billings [:abillings - ex-MoCo]]

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

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

(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.

Comment 21

[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

Comment 22

[Cristian Bellomo]

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

[Cristian Bellomo]

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

[Cristian Bellomo]

I'm sorry, the correct file in which the function is defined is "/browser/components/places/content/editBookmarkOverlay.js".

Comment 25

[Baldo Alessandro]

I reproduced the bug by adding a very long text to the Tags field.

I also get this javascript timeout.

Comment 26

[Cristian Bellomo]

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

[:Paolo Amadini]

(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

[:Paolo Amadini]

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

[Cristian Bellomo]

Attachment: Work in progress

Comment 30

[Marco Bonardo [:mak]]

Assigning the bug to Cristian who provided a first patch to the issue.

Comment 31

[:Paolo Amadini]

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

[Cristian Bellomo]

Attachment: Intervention on nsTaggingService.js

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).

Comment 33

[:Paolo Amadini]

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

Comment 34

[:Paolo Amadini]

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.

Comment 35

[Mike Hoye [:mhoye]]

Can you confirm that you're still working on this bug?

Comment 36

[Cristian Bellomo]

(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.

Comment 37

[Mike Hoye [:mhoye]]

OK, throwing it back.

Comment 38

[Ashutosh Dhundhara [:ashutoshd]]

Hi,
I would like to work on this bug.

Comment 39

[:Paolo Amadini]

(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.

Comment 40

[Iaroslav Sheptykin]

Attachment: Limits the length of the bookmarks tag to 100 characters. bug407821.diff

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.

Comment 41

[:Paolo Amadini]

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.

Comment 42

[Iaroslav Sheptykin]

(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

[:Paolo Amadini]

Tests look good, pushed the change to the fx-team integration branch:

https://hg.mozilla.org/integration/fx-team/rev/f1d1f89e5bd5

Comment 44

[Iaroslav Sheptykin]

(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

[Ed Morley [:emorley]]

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

[Ed Morley [:emorley]]

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

[Marco Bonardo [:mak]]

it doesn't need a UUID bump. if we have an hook that hook is wrong.

Comment 48

[Marco Bonardo [:mak]]

or something changed recently. We never needed UUID bumps for constants.

Comment 49

[Benjamin Smedberg]

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.

Comment 50

[Iaroslav Sheptykin]

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

[Marco Bonardo [:mak]]

(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.

Comment 52

[Iaroslav Sheptykin]

Attachment: Full patch for bug 407821 with updated UUID

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.

Comment 53

[:Paolo Amadini]

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

Comment 54

[:Paolo Amadini]

https://hg.mozilla.org/integration/fx-team/rev/232b2eadb49a

Comment 55

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/232b2eadb49a

Comment 56

[Iaroslav Sheptykin]

Awesome.