256885 - firefox history database is endian; corrupts the display of titles

Status: RESOLVED WONTFIX

(Firefox :: Bookmarks & History, defect)

Description

[jmorzins]

User-Agent:       Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.7) Gecko/20040630 Firefox/0.9.1
Build Identifier: Mozilla/5.0 (X11; U; SunOS sun4u; en-US; rv:1.7) Gecko/20040630 Firefox/0.9.1

This bug is best experienced with screenshots, so I've put some up at
http://web.mit.edu/jmorzins/www/mozilla/history-endian-firefox.html

What's wrong:
Firefox stores titles into the history database in an endian-dependent fashion.

What happens:
If a user switches between little-endian machines (like PCs) and big-endian
machines (like Sun workstations), they will only be able to read half of their
history titles at a time.  The little-endian machine is able to read the
little-endian titles, and big-endian ones come out looking like garbled Chinese.
Likewise, the big-endian machine will be able to read the big-endian titles,
with little-endian titles coming out as garbled Chinese.

What should have happened:
Firefox should either store names in an endian-independent fashion, or should
tag the Unicode strings in a way that allows Firefox to recognize what
endianness the strings are.


Reproducible: Always
Steps to Reproduce:
This bug will be trivial for anyone at a large Unix site to duplicate, but
perhaps there aren't that many of us left.

 1) Run Firefox on a little-endian machine, like an Intel PC running Linux.
    Visit some pages, such as:
      http://web.mit.edu/jmorzins/www/mozilla/little-endian.html
 2) Quit Firefox (to save your history).
 3) Run Firefox on a big-endian machine, like a Sun SunBlade running Solaris.
    Visit some pages, such as:
      http://web.mit.edu/jmorzins/www/mozilla/big-endian.html
 4) Quit Firefox (to save your history).
 5) Now restart Firefox. Notice that:
       (a) if you type URLs into the location bar, the pop-up autocompletion
           menu has garbled (Chinese!) titles for other-endian platforms
       (b) if you view your history, the list of titles in the history has
           garbled (Chinese!) titles for other-endian platforms.

Comment 1

[jmorzins]

I forgot to add that I reported this bug against Mozilla nearly four years ago -
bug 108134. It was fixed in Mozilla about a year ago. Apparently it hasn't yet
been fixed in Firefox. 

Comment 2

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

CC'ing some of participants in the older bug.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Firefox branched history altogether back in 2002.  The fix for bug 108134 was
never ported to the forked history impl (nor the fix for performance bug 180109,
nor a bunch of other things, probably).

mconnor, got time for some porting?

Setting flags appropriately....

Comment 4

[Mike Connor [:mconnor]]

Attachment: port of seamonkey patch

pretty much a perfectly straight port.	Needs testing by someone with access to
both types of machines though.

Comment 5

[Mike Connor [:mconnor]]

Comment on attachment 157838 [details] [diff] [review]
port of seamonkey patch

Look away, nothing to see here

Comment 6

[Mike Connor [:mconnor]]

Attachment: the patch

Comment 7

[Mike Connor [:mconnor]]

Attachment: what actually got checked in

missed the bit about various bits of code cleanup at checkin time in the last
comment of the bug.

this is basically a cvs diff of the original checkin + bustage fix, applied to
the toolkit side of the fork

Comment 8

[Mike Connor [:mconnor]]

this patch sucks, I'll fix it tomorrow, seriously.

Comment 9

[Ben Goodger (use ben at mozilla dot org for email)]

Not a "blocker"

Comment 10

[Boris Zbarsky [:bzbarsky]]

Just for clarity, this would be rather a blocker to deploying Firefox at MIT as
a default browser (the current default is SeaMonkey).

Comment 11

[Mike Shaver (:shaver emeritus)]

Renominating: history database corruption is dataloss, and
regression-vs-seamonkey, which both seem to be criteria for blockers.

If I'm wrong, please explain how here in more detail than the last time!

Comment 12

[Mike Connor [:mconnor]]

Attachment: non sucky version of the original CVS checkin port

bz, shaver, can you double-check that I did it right this time?  I'm pretty
bagged, but it looks fine to me.

Comment 13

[Brendan Eich [:brendan]]

This mork endian dependency is ridiculous -- thanks, mconnor.  I say we want
this fixed for 1.0.

/be

Comment 14

[Boris Zbarsky [:bzbarsky]]

