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

RESOLVED FIXED

Status

defect
--
major
RESOLVED FIXED
15 years ago
7 years ago

People

(Reporter: sgupta, Assigned: roc)

Tracking

({fixed1.7, fixed1.8.1})

Dependency tree / graph
Bug Flags:
blocking1.8b3 -
blocking1.8b5 -
blocking1.9a1 +
blocking1.8.1 +
blocking1.8.0.4 -
blocking1.8.0.7 -
blocking1.8.0.8 -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [sg:spoof], URL)

Attachments

(8 attachments, 6 obsolete attachments)

80.42 KB, image/jpeg
Details
71.19 KB, image/jpeg
Details
1.42 KB, patch
bryner
: review+
blizzard
: superreview+
chofmann
: approval1.7+
Details | Diff | Splinter Review
16.37 KB, text/html
Details
53.81 KB, patch
smontagu
: review+
Details | Diff | Splinter Review
219.14 KB, patch
smontagu
: review+
Details | Diff | Splinter Review
125.02 KB, patch
Details | Diff | Splinter Review
3.32 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

15 years ago
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.
(Reporter)

Comment 1

15 years ago
This snapshot displays the hidden text misbehavior. Some portion of the hidden
text has been highlighted to reveal the bug.
(Reporter)

Comment 2

15 years ago
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

