328982 - formhistory.dat is endian

Status: RESOLVED FIXED

(Toolkit :: Form Manager, defect)

Description

[Mark Mentovai]

formhistory.dat stores UCS2 strings in native endianness.  This causes problems when bringing a profile between architectures.  The reason that's a problem is contained in bug 328981.

Consider doing what was done in bug 108134: store native endianness in the file and swap appropriately.

Comment 1

[Mark Mentovai]

Nominating because of the effect this has on the x86 transition.

Comment 2

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

isn't this a duplicate of bug 256885?

Comment 3

[Mark Mentovai]

No, but the remaining issue there is the same as bug 328981.

Comment 4

[Mark Mentovai]

To clarify: this bug is about formhistory.dat, not history.dat.  formhistory.dat does not currently carry any endianness data.  history.dat does - that's what bug 256885 was.  Bug 328981 and the later comments in bug 265885 are about byte-swapping when reading from the endian-marked history.dat for autocomplete purposes.

Comment 5

[Mark Mentovai]

Attachment: Account for endianness in formhistory.dat

This is a rough port of the endian marker stuff from bug 108134/bug 256885 (for history.dat) over to formhistory.dat.  I am using literal "BBBB" and "llll" as endian markers instead of "BE" and "LE" because nsFormHistory only deals in UTF-16, and markers are needed that look the same in UTF-16BE and UTF-16LE.

This patch depends on bug 328981 to properly byte-swap and display autocomplete entries as needed.

nsFormHistoryImporter in nsStorageFormHistory.cpp will need to be updated so Places will properly be able to import from arbitrary-endianness formhistory.dat files:

http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/toolkit/components/satchel/src/nsStorageFormHistory.cpp&rev=1.6&mark=569#557

I will do that once the markers and implementation here are final.

Note that this is a 1.8.0 branch blocker and I intend to seek 1.8.0 branch approval for this patch.  A file format is changing slightly, but in a harmless and backwards-compatible way.  This is needed to properly support x86 Mac users, who would otherwise have corrupt entries in their existing formhistory upon upgrading from ppc to x86 Firefox.

As with bug 328981, I'd like to land this on the trunk and 1.8 branches to keep that code from becoming stale.  Even though it will die there soon due to Places, it's not dead yet.

Comment 6

[Brian Ryner (not reading)]

Comment on attachment 213833 [details] [diff] [review]
Account for endianness in formhistory.dat

