401434 - Global History ::RowMatches creates UTF16 strings with the wrong length

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[jag (Peter Annema)]

if (property_column == kToken_NameColumn) {
        AppendUTF16toUTF8(Substring((const PRUnichar*)yarn.mYarn_Buf,
                                    (const PRUnichar*)yarn.mYarn_Buf + yarnLength),
                          titleStr);

That should use yarnLength / sizeof(PRUnichar).

Comment 1

[jag (Peter Annema)]

Attachment: Fix bug, fix string usage

Comment 2

[jag (Peter Annema)]

Attachment: Minimal fix

Comment 3

[jag (Peter Annema)]

Oh, I found this because in the console I'd see things like:

WARNING: got a low Surrogate but no high surrogate: file ../../../dist/include/string/nsUTF8Utils.h, line 772
WARNING: got a low Surrogate but no high surrogate: file ../../../dist/include/string/nsUTF8Utils.h, line 693
WARNING: got a high Surrogate but no low surrogate: file ../../../dist/include/string/nsUTF8Utils.h, line 763
WARNING: got a High Surrogate but no low surrogate: file ../../../dist/include/string/nsUTF8Utils.h, line 681

Comment 4

[[e-mail]]

Comment on attachment 286463 [details] [diff] [review]
Fix bug, fix string usage

Nice cleanup, but this still doesn't quite work on history files created by Mac PPC but used on Mactel, because the byte-order is wrong, isn't it? Maybe you should use GetRowValue (for titles) and GetRowURL (for everything else)? (Rename GetRowURL if necessary; I only needed it to autocomplete URLs.)

Comment 5

[jag (Peter Annema)]

I thought about using GetRowValue() but I couldn't see where we set mYarn_Form when we write out the title. I'll do some more investigation.

Comment 6

[jag (Peter Annema)]

Ah, I guess we just always use the UTF-16 form. Kinda annoying that they gave it value 0 instead of leaving that for Latin1, use 1 for UTF-8, and 2 for UTF-16; then we could actually have looked at the mYarn_Form and done something clever without needing to know which column we're looking at.

So, option 1: add a GetRowValueAsUTF16() to avoid an extra string copy (and make GetRowURL() use it too); option 2: teach GetRowValue(..., nsAString&) about kToken_NameColumn so it can do AppendUTF8toUTF16 for everything else. Of course this means if we want to add another UTF-16 column at any point we'll have to make sure that that list gets updated.

Comment 7

[[e-mail]]

How about if we rename Set/GetRowValue(aRow, aCol, nsAString& aResult) to Set/GetTitleValue(aRow, aResult) (because it only works on titles), and rename GetRowURL(aRow, aResult) to GetRowValue(aRow, aCol, aResult)?

Comment 8

[[e-mail]]

(In reply to comment #6)
>if we want to add another UTF-16 column at any point we'll
>have to make sure that that list gets updated.
We're more likely to move to Places, at least for history.

Comment 9

[jag (Peter Annema)]

I have a patch I mostly like that I'll attach when I get home.

Comment 10

[jag (Peter Annema)]

Attachment: Teach GetRowValue() about the name column, have GetRowURL() call GetRowValue()

Comment 11

[[e-mail]]

Comment on attachment 288305 [details] [diff] [review]
Teach GetRowValue() about the name column, have GetRowURL() call GetRowValue()

I still think GetRowURL could be inlined.

Comment 12

[jag (Peter Annema)]

Attachment: As previous, but with GetRowURL() inlined.

Comment 13

[[e-mail]]

Comment on attachment 288502 [details] [diff] [review]
As previous, but with GetRowURL() inlined.

>+  nsGlobalHistory* history = closure->history;
I can see the point of this...

> 
>   // get visit counts - we're ignoring all errors from GetRowValue(), 
>   // and relying on default values
>+  const mdb_column& kToken_VisitCountColumn = history->kToken_VisitCountColumn;
... but I'm not sure about this, because although we know that history->kToken_Foo won't change, the compiler won't.

Comment 14

[jag (Peter Annema)]

Attachment: And inline history->kToken_FooColumn

Carrying forward sr=Neil.

Comment 15

[jag (Peter Annema)]

Checking in nsGlobalHistory.cpp;
/cvsroot/mozilla/xpfe/components/history/src/nsGlobalHistory.cpp,v  <--  nsGlobalHistory.cpp
new revision: 1.230; previous revision: 1.229
done
Checking in nsGlobalHistory.h;
/cvsroot/mozilla/xpfe/components/history/src/nsGlobalHistory.h,v  <--  nsGlobalHistory.h
new revision: 1.61; previous revision: 1.60
done

Comment 16

[Reed Loden [:reed]]

Does suiterunner use the xpfe or the toolkit version of nsGlobalHistory? Is there anybody using toolkit/components/history/src/nsGlobalHistory.cpp currently?

Comment 17

[[e-mail]]

To use toolkit/components/history you need to be a XUL app, but not SeaMonkey or Thunderbird, and not using Places.

Comment 18

[Robert Kaiser]

toolkit/components/history is dead, not used by anyone and scheduled to be removed (bug 383833).

SeaMonkey will continue to use xpfe history until it switches to places history (which I'd hope someone will work on soon), so I think this is fixed for all cases that concern us, right? Clearing blocking request because of that.