15 years ago
(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.
*** Bug 238399 has been marked as a duplicate of this bug. ***
Assignee: p_ch → mozeditor
Severity: minor → major
Status: UNCONFIRMED → NEW
Component: Bookmarks → Editor: Core
Ever confirmed: true
Flags: blocking1.7?
OS: Windows XP → All
QA Contact: seamonkey.bookmarks → bugzilla
Summary: Text disappears in ‘Name’ field on entering a large set of characters → Text disappears in single-line field on entering a large set of characters

Comment 5

15 years ago
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.
Whiteboard: 11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111 111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111…

Comment 6

15 years ago
oops, I meant 4680 for content, not 40680.

Updated

15 years ago
Whiteboard: 11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111 111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111…

Comment 7

15 years ago
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
Assignee: mozeditor → nobody
Component: Editor: Core → Layout
QA Contact: bugzilla → core.layout
Whiteboard: 1111111111111111111111111111111111111111111111111111111111111111 1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111…
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.
Assignee: nobody → general
Component: Layout → GFX
QA Contact: core.layout → ian
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.
Attachment #144694 - Flags: superreview?(blizzard)
Attachment #144694 - Flags: review?(bryner)
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.
Attachment #144694 - Attachment is obsolete: true
Attachment #144694 - Flags: superreview?(blizzard)
Attachment #144694 - Flags: review?(bryner)
Attachment #144699 - Flags: superreview?(blizzard)
Attachment #144699 - Flags: review?(bryner)
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.
I'm a little concerned that we're going to break some complex layouts where
characters next to each other affect glyph selection.
Attachment #144699 - Flags: superreview?(blizzard) → superreview-

Comment 15

15 years ago

*** This bug has been marked as a duplicate of 203964 ***
Status: NEW → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → DUPLICATE
Status: RESOLVED → VERIFIED
Sorry, please don't mark bugs that have good technical analysis and patches
duplicate of those that don't.
Status: VERIFIED → REOPENED
Resolution: DUPLICATE → ---
*** Bug 203964 has been marked as a duplicate of this bug. ***

Comment 18

15 years ago
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.
Blocks: 35229
Summary: Text disappears in single-line field on entering a large set of characters → Text disappears in single-line input field or location bar on entering a large set of characters
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 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.
Attachment #144699 - Flags: superreview- → superreview?(blizzard)
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?

Updated

15 years ago
Hardware: PC → All
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.
Attachment #144699 - Flags: review?(bryner) → review+
Attachment #144699 - Flags: superreview?(blizzard) → superreview+

Comment 23

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

a=chofmann for 1.7
Attachment #144699 - Flags: approval1.7? → approval1.7+
Attachment #144699 - Attachment description: fix for nsFontMetricsXft → fix for nsFontMetricsXft (checked in 2004-04-05 12:18 -0700)
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.
Flags: blocking1.7? → blocking1.7-
Keywords: fixed1.7

Comment 25

15 years ago
WTF? My Bug came first, This bug is a duplicate of Mine!!
Whiteboard: 1111111111111111111111111111111111111111111111111111111111111111 1111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111… → 1111111111111111111111111111111111111111111111111111111111111111l 111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111…
Whiteboard: 11111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111111 … → <lot's o' ones>
Whiteboard: <lot's o' ones>
*** Bug 237207 has been marked as a duplicate of this bug. ***
attachment 154203 [details] from bug 252877 has another testcase for Mozilla 1.7 on windows
*** Bug 252877 has been marked as a duplicate of this bug. ***

Updated

15 years ago
Blocks: 92193

Comment 29

15 years ago
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.
*** Bug 269680 has been marked as a duplicate of this bug. ***
Flags: blocking1.8b?
Flags: blocking-aviary1.1?
Whiteboard: [sg:fix]

Updated

15 years ago
Flags: blocking1.8b?
Flags: blocking1.8b+
Flags: blocking-aviary1.1?
Flags: blocking-aviary1.1+

Comment 31

15 years ago
rbs, see comment #11
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.
Flags: blocking1.8b2+
Flags: blocking1.8b+
Flags: blocking1.7-
maybe for 1.8b3. Dveditz, is this something we should be concerned about for
security?
Flags: blocking1.8b2+ → blocking1.8b3+

Comment 34

14 years ago
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.
->
Assignee: general → pavlov
Status: REOPENED → NEW

Comment 36

14 years ago
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

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

Updated

14 years ago
Flags: blocking1.8b4+
Flags: blocking1.8b3-
Flags: blocking1.8b3+

Updated

14 years ago
Blocks: longlines
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!
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

Updated

14 years ago
Flags: blocking1.8b4-
Flags: blocking1.8b4+
Flags: blocking-aviary1.5+
*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.
*** Bug 308101 has been marked as a duplicate of this bug. ***
*** Bug 317746 has been marked as a duplicate of this bug. ***
Whiteboard: [sg:fix] → [sg:spoof]

Comment 44

14 years ago
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.
Flags: blocking1.8.0.3?
Summary: Text disappears in single-line input field or location bar on entering a large set of characters → Text disappears with a large set of characters. Number depends on text size

Comment 45

14 years ago
*** Bug 235344 has been marked as a duplicate of this bug. ***
Summary: Text disappears with a large set of characters. Number depends on text size → Text disappears with a large string of unbroken characters, e.g., in text input/field
*** Bug 328209 has been marked as a duplicate of this bug. ***

Comment 47

13 years ago
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

13 years ago
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

13 years ago
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?
Flags: blocking1.9a1+
Flags: blocking1.8.1?
Flags: blocking1.8.0.3?
Flags: blocking1.8.0.3-
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

13 years ago
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

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

Comment 53

13 years ago
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.
Flags: blocking1.8.1?

Comment 54

13 years ago
(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.
the patch in bug 338251 didn't fix this bug, because that patch has landed and yet this bug is still present on trunk.
Posted patch Plan BSplinter Review
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.
Assignee: pavlov → roc
Status: NEW → ASSIGNED
Attachment #225672 - Flags: superreview?(rbs)
Attachment #225672 - Flags: review?(smontagu)

Comment 57

13 years ago
rev uuids for modified interfaces :)
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

13 years ago
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.
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.
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.
Attachment #225672 - Flags: review?(smontagu) → review+
Posted patch plan C (obsolete) — Splinter Review
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.
Attachment #225837 - Flags: superreview?(rbs)
Attachment #225837 - Flags: review?(smontagu)
Posted patch plan C v2Splinter Review
oops, I busted Thebes/cairo by deleting two lines I shouldn't have.
Attachment #225837 - Attachment is obsolete: true
Attachment #225842 - Flags: superreview?(rbs)
Attachment #225842 - Flags: review?(smontagu)
Attachment #225837 - Flags: superreview?(rbs)
Attachment #225837 - Flags: review?(smontagu)
Attachment #225842 - Flags: review?(smontagu) → review+

Comment 64

13 years ago
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|
Attachment #225842 - Flags: superreview?(rbs) → superreview+
checked in on trunk.
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago13 years ago
Resolution: --- → FIXED
I had to fix Mac bustage ... the 7-arg GetTextDimensionsInternal variants are not present there.

so far btek Tp looks good.
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
Attachment #225842 - Flags: approval1.8.1?

Updated

13 years ago
Blocks: 342922

Updated

13 years ago
No longer blocks: 342922
Depends on: 342922
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. :-)
Attachment #225842 - Flags: approval1.8.1? → approval1.8.1+
Posted patch branch patch (obsolete) — Splinter Review
This is the patch for branch. It includes the GetRightToLeftText changes from bug 338251 (minus the interface change), because this depends on those.
Fixed on 1.8.1 branch
Keywords: fixed1.8.1
branch patch with fix for bug 342922 included
Attachment #227625 - Attachment is obsolete: true
*** Bug 259145 has been marked as a duplicate of this bug. ***
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.
Flags: blocking1.8.0.6?