>+static void SwapBytes(PRUnichar* aDest, const PRUnichar* aSrc, PRUint32 aLen)
>+{
>+  for(PRUint32 i = 0; i < aLen; i++)
>+  {
>+    PRUnichar aChar = *aSrc++;
>+    *aDest++ = (0x00FF & (aChar >> 8)) | (0xFF00 & (aChar << 8));

This is equivalent to just  (aChar << 8) | (aChar << 8) isn't it?  (bits added from shifting will be 0 already)

>@@ -740,16 +798,17 @@ nsFormHistory::AutoCompleteSearch(const 
>         result->RemoveValueAt(i, PR_FALSE);
>     }
>   } else {
>     result = do_CreateInstance("@mozilla.org/autocomplete/mdb-result;1");
> 
>     result->SetSearchString(aInputValue);
>     result->Init(mEnv, mTable);
>     result->SetTokens(kToken_ValueColumn, nsIAutoCompleteMdbResult::kUnicharType, nsnull, nsIAutoCompleteMdbResult::kUnicharType);
>+    result->SetReverseByteOrder(mReverseByteOrder);
> 

huh, how does this work?

>+nsresult
>+nsFormHistory::InitByteOrder(PRBool aForce)
>+{
>+  // bigEndian and littleEndian are endianness markers that are stored in
>+  // the formhistory db as UTF-16.  Define them to be strings easily
>+  // recognized in either endianness.
>+  nsAutoString bigEndianByteOrder((PRUnichar*)"BBBB");
>+  nsAutoString littleEndianByteOrder((PRUnichar*)"llll");

can these just be NS_NAMED_LITERAL_STRING ?

Comment 7

[Mark Mentovai]

(In reply to comment #6)
> This is equivalent to just  (aChar << 8) | (aChar << 8) isn't it?  (bits added
> from shifting will be 0 already)

Right shift MAY leave the sign bit set, so at least that needs to be masked.

> >+    result->SetReverseByteOrder(mReverseByteOrder);
> 
> huh, how does this work?

Bug 328981.

> >+nsresult
> >+nsFormHistory::InitByteOrder(PRBool aForce)
> >+{
> >+  // bigEndian and littleEndian are endianness markers that are stored in
> >+  // the formhistory db as UTF-16.  Define them to be strings easily
> >+  // recognized in either endianness.
> >+  nsAutoString bigEndianByteOrder((PRUnichar*)"BBBB");
> >+  nsAutoString littleEndianByteOrder((PRUnichar*)"llll");
> 
> can these just be NS_NAMED_LITERAL_STRING ?

No, because that'll give endian-dependent UTF-16 strings like "\0B\0B\0B\0B", and that'll never match "B\0B\0B\0B\0" with the other endianness.

Comment 8

[Mark Mentovai]

Attachment: v2, avoid an AND in SwapBytes

Comment 9

[Mark Mentovai]

Attachment: v3, use extended nsIAutoCompleteMdbResult2 interface

Companion to 328981 v3.

Comment 10

[Brian Ryner (not reading)]

Comment on attachment 213852 [details] [diff] [review]
v3, use extended nsIAutoCompleteMdbResult2 interface

>--- mozilla/toolkit/components/satchel/src/nsFormHistory.cpp	19 Jun 2005 18:09:51 -0000	1.26
>+++ mozilla/toolkit/components/satchel/src/nsFormHistory.cpp	3 Mar 2006 03:53:34 -0000
>@@ -678,19 +710,34 @@ nsFormHistory::AppendRow(const nsAString
>   
>   return NS_OK;  
> }
> 
> nsresult
> nsFormHistory::SetRowValue(nsIMdbRow *aRow, mdb_column aCol, const nsAString &aValue)
> {
>   PRInt32 len = aValue.Length() * sizeof(PRUnichar);
>+  PRUnichar *swapval = nsnull;
>+  mdbYarn yarn = {nsnull, len, len, 0, 0, nsnull};
>+  const nsPromiseFlatString& buffer = PromiseFlatString(aValue);
>+
>+  if (mReverseByteOrder) {
>+    swapval = (PRUnichar *)malloc(len);

new PRUnichar[len] is generally better in C++ code if there's no specific reason to use malloc/free.

>+  if (swapval)
>+    free(swapval);

If you do that, don't forget to change this to delete (and you can remove the null check).

Same comments apply to GetRowValue

Why not just make OpenDatabase return failure if the meta row couldn't be created/located? Then you don't need to check for it separately.

NS_WARNiNGs don't need \n, they do that for you.

Looks ok otherwise.

Comment 11

[Mark Mentovai]

Attachment: Addresses review comments

Fixed on the trunk (with slight changes for context).

Comment 12

[Tim Riley [:timr]]

Comment on attachment 214224 [details] [diff] [review]
Addresses review comments

a=timr for drivers.  We were waiting for the bryner review.  Got that!  Land it!

Comment 13

[Mark Mentovai]

Landed on 1_8 and 1_8_0 branches.

Comment 14

[Jay Patel [:jay]]

Someone with an x86 Mac needs to verify this fix.

Comment 15

[Dave Liebreich [:davel]]

Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.2) Gecko/20060310 Firefox/1.5.0.2

verified in universal binary build from maya

Comment 16

[Nick Thomas [:nthomas] (UTC+12)]

Could bug 334464 and bug 334844 be fallout from the changes here ?

Comment 17

[Mark Mentovai]

Probably - investigating.