(In reply to comment #12)

> bz, shaver, can you double-check that I did it right this time?

The cpp is fine.  The SwapBytes decls in the header don't match the cpp; remove
the one and make the other take PRUnichar*?

Comment 15

[Ben Goodger (use ben at mozilla dot org for email)]

This is *NOT* a blocker. Everyone here involved with aviary work should be
involved in our non-blocker list, not bugs that require two different platforms
and a shared profile (which we do not claim to support). 

Comment 16

[jmorzins]

As long as the bug gets fixed, I'm not terribly interested in flames about
whether the bug is or is not a blocker.

I will point out, though, that while Netscape/Mozilla has never supported
simultaneous sharing of config files, it has supported endian-neutral config
files since  1995 or earlier.  There is a long history of Netscape/Mozilla being
useful for large Unix-focused organizations, and it would be a shame if these
organizations were now told that they need to find a different browser.

(These history bugs, in fact, are a case where Mozilla/Firefox is still trying
to maintain an endian-neutral config file.  There is a bug in the conversion
from Unicode to the ascii-encoding-scheme, and once the bug is fixed, Firefox
will be returned to the happy state where it is useful to us at MIT.)

Comment 17

[Boris Zbarsky [:bzbarsky]]

This bug does NOT require a shared profile, merely a profile that lives in a
network share.  And from the firefox point of view, it's the same ("unix")
platform, which we DO claim to support, last I checked.

Forget it, though.  If you're going to flame without trying to get your facts
straight...

Comment 18

[Mike Connor [:mconnor]]

Attachment: with the header changes

I remember now, I wasn't done porting the patch from the cvs diff between
versions, so that's why the header wasn't changed.

Comment 19

[Mike Shaver (:shaver emeritus)]

Comment on attachment 160775 [details] [diff] [review]
with the header changes

Oh crapsticks, _I_'m the reason this didn't go in?  sr=shaver.

(But boy, wouldn't I love to get rid of mork instead...)

Comment 20

[David G King]

This patch breaks my Win32/cygwin/MinGW build of Firefox :-

e:/mozilla/source/firefox/mozilla/toolkit/components/history/src/nsGlobalHistory
.cpp: In member function `void nsGlobalHistory::SwapBytes(const PRUnichar*, PRUn
ichar*, PRInt32)':
e:/mozilla/source/firefox/mozilla/toolkit/components/history/src/nsGlobalHistory
.cpp:1060: error: invalid conversion from `const PRUnichar*' to `const PRUint16*
'
e:/mozilla/source/firefox/mozilla/toolkit/components/history/src/nsGlobalHistory
.cpp:1061: error: invalid conversion from `PRUnichar*' to `PRUint16*'
make[6]: *** [nsGlobalHistory.o] Error 1
make[6]: Leaving directory `/cygdrive/e/mozilla/source/firefox/mozilla/toolkit/c
omponents/history/src'
make[5]: *** [libs] Error 2
make[5]: Leaving directory `/cygdrive/e/mozilla/source/firefox/mozilla/toolkit/c
omponents/history'
make[4]: *** [libs] Error 2
make[4]: Leaving directory `/cygdrive/e/mozilla/source/firefox/mozilla/toolkit/c
omponents'
make[3]: *** [libs] Error 2
make[3]: Leaving directory `/cygdrive/e/mozilla/source/firefox/mozilla/toolkit'
make[2]: *** [libs] Error 2
make[2]: Leaving directory `/cygdrive/e/mozilla/source/firefox/mozilla'
make[1]: *** [alldep] Error 2
make[1]: Leaving directory `/cygdrive/e/mozilla/source/firefox/mozilla'
make: *** [alldep] Error 2

Comment 21

[Mike Connor [:mconnor]]

checked in a fix for the mingw bustage, this is probably mingw gcc 3.3.1, fixed
long after the initial fix in mozilla.

Comment 22

[David G King]

Actually, it was gcc 3.4.2

Comment 23

[Mike Connor [:mconnor]]

yeah, I should have added "or greater" there.  I'm assuming this fixed the bustage?

Comment 24

[David G King]

Yup, fixed the bustage....thanks for the quick reponse.

Comment 25

[Robert Basch]

Was this ever tested?  I have been working on integrating Firefox 1.0.2.
After finding that this fix was not included in that release, I tried
applying relevant patches to nsGlobalHistory.cpp and nsGlobalHistory.h.
It looks like the byte order is recorded properly in the file, but that
bytes do not get swapped when displaying titles on an other-endian
machine, i.e. after reading and matching relevant rows from the
database in an "Auto Complete" search.

Comment 26

[Robert Basch]

I think I see the problem.  The search result rows are now passed to
nsAutoCompleteMdbResult, which handles extracting the relevant tokens
from the row.  So nsAutoCompleteMdbResult::GetRowValue() needs logic
to do byte-swapping as appropriate.  Unless we want to reconsider the
idea of prepending byte-order markers to Unicode strings in the
database, this probably means adding to the nsAutoCompleteMdbResult
interface, to provide a way to tell it whether to reverse the byte
order.  I can try to work on such a change, unless someone objects
(or prefers to do it themselves).

Also, formhistory.dat seems to be byte order-dependent as well,
so I imagine that code will need to be changed analogously, though
that is probably less critical than fixing the global history file.

Comment 27

[Mike Shaver (:shaver emeritus)]

*cough* sqlite handles endianness issues quite nicely *cough*

Reopening; we're apparently still endian-sensitive.

Comment 28

[Robert Basch]

Attachment: Handle byte-swapping in nsAutoCompleteMdbResult

This patch adds byte-swapping logic to nsAutoCompleteMdbResult, and the
necessary
setup calls to nsGlobalHistory.  I have tested this using a home directory
shared
between Sun/Solaris (big-endian) and 386/Linux (little-endian) machines.

The patch is against our local repository, containing Firefox 1.0.2 plus the
previous nsGlobalHistory revisions for this bug.

Comment 29

[timeless]

Comment on attachment 181453 [details] [diff] [review]
Handle byte-swapping in nsAutoCompleteMdbResult

some of the indentation is off (don't use tabs!), and you need to bump the uuid
of the interface in this file:
>Index: toolkit/components/autocomplete/public/nsIAutoCompleteResultTypes.idl
>@@ -68,6 +68,8 @@
because you added a method:
>+  void setReverseByteOrder(in boolean reverse);

>Index: toolkit/components/autocomplete/src/nsAutoCompleteMdbResult.cpp
>@@ -51,7 +51,8 @@
>+	free(swapval);
>+      }

Comment 30

[Robert Basch]

Attachment: Updated patch to handle byte-swapping in nsAutoCompleteMdbResult

OK, here is a new patch.  I untabified, and regenerated a new uuid
with uuidgen (is there another preferred way to do that?).  Also,
I added reverseByteOrder as an attribute in the idl, so there are
now both GetReverseByteOrder and SetReverseByteOrder methods.

Comment 31

[Robert Basch]

Any chance of getting this fixed soon?  The last patch I submitted applies
cleanly to the latest sources, and still seems to work OK.  If there are
problems with it, please let me know.  Otherwise, can it be accepted and
committed?  We would really like to see this fixed in an upcoming Firefox
release.

Comment 32

[Benjamin Smedberg]

This will not be fixed for Firefox 1.5.0.x and FF 2 has the new places history implementation.

Comment 33

[Brendan Eich [:brendan]]

(In reply to comment #32)
> This will not be fixed for Firefox 1.5.0.x and FF 2 has the new places history
> implementation.


Which uses sqlite, which has no byte-order dependencies -- right?  IOW, you are not saying WONTFIX to the requirement on Firefox 2, only to the requirement on bad old Mork.

/be

Comment 34

[Benjamin Smedberg]

Right, you could just as well mark this bug FIXED.

Comment 35

[Brendan Eich [:brendan]]

Is this bug still relevant to Thunderbird?

Is this something that SeaMonkey folks care about?

Might it be worth fixing for Firefox 1.8.0.x or 1.0.y, to get the fix out before Firefox 2 is done?

Just askin'.

/be

Comment 36

[Brendan Eich [:brendan]]

Need a better resolution status, and more certainty.

/be

Comment 37

[Brendan Eich [:brendan]]

(In reply to comment #31)
> Any chance of getting this fixed soon?  The last patch I submitted applies
> cleanly to the latest sources, and still seems to work OK.  If there are
> problems with it, please let me know.  Otherwise, can it be accepted and
> committed?  We would really like to see this fixed in an upcoming Firefox
> release.

How did this miss Mozilla 1.8 / Firefox 1.5?  Looks like neither the bug's blocking1.8? flag, nor the patch's request flags, were set.

/be

Comment 38

[Benjamin Smedberg]

> Is this bug still relevant to Thunderbird?

Thunderbird and Seamonkey still use the version in xpfe/components/history which was fixed separately in bug 108034.

> Might it be worth fixing for Firefox 1.8.0.x or 1.0.y, to get the fix out
> before Firefox 2 is done?

the patch as written involves an interface change, and unless I am mistaken a file format change as well.

Comment 39

[Mark Mentovai]

Ongoing work, and a reimplemented patch that looks a hell of a lot like what's here, in bug 328981.  I don't know how this got missed, but not having had r? flags set looks like a good start.

Comment 40

[Robert Basch]

(In reply to comment #38)

> the patch as written involves an interface change, and unless I am mistaken a
> file format change as well.

The patch does not change the file format (the byte order is already recorded
properly); it only adds byte-swapping logic when reading titles.

Comment 41

[Marco Bonardo [:mak]]

Plaes uses sqlite, and sqlite databases are cross-platform, 2.0 is no more supported, so unless anyone has something against that i'm going to mark this as wontfix.