237085 - Text disappears with a large string of unbroken characters, e.g., in text input/field

Status: RESOLVED FIXED

(Core Graveyard :: GFX, defect)

Description

[Saurabh Gupta]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.6) Gecko/20040206 Firefox/0.8

When entering (copy-paste) a large set of characters (several pages of a text
document) in the 'Name' field the text disappears after a certain number of
copy-paste operations.

Reproducible: Always
Steps to Reproduce:
1.Create a text document with one page of characters, with or without blanks.
2.Copy this entire page, and start pasting it in the ‘Name Field’ of the  
  Bookmarks->Add Bookmarks Box.
3.Keep pasting the copied characters.
 
Actual Results:  
The 'Name' field accepts almost an infinite amount of characters (several
thousand pages of a text document) and the text disappears after a number of
copy-paste operations. 
Although, even after the text disappears, the field still accepts more characters.
(refer attachment for snapshots)

Expected Results:  
1. If not a feature then the 'Name' field should not accept more than a certain 
   number of characters. (I.E. accepts upto 259 characters as the name of a 
   bookmark).
2. If a feature, then the 'Name' field when accepting large number of characters
   should not depic an unexpected behavior of 'disappearing text'.

(refer attachment for snapshots)

Follow up Tests:
----------------
1. The first follow up test was, comparing this functionality against an 
   Oracle(I.E.). I.E. does not accept more than 259 characters and hence does 
   not depict any 'disappearing text' misbehavior.

2. Secondly, instead of using copy-paste operation, simply entering the 
   characters through the keyboard shows this misbehavior rather inconsistently.
   Sometimes it might accept characters upto a several thousand pages and not 
   depict this misbehavior, and other times it would.

3. To verify if the text was actually present after it disappeared, we 
   highlighted the preceeding (disappeared) text using the Shift + <- keys 
   (refer snapshot). This revealed that the text was indeed present and just not 
   displayed somehow.

4. Replication of the same bug holds for Suse Linux as well.


Importance of the Bug:
----------------------
1. The bug although not serious in nature, might lead to unexpected results, 
   like overflowing the buffer and corrupting data (not verified). Since there 
   is no constraint on the number of characters that can be input, hence this 
   can be an easy exploit.

Comment 1

[Saurabh Gupta]

Attachment: Disappearing Text (highlighted to reveal text)

This snapshot displays the hidden text misbehavior. Some portion of the hidden
text has been highlighted to reveal the bug.

Comment 2

[Saurabh Gupta]

Attachment: Snapshot of 'properties' showing the disappearing text

This snapshot shows that even the properties field shows the disappearing text.
The text has been highlighted to show the blank (disappeared text).

*This is yet another follow up test to confirm the misbehavior.

Comment 3

[Manav Rattan]

(Manav Rattan 03/11/04)

Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.6)
Gecko/20040207 Firefox/0.8

Successfully replicated the bug on Suse Linux (Linux i686)for:
 1. The 'Name' field in Add Bookmarks and also for the
*2. Name field in the Add Bookmarks->New Folder option.