Updated

13 years ago
Depends on: 345588

Comment 74

13 years ago
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?
Yes, please file a new bug for that.

Comment 76

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

Done. Bug 346234.

Comment 77

13 years ago
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).
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

13 years ago
(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

13 years ago
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 82

13 years ago
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.
(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

13 years ago
(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

13 years ago
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

13 years ago
Posted patch Fix for Mac (obsolete) — Splinter Review
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 :)
Attachment #231200 - Attachment is obsolete: true
Attachment #231754 - Flags: review?(roc)
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.
Attachment #231754 - Flags: superreview+
Attachment #231754 - Flags: review?(roc)
Attachment #231754 - Flags: review+

Comment 88

13 years ago
Thanks Robert. Updated patch attached.
Attachment #231754 - Attachment is obsolete: true
Attachment #231893 - Flags: review?(roc)
Attachment #231893 - Flags: approval1.8.1?

Comment 89

13 years ago
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

13 years ago
(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.
Attachment #231893 - Attachment is obsolete: true
Attachment #231894 - Flags: review?(roc)
Attachment #231894 - Flags: approval1.8.1?
Attachment #231893 - Flags: review?(roc)
Attachment #231893 - Flags: approval1.8.1?
Attachment #231894 - Flags: superreview+
Attachment #231894 - Flags: review?(roc)
Attachment #231894 - Flags: review+
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.
Attachment #231894 - Flags: approval1.8.1? → approval1.8.1+

Comment 92

13 years ago
(In reply to comment #91)&#13;&#10;> (From update of attachment 231894 [details] [diff] [review] [edit])&#13;&#10;> a=dbaron on behalf of drivers.  Please check in to MOZILLA_1_8_BRANCH and add&#13;&#10;> the keyword fixed1.8.1 once you have done so.&#13;&#10;&#13;&#10;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.&#13;&#10;&#13;&#10;
Find someone on IRC to do it for you
I can check it in later today.
Whiteboard: [sg:spoof] → [checkin needed (1.8 branch)][sg:spoof]
Just to confirm: attachment 231894 [details] [diff] [review] needs to be landed on both the trunk and 1.8 branch?

Comment 96

13 years ago
(In reply to comment #95)&#13;&#10;> Just to confirm: attachment 231894 [details] [diff] [review] [edit] needs to be landed on both the trunk and 1.8&#13;&#10;> branch?&#13;&#10;&#13;&#10;Yes please! Thanks, Gavin :)
Mac patch checked in to the trunk.
mozilla/gfx/src/mac/nsFontMetricsMac.cpp 	1.77
mozilla/gfx/src/mac/nsFontMetricsMac.h 	1.35
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.
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.
Keywords: fixed1.8.1
When these land, the fixes in bug 342922 and 345588 should land with them.
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
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)][sg:spoof] → [sg:spoof]

Comment 102

13 years ago
Thanks Gavin!

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

Comment 103

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

Comment 104

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

Comment 105

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

Comment 106

13 years ago
*** Bug 305264 has been marked as a duplicate of this bug. ***
Comment on attachment 231894 [details] [diff] [review]
Fix for Mac, adjusted for review comments

approved for 1.8.0 branch, a=dveditz for drivers
Attachment #231894 - Flags: approval1.8.0.7? → approval1.8.0.7+
Flags: blocking1.8.0.7? → blocking1.8.0.7+
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.
Attachment #231894 - Flags: approval1.8.0.7+ → approval1.8.0.8?
Attachment #227898 - Flags: approval1.8.0.8?
Attachment #227898 - Flags: approval1.8.0.7?
Attachment #227898 - Flags: approval1.8.0.7-
Flags: blocking1.8.0.8+
Flags: blocking1.8.0.7-
Flags: blocking1.8.0.7+

Comment 109

13 years ago
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?
It was intended to be fixed on Linux. Please file a new bug with testcase if there are any remaining issues.

Comment 111

13 years ago
I don't use Linux, so I was hoping someone who does could independently confirm the claim on that other bug.
Depends on: 349229
Flags: blocking1.8.0.8+ → blocking1.8.0.8?
Restoring lost blocking flag
Flags: blocking1.8.0.9?
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.
Flags: blocking1.8.0.9? → blocking1.8.0.8-
Attachment #227898 - Flags: approval1.8.0.9?
Attachment #231894 - Flags: approval1.8.0.9?
Product: Core → Core Graveyard
I'm experiencing this bug on 10.0.2 on Debian Wheezy/Sid. Text disappears in long textareas.
Should I fill this bug again?
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.
You need to log in before you can comment on or make changes to this bug.