1056235 - _getInitialSelection in findbar.xml does not handle surrogate pairs correctly

Status: RESOLVED FIXED

(Toolkit :: Find Toolbar, defect, P1)

Description

[kanata]

Attachment: test.html

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:34.0) Gecko/20100101 Firefox/34.0
Build ID: 20140820030202

See bug991427

Steps to reproduce:
1. Open attached test.html,
2. Select all, and
3. Do Ctrl+F.

Actual results:
Findbar shows one "a" and 74 fires and one "D83D in a box" character.

Expected results:
Selection text is truncated by a correct boundary.

Comment 1

[Mike de Boer [:mikedeboer]]

To reproduce you now have to do 'CMD+e' to copy the selection to the global find clipboard and that will be prefilled in the findbar textbox.

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: deal with surrogate pairs properly

Masayuki, are you comfortable with reviewing this patch?

Comment 3

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8759648 [details] [diff] [review]
Patch 1: deal with surrogate pairs properly

Well, I'm not a good reviewer for the findbar in these days, but perhaps, I can review this.

>-    return selText.trim().replace(/\s+/g, " ").substr(0, kSelectionMaxLen);
>+    selText = selText.trim().replace(/\s+/g, " ");
>+    let truncLength = kSelectionMaxLen;
>+    if (selText.length > truncLength) {
>+      let truncChar = selText.charAt(truncLength).charCodeAt(0);
>+      if (truncChar >= 0xDC00 && truncChar <= 0xDFFF)

I think that you should put {} even if there is only one line in the block but I'm not sure the our coding rules of JS.

>+        truncLength++;

In this case, isn't the value is already its max value? (|let truncLength = kSelectionMaxLen;| is above.)

So, isn't truncLength--; is expected by the others?


If these points make sense, please fix them (or one of them) before landing.

Comment 4

[Mike de Boer [:mikedeboer]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) from comment #3)
> >+        truncLength++;
> 
> In this case, isn't the value is already its max value? (|let truncLength =
> kSelectionMaxLen;| is above.)

It's at its max, but it will never exceed the length of selText. When we hit a surrogate pair as the last character, allowing the substr() to include one more char will result in the complete glyph, but when you'd count the amount of characters in the find field, you'll find no more than kSelectionMaxLen.

> So, isn't truncLength--; is expected by the others?

Doing truncLength-- would also fix the issue, but result in one less character in the find field. Not a big difference, but it does mean that truncLength++ is more correct.

Comment 5

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/8ab79f629a310879863ef10e4df38f4944899b04
Bug 1056235 - deal with surrogate pairs properly when retrieving the currently selected text for prefill in the findbar. r=masayuki

Comment 6

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

(In reply to Mike de Boer [:mikedeboer] from comment #4)
> (In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due
> to injured) from comment #3)
> > >+        truncLength++;
> > 
> > In this case, isn't the value is already its max value? (|let truncLength =
> > kSelectionMaxLen;| is above.)
> 
> It's at its max, but it will never exceed the length of selText. When we hit
> a surrogate pair as the last character, allowing the substr() to include one
> more char will result in the complete glyph, but when you'd count the amount
> of characters in the find field, you'll find no more than kSelectionMaxLen.

Why? String.length counts a surrogate pair as 1? And I worry about XPCOM. If the string goes to C++ code via XPCOM, I guessed that it whose length is longer than kSelectionMaxLen causes buffer overflow. Is that really safe?

> > So, isn't truncLength--; is expected by the others?
> 
> Doing truncLength-- would also fix the issue, but result in one less
> character in the find field. Not a big difference, but it does mean that
> truncLength++ is more correct.

I don't think that it's "more" correct because it's really grater than "maximum length".

Comment 7

[Mike de Boer [:mikedeboer]]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) from comment #6)
> Why? String.length counts a surrogate pair as 1?

Unfortunately I don't know the full story behind this. I just know that this is what I _see_.

> And I worry about XPCOM. If
> the string goes to C++ code via XPCOM, I guessed that it whose length is
> longer than kSelectionMaxLen causes buffer overflow. Is that really safe?

Sure. The maximum length is not there to provide a sane maximum size of a string insert into the textbox, not so much due to worries about overloading the XPCOM interface.

> > Doing truncLength-- would also fix the issue, but result in one less
> > character in the find field. Not a big difference, but it does mean that
> > truncLength++ is more correct.
> 
> I don't think that it's "more" correct because it's really grater than
> "maximum length".

I understand what you mean and strictly by character count it's indeed not 'more' correct. It's an error in both directions, but within an acceptable margin I think...

Comment 8

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/8ab79f629a31