Steps of replication:
---------------------
1. Open the Add Bookmarks->New Folder option and paste a large set of characters 
   as the name of folder. (Use copy-paste operation, by copying and from a text 
   editor and pasting a page at a time into the 'Name' field.

Results: 
--------
Disappearing text mishbehavior is confirmed for both these 'Name' fields for
both the functions (Add Bookmarks and New Folder functions).

Follow up Tests:
----------------
1. Replicated the bug without using the copy-paste operation, rather by entering
characters through the keyboard. In this case there was inconsistency with the
boundary when the text started to disappear. The number of characters after
which the text disappeared varied humongously.


Importance of the bug:
----------------------
1. Since this bug apears on both Windows and Suse Linux, there is a strong
indication of this being a 'platform independent' bug. This could means that the
browser functionality is inherently problematic. 
If considered as an extra feature, still the disappearing text is a strong
indication of some bug.

2. Also as stated in the bug report, there is no constraint on the limit of the
number of characters that can be input to the field (New folder name), hence can
be an exploit for buffer overflow.

Comment 4

[Alex Hirzel]

*** Bug 238399 has been marked as a duplicate of this bug. ***

Comment 5

[Asa Dotzler [:asa]]

This bug appears to impact all single line text areas in Mozilla. The limit is
about 5460 characters for the URL bar and the mail message subject line (I just
typed the number 1 repeatedly until the field went blank and then counted them).
It's 40680 characters for the "keyword" and other single line text fields in
this bug page.

So I guess the bug is slightly different for chrome text fields than it is for
content text fields but it's basically the same bug, I believe.

Comment 6

[Asa Dotzler [:asa]]

oops, I meant 4680 for content, not 40680.

Comment 7

[Asa Dotzler [:asa]]

Further testing reveals that this depends on font size and isn't a hard
character limit but rather a width problem, possibly a rounding or overflow
problem. ->Layout

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I debugged this on GTK2+Xft, and the problem was in platform-specific code in
nsFontMetricsXft::TextDimensionsCallback .  Xft returns text dimensions in an
XGlyphInfo struct, which has |short| members, so we can't have sizes larger than
15 bits.  (We could automatically turn negative sizes into positive ones to
double our capacity, I guess.)

Presumably other platforms have similar problems.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix for nsFontMetricsXft

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 144694 [details] [diff] [review]
fix for nsFontMetricsXft

512 characters is a conservative estimate -- since the width limit is 32767
pixels.  But displays are increasing in resolution, and this probably won't
slow things down much anyway, since hopefully the function call overhead for
entering these functions twice isn't too bad.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

On Windows, this could be fixed partly, in a similar way, in
nsFontMetricsWin{,A}::Resolve{Forwards,Backwards}.  But it's worth noting that
this doesn't fix the DrawString(const char*, ...) case.

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Attachment: fix for nsFontMetricsXft (checked in 2004-04-05 12:18 -0700)

Comment 13

[Asa Dotzler [:asa]]

Additional information: winIE 6 only accepts 2047 characters (I've read that for
IE 4 and 5 it's 2083 characters).  Apache limits URLs to 8190 characters
(anything beyond generates a "Error 414 Request-URI Too Large".)  Many proxy
servers limit the URLs to 255 characters and (I believe) because of that,
RFC2068 suggests: "Servers should be cautious about depending on URI lengths
above 255 bytes, because some older client or proxy implementations may not
properly support these lengths.

Comment 14

[Christopher Blizzard (:blizzard)]

I'm a little concerned that we're going to break some complex layouts where
characters next to each other affect glyph selection.

Comment 15

[Jesse Ruderman]

*** This bug has been marked as a duplicate of 203964 ***

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Sorry, please don't mark bugs that have good technical analysis and patches
duplicate of those that don't.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 203964 has been marked as a duplicate of this bug. ***

Comment 18

[Scott Gifford]

This bug also appears in the location bar with very long URLs; updating summary
to make it more findable.

Bug 35229 is related to this bug; I'm not sure if it's a dup or not, so I'm
marking a dependency.  The first attachment shows a very similar bug that occurs
in the layout of ordinary text, not in a text input of any kind, which is
currently out-of-scope for this bug.

Comment 19

[Brian Ryner (not reading)]

Comment on attachment 144699 [details] [diff] [review]
fix for nsFontMetricsXft (checked in 2004-04-05 12:18 -0700)

blizzard: can you come up with a concrete example?

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 144699 [details] [diff] [review]
fix for nsFontMetricsXft (checked in 2004-04-05 12:18 -0700)

Could I get you to reconsider with the following comment:

	// Don't try to handle more than 512 characters at once, since
	// Xft text measurement can't deal with anything with a width of
	// more than 2^15 (32768) pixels.  This is a hack, and it could
	// break things like combining characters, but that's not nearly
	// as bad as not displaying anything, and it's also very rare to
	// draw strings this long without any breaks.

The key point is that we'll never display anything this wide with wrapped text
-- it's only going to happen with preformatted or single-line text, and even
then it's quite rare.

Comment 21

[Hixie (not reading bugmail)]

It is my understanding that, as mentioned above, this occurs in text fields that
have long text too. For example, any <type input="text"> or the location bar. In
these scenarios, 512 characters are not _that_ rare.

However, combining characters in those scenarios are indeed rare. In any case,
as far as I can tell we don't yet support combining characters anyway, so that
point is moot, no?

Comment 22

[Brian Ryner (not reading)]

Comment on attachment 144699 [details] [diff] [review]
fix for nsFontMetricsXft (checked in 2004-04-05 12:18 -0700)

I think this is the best we can do given the current limits of xft.

Comment 23

[chris hofmann]

Comment on attachment 144699 [details] [diff] [review]
fix for nsFontMetricsXft (checked in 2004-04-05 12:18 -0700)

a=chofmann for 1.7

Comment 24

[Asa Dotzler [:asa]]

Not going to block 1.7 for this problem which we've had forever and aren't
likely to see a comprehensive fix for anytime soon. Adding fixed1.7 to note that
the approved patch (linux fix only) has been landed for 1.7. Note, though, that
this is not fixed for windows or mac and probably won't be any time soon.

Comment 25

[Alex Hosking]

WTF? My Bug came first, This bug is a duplicate of Mine!!

Comment 26

[Asa Dotzler [:asa]]

*** Bug 237207 has been marked as a duplicate of this bug. ***

Comment 27

[Daniel Veditz [:dveditz]]

attachment 154203 [details] from bug 252877 has another testcase for Mozilla 1.7 on windows

Comment 28

[Daniel Veditz [:dveditz]]

*** Bug 252877 has been marked as a duplicate of this bug. ***

Comment 29

[Wayne Woods]

Asa, just checking, is this bug really fixed for 1.7?

The test case that Daniel mentioned: attachment 154203 [details] from bug 252877 still
seems to be broken in the 1.7 branch using 20040716 1.7.2. I'm afraid I don't
have a more recent 1.7 branch build to try it on, but the fixed keyword seems to
have been added in April, anyway.

Comment 30

[Blake Kaplan (:mrbkap) (inactive)]

*** Bug 269680 has been marked as a duplicate of this bug. ***

Comment 31

[Jungshik Shin]

rbs, see comment #11

Comment 32

[Asa Dotzler [:asa]]

If this isn't fixed on the trunk, we'll want to get it for 1.8b2 and Firefox
1.1. This looks like a potential spoofing or phishing vector.

Comment 33

[Asa Dotzler [:asa]]

maybe for 1.8b3. Dveditz, is this something we should be concerned about for
security?

Comment 34

[rbs]

If this is a security issue needing urgent fix, then a stop-gap hack might be to
clamp the length. We already do that to avoid a crash on Win 95/98/ME (bug 255120):

http://lxr.mozilla.org/mozilla/source/gfx/src/windows/nsRenderingContextWin.cpp#1415
1415 // This must be called to clamp string lengths to 8K for Win95/98/ME.
1416 inline void CheckLength(PRUint32 *aLength)
1417 {
1418   if (gIsWIN95 && *aLength > 8192)
1419     *aLength = 8192;
1420 }

Changing |if (gIsWIN95 && *aLength > 8192)| to |if (*aLength > 8192)| will
introduce a 8KB limit -- which is more than IE6 as per comment #13. But the
limit will apply everywhere: pre, content window, view-source, etc. Note however
that limit is on the display appearance, not the DOM.

Comment 35

[Asa Dotzler [:asa]]

->

Comment 36

[Chris]

newest patch does not fix the problem for me.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050614
Firefox/1.0+

Comment 37

[rbs]

What newest patch are you referring to? No new patch has been checked in.

Comment 38

[Majken Connor [:Kensie]]

for what it's worth, I just tested this myself in a few different ways and the
problem for me is DEFINITELY holding down ctrl+v (didn't test with other kb
shortcuts) with a long string in the clip board.

1. I copied a 10000+ character string to clip board
2. Opened bookmark manager and created a new bookmar
3. pasted characters into name field by hitting ctrl+v once
*no problem*
4. started typing random characters (it was reported either her or in similar
bug that this would create the behavior
*no problem*

Start over

1. hold down one key in name field for a LONG time
*no problem*
2. Copy single character paste into name field holding down ctrl+v
*no problem*

Start over

1. copy said 10000+ character string
2. paste into name field using ctrl+v once and letting go
3. repeat 2, letting go each time
*no problem*
4. paste into name field by HOLDING DOWN ctrl+v

***BINGO***

I could ONLY replicate this by holding down ctrl+v while a huge string was in
the clipboard

I sure hope this helps!

Comment 39

[Majken Connor [:Kensie]]

I really should have told you I'm using xp sp2 and dpA2

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b3) Gecko/20050712
Firefox/1.0+

sorry for the spam

Comment 40

[Josh Bressers]

This issue has been assigned the CVE id CAN-2005-2602
http://cve.mitre.org/cgi-bin/cvename.cgi?name=CAN-2005-2602

Comment 41

[Majken Connor [:Kensie]]

*sigh* after reading the link that was posted, I tried this again, it seems I
stopped short of the character limit that breaks this for me, which I don't see
how as it seems today 40k characters are causing the problem, but I made sure to
paste my 10k string at least 5 times before I came to the conclusing that
holding down ctrl-v did it where hitting it and letting go repeatedly did not.  

Today what I am seeing in the address bar is that anything under 39672
characters the characters will show up if I move the mouse over the field or
switch windows to something else and back. 39673 characters will only show for
me if I switch windows and disappears once I move the mouse over the address
bar.  The numbers are the same give or take a few characters in the add bookmark
fields and in the search field in the manage bookmarks window.  It breaks at
31054 characters in the fields on this page.

Sorry for the conflicting reports.

Comment 42

[Phil Ringnalda (:philor)]

*** Bug 308101 has been marked as a duplicate of this bug. ***

Comment 43

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

*** Bug 317746 has been marked as a duplicate of this bug. ***

Comment 44

[Wayne Woods]

In bug 235344 comment 12, someone has noted that this affects people trying to edit their blogs on blogger.com, which could affect a lot of people (as if the spoofing potential wasn't a serious issue all of its own). Requesting a block, if nothing else to get it some attention again in the near future.

Comment 45

[Wayne Woods]

*** Bug 235344 has been marked as a duplicate of this bug. ***

Comment 46

[Uri Bernstein]

*** Bug 328209 has been marked as a duplicate of this bug. ***

Comment 47

[Darin Fisher]

pavlov: Is this something you are working on fixing?  Do you think that we should try to fix this for 1.8.0.3?  Who can help?

Comment 48

[Stuart Parmenter]

er, uh, this wasn't really on my radar at all.  I guess you want this fixed on windows?  I can try to spend some cycles on it if we think it is needed for 1.8.0.3, but I don't know the current windows font code hardly at all.

Comment 49

[Wayne Woods]

It's a problem on OS X as well. So is this something that really needs to be split into separate bugs for different OSes then? And assigned to those who are current with the font code for those?

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Someone please test this on trunk on Windows. If this is fixed now, it was probably fixed by the patch for bug 338251, which can be backported fairly easily to the 1.8 branch. Possibly even the 1.8.0 branch although I'm not sure that's a good idea.

Comment 51

[Wayne Woods]

It's still broken.

As an example, open attachment 143196 [details], which displays a large string of unbroken Xs directly on the web page (not in a text box). If two rows of Xs appear, increase the text size and they'll eventually vanish. See bug 235344 comment 2 for the origin of this attachment.

To demonstrate in a text box, simply copy one row of Xs from the attachment and paste into a text box. Then increase text size and eventually they'll vanish.

There's one related problem that I've never noticed before, so maybe it's new... When viewing the above attachment at a size where you CAN see the Xs, place the cursor at some point in the row and select some text. All the Xs after the selection point vanish, unless the text is very small.

Comment 52

[Wayne Woods]

Attachment: Testcase: two long rows of Xs. Iincrease page text size until they vanish

Adding a testcase taken from attachment 143196 [details] from bug 235344. Silly that this bug had no testcase.

Comment 53

[Darin Fisher]

It looks like bug 338251 has a patch to fix this in a cross-platform manner.  Can we close this bug?

If we take anything for 1.8.1, it'll most likely be the fix in bug 338251.  Removing the blocking 1.8.1 request from this bug.

Comment 54

[Wayne Woods]

(In reply to comment #53)
> It looks like bug 338251 has a patch to fix this in a cross-platform manner. 
> Can we close this bug?

Bug 338251 is a restricted bug, so I can't see the name, or what's going on there (except that it's already marked as Fixed). Is the patch ready to be applied? If not, I think it'd be good to leave this bug open until the fix is in. Otherwise "the public" have no idea that there's an open bug for this issue, and will file more.

Comment 55

[Robert O'Callahan (:roc) (email my personal email if necessary)]

the patch in bug 338251 didn't fix this bug, because that patch has landed and yet this bug is still present on trunk.

Comment 56

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Plan B

Take 2: add support for varying the maximum string length based on the current font's max-advance. This fixes this bug for me; the testcase in this bug shows that Xft chokes when the string length gets bigger than 32767 pixels.

I'm not sure what the heuristics should be on all platforms, but after this patch, we can tune them per-platform and take font metrics into account.

Comment 57

[Darin Fisher]

rev uuids for modified interfaces :)

Comment 58

[Robert O'Callahan (:roc) (email my personal email if necessary)]

stuart pointed out that we can't actually modify these interfaces on branch :-(.

But the most obvious way to do it without modifying interfaces would be to do it all in layout:

  nsCOMPtr<nsIFontMetrics> metrics;
  aContext->GetFontMetrics(*getter_AddRefs(metrics));
  if (!metrics) ... do something ...
  nsCOMPtr<nsIDeviceContext> dc;
  aContext->GetDeviceContext(*getter_AddRefs(dc));
  if (!dc) ... do something ...
  nscoord maxAdvance;
  metrics->GetMaxAdvance(maxAdvance);
  float f = dc->AppUnitsToDevUnits();
  PRInt32 maxLen = (PRInt32)floor(32767.0/(f*maxAdvance));
  maxLen = PR_MAX(1, maxLen);

Which doesn't seem like a good idea to me, these functions are in the critical path.

Comment 59

[Darin Fisher]

Typically, we make the preferred changes on the trunk, and then we introduce funky nsIFoo_MOZILLA_1_8_BRANCH interfaces for the branch.  Ugly and inefficient, but it does the trick, generally.  It makes it clear that the interfaces exist only for this branch.

Comment 60

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I can easily get rid of the nsIRenderingContext change ... just move the GetFontMetrics call up to layout and call GetMaxStringLength from there.

Adding an nsIFontMetrics_MOZILLA_1_8_BRANCH to all the nsIFontMetrics implementations and QIing to it will be a real pain but I guess that's what we have to do.

Comment 61

[Robert O'Callahan (:roc) (email my personal email if necessary)]

The other option, which would have no performance penalty, would be to move all this code down into gfx. E.g. nsRenderingContextImpl's implementation of the text functions would be taken from nsLayoutUtils, and GetMaxStringLength would be a protected virtual method declared in nsRenderingContextImpl, and we would call ...Internal versions of the platform-specific text methods. Maybe I'll try that.

Comment 62

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: plan C

Okay, this version has no interface changes. As I suggested in comment #61, we move the safety logic to nsRenderingContextImpl, which overrides the usual nsIRenderingContext methods. Platforms which want to take advantage of that logic rename their internal string methods to *Internal, and implement GetMaxStringLength by delegating to their internal textmetrics class.

Comment 63

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: plan C v2

oops, I busted Thebes/cairo by deleting two lines I shouldn't have.

Comment 64

[rbs]

Comment on attachment 225842 [details] [diff] [review]
plan C v2

sr=rbs

+PRInt32 nsFontMetricsGTK::GetMaxStringLength(nscoord &aAveCharWidth)
+{
+  return mMaxStringLength;
+}

Typo. (Unintended argument from the copy-paste.)

================
 NS_IMETHODIMP 
-nsThebesRenderingContext::GetBoundingMetrics(const char*        aString,
-                                             PRUint32           aLength,
-                                             nsBoundingMetrics& aBoundingMetrics)
+nsThebesRenderingContext::GetBoundingMetricsInternal(const char*        aString,
+                                                     PRUint32           aLength,
+                                                     nsBoundingMetrics& aBoundingMetrics)
 {
   return NS_OK;
 }
 
 NS_IMETHODIMP
-nsThebesRenderingContext::GetBoundingMetrics(const PRUnichar*   aString,
-                                             PRUint32           aLength,
-                                             nsBoundingMetrics& aBoundingMetrics,
-                                             PRInt32*           aFontID)
+nsThebesRenderingContext::GetBoundingMetricsInternal(const PRUnichar*   aString,
+                                                     PRUint32           aLength,
+                                                     nsBoundingMetrics& aBoundingMetrics,
+                                                     PRInt32*           aFontID)
 {
     return NS_OK;
 }
 #endif // MOZ_MATHML

While you are in that neck of the world, these should better be:
 |return NS_ERROR_NOT_IMPLEMENTED|

Comment 65

[Robert O'Callahan (:roc) (email my personal email if necessary)]

checked in on trunk.

Comment 66

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I had to fix Mac bustage ... the 7-arg GetTextDimensionsInternal variants are not present there.

so far btek Tp looks good.

Comment 67

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 225842 [details] [diff] [review]
plan C v2

I think we should take this on branch once we've verified that it fixes some bugs. The patch looks big but
-- the layout changes are just backing out some trunk changes that were never on branch
-- most of the rest are cookie-cutter changes across a bunch of platforms
-- no interface changes
-- the guts of the patch are the safe string wrapper functions, which aren't much changed from what's been on trunk for a couple of weeks

Comment 68

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 225842 [details] [diff] [review]
plan C v2

a=dbaron on behalf of drivers for 1.8.1.  Please check in to MOZILLA_1_8_BRANCH and add fixed1.8.1 when checked in.

(And don't back out anything else by accident while you're there. :-)

Comment 69

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: branch patch

This is the patch for branch. It includes the GetRightToLeftText changes from bug 338251 (minus the interface change), because this depends on those.

Comment 70

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Fixed on 1.8.1 branch

Comment 71

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: updated branch patch

branch patch with fix for bug 342922 included

Comment 72

[Phil Ringnalda (:philor)]

*** Bug 259145 has been marked as a duplicate of this bug. ***

Comment 73

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we probably need this fixed on the 1.8.0.x branch, although I'd still like someone to tell me which bugs this fixes on various platforms.

Comment 74

[Wayne Woods]

Note that the effect I mentioned in the last paragraph of comment 51 still exists.  Namely:
- open attachment 143196 [details]
- highlight some text (just one character is enough)
- increase font size, and eventually all text after the highlight will vanish

The same is true if you go to the URL mentioned in attachment 154203 [details], and highlight a character or more in the location bar.

So it's not entirely fixed. Should I open a new bug for this?

Comment 75

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, please file a new bug for that.

Comment 76

[Wayne Woods]

(In reply to comment #75)
> Yes, please file a new bug for that.

Done. Bug 346234.

Comment 77

[Wayne Woods]

By the way, were the patches supposed to fix the Mac version too? I just realized that neither the trunk nor the 1.8 branch are fixed on Mac as of 20060727 (I'd previously only tested on Windows). And I'm talking about the original bug here, not the new one I just opened (which was discovered on Windows).

Comment 78

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes they were supposed to fix Mac. What exactly is the problem on Mac? At what width do the characters disappear? And do they just disappear, or do they crash, or what?

Comment 79

[Wayne Woods]

(In reply to comment #78)
> Yes they were supposed to fix Mac. What exactly is the problem on Mac? At what
> width do the characters disappear? And do they just disappear, or do they
> crash, or what?

The problem is that on the Mac this bug is behaving exactly as it was before. For example, If you view attachment 225522 [details] at 12 pt Times, no Xs appear at all. If you view at 11 pt, one row appears. At 10 pt, both rows appear. The URL I entered in the box above also fails to appear until the default text size is set to 10 pt or below.

Oddly, at exactly 11 pt, when one row of Xs appears in the attachment, if I highlight an X, then all Xs after it vanish, as per bug 346234. If I drop to 10 pt, this doesn't happen. The URL in the box above exhibits the same symptoms at 10 pt. Cairo isn't switched on for Mac yet, is it?

Comment 80

[Wayne Woods]

Forgot to mention that I checked trunk builds for a few days after this landed on the trunk (20060626) and none appear to be fixed, so this isn't a more recent regression.

Comment 81

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess they weren't really supposed to fix Mac since I didn't have a string length limit defined on Mac.

Try applying these patches to nsFontMetricsMac.cpp/.h:
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsFontMetricsWin.cpp&branch=&root=/cvsroot&subdir=mozilla/gfx/src/windows&command=DIFF_FRAMESET&rev1=3.246&rev2=3.247
http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=nsFontMetricsWin.h&branch=&root=/cvsroot&subdir=mozilla/gfx/src/windows&command=DIFF_FRAMESET&rev1=3.59&rev2=3.60

If that fixes it, we can use that.

Comment 82

[Wayne Woods]

Attachment: Partial Mac fix, but not there yet

I tried this patch on Mac. I had to guess at how to get a max char width, as there's no metrics object in the Mac code. It stops the long text from vanishing, so that much works. However, as you increase font size, the characters begin to overlap until they eventually form a solid bar. So I guess I haven't got something right yet.

Comment 83

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #82)
> Created an attachment (id=231200) [edit]
> Partial Mac fix, but not there yet
> 
> I tried this patch on Mac. I had to guess at how to get a max char width, as
> there's no metrics object in the Mac code. It stops the long text from
> vanishing, so that much works. However, as you increase font size, the
> characters begin to overlap until they eventually form a solid bar. So I guess
> I haven't got something right yet.

That only happens for the long text, right?

If this is a clear improvement, we should probably take it.

Comment 84

[Wayne Woods]

(In reply to comment #83)
> That only happens for the long text, right?

Yes, only for the long text. The rest of the text scales accordingly without overlap.

Is there an easy way to figure out what the correct string length limit is for Mac? Should I just keep increasing the constant until the bug reappears?

Comment 85

[Wayne Woods]

Well, I determined a string length at which I thought this bug no longer occurred on Mac, which is just a little higher than the 32767 for Windows. But then I discovered that the bug still occurs for a small % of fonts (most of which seem to be monotype, for some reason). So I'm going to have to run the numbers again till I can be sure that I have a figure that works for those fonts too. Will lodge a patch asap.

It looks like the "big character overlap" problem will occur regardless of the max string length I set, so I'm going to have to just spin off a separate bug for that issue... maybe one of the Mac text gurus will know what's going on.

Comment 86

[Wayne Woods]

Attachment: Fix for Mac

Looks like I should have stuck with the original max... it's close enough to the limit. This patch fixes the problem with all fonts I tested. Not sure who else should be on the review of this one, so could you please add them, roc? Thanks :)

Comment 87

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 231754 [details] [diff] [review]
Fix for Mac

I feel qualified to give this an r+ since I did the other platforms.

+  float MaxCharWidth = float(::CharWidth('M'));

Make this maxCharWidth.

Comment 88

[Wayne Woods]

Attachment: Fix for Mac, adjusted for review comments

Thanks Robert. Updated patch attached.

Comment 89

[rbs]

Nit: you might want to remove the folloing comment at check-in, as it is now irrelevant.
   // No known string length limits on Mac

Comment 90

[Wayne Woods]

Attachment: Fix for Mac, adjusted for review comments

(In reply to comment #89)
> Nit: you might want to remove the folloing comment at check-in, as it is now
> irrelevant.
>    // No known string length limits on Mac

Thanks! I should have noticed that. Since roc hasn't looked at the new patch yet, I'll just update it to include that.

Comment 91

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 231894 [details] [diff] [review]
Fix for Mac, adjusted for review comments

a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH and add the keyword fixed1.8.1 once you have done so.

Comment 92

[Wayne Woods]

(In reply to comment #91)
> (From update of attachment 231894 [details] [diff] [review] [edit])
> a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH and add
> the keyword fixed1.8.1 once you have done so.

Thanks :) One question though... normally, once a patch is approved, do I then have to go and find someone to check it in? If so, I'm not sure who. I wasn't sure if I or my reviewers take care of that. Cheers.



Comment 93

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Find someone on IRC to do it for you

Comment 94

[:Gavin Sharp [email: [e-mail]]]

I can check it in later today.

Comment 95

[:Gavin Sharp [email: [e-mail]]]

Just to confirm: attachment 231894 [details] [diff] [review] needs to be landed on both the trunk and 1.8 branch?

Comment 96

[Wayne Woods]

(In reply to comment #95)
> Just to confirm: attachment 231894 [details] [diff] [review] [edit] needs to be landed on both the trunk and 1.8
> branch?

Yes please! Thanks, Gavin :)

Comment 97

[:Gavin Sharp [email: [e-mail]]]

Mac patch checked in to the trunk.
mozilla/gfx/src/mac/nsFontMetricsMac.cpp 	1.77
mozilla/gfx/src/mac/nsFontMetricsMac.h 	1.35

Comment 98

[Daniel Veditz [:dveditz]]

This is requesting blocking1.8.0.7, but which patches need approval? Were the two marked approval1.8.1 both needed, and still appropriate for the 1.8.0 branch?

An explicit approval request would help.

Comment 99

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I believe attachment 225842 [details] [diff] [review] and attachment 231894 [details] [diff] [review] both need 1.8.0.x approval.

Also, I'm removing fixed1.8.1 until attachment 231894 [details] [diff] [review] lands on the 1.8 branch.

Comment 100

[Robert O'Callahan (:roc) (email my personal email if necessary)]

When these land, the fixes in bug 342922 and 345588 should land with them.

Comment 101

[:Gavin Sharp [email: [e-mail]]]

Checked in the mac patch on the 1.8 branch.
mozilla/gfx/src/mac/nsFontMetricsMac.cpp 	1.73.12.3
mozilla/gfx/src/mac/nsFontMetricsMac.h 	1.33.12.2

Comment 102

[Wayne Woods]

Thanks Gavin!

I just spun off bug 348202 for the Mac colliding-characters problem.

Comment 103

[Wayne Woods]

*** Bug 331086 has been marked as a duplicate of this bug. ***

Comment 104

[Wayne Woods]

*** Bug 264405 has been marked as a duplicate of this bug. ***

Comment 105

[Wayne Woods]

*** Bug 35229 has been marked as a duplicate of this bug. ***

Comment 106

[Denis Kasak]

*** Bug 305264 has been marked as a duplicate of this bug. ***

Comment 107

[Daniel Veditz [:dveditz]]

Comment on attachment 231894 [details] [diff] [review]
Fix for Mac, adjusted for review comments

approved for 1.8.0 branch, a=dveditz for drivers

Comment 108

[Daniel Veditz [:dveditz]]

Comment on attachment 231894 [details] [diff] [review]
Fix for Mac, adjusted for review comments

Take that back, this bug has a large patch and we'd like to see how it goes on 1.8.1 for a bit longer.

Comment 109

[Wayne Woods]

Out of interest, was this fixed for Linux? I had a feeling it was, but please see bug 322920 comment 17. Can anyone using Linux still reproduce this bug when using a monospace font?

Comment 110

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It was intended to be fixed on Linux. Please file a new bug with testcase if there are any remaining issues.

Comment 111

[Wayne Woods]

I don't use Linux, so I was hoping someone who does could independently confirm the claim on that other bug.

Comment 112

[Daniel Veditz [:dveditz]]

Restoring lost blocking flag

Comment 113

[Daniel Veditz [:dveditz]]

I doubt this spoofing bug will ever meet the criteria of the 1.8.0 branch given the many regressions vs. the low impact of the bug itself.

Comment 114

[Vitaliy Filippov]

I'm experiencing this bug on 10.0.2 on Debian Wheezy/Sid. Text disappears in long textareas.
Should I fill this bug again?

Comment 115

[Majken Connor [:Kensie]]

viatliy,

yes, but make sure you're experiencing it on a download from Mozilla as well. If not file it with Debian, or file it in both places and link to each other so the right people can ask the right questions.