firefox history database is endian; corrupts the display of titles

RESOLVED WONTFIX

Status

()

--
major
RESOLVED WONTFIX
14 years ago
10 years ago

People

(Reporter: jmorzins, Unassigned)

Tracking

({dataloss})

unspecified
dataloss
Points:
---
Bug Flags:
blocking-aviary1.0 -

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(2 attachments, 5 obsolete attachments)

(Reporter)

Description

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

Comment 1

14 years ago
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. 
CC'ing some of participants in the older bug.
Status: UNCONFIRMED → NEW
Ever confirmed: true
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....
Severity: normal → major
Flags: blocking-aviary1.0?
Keywords: dataloss
Created attachment 157838 [details] [diff] [review]
port of seamonkey patch

pretty much a perfectly straight port.	Needs testing by someone with access to
both types of machines though.
Assignee: firefox → mconnor
Status: NEW → ASSIGNED
Comment on attachment 157838 [details] [diff] [review]
port of seamonkey patch

Look away, nothing to see here
Attachment #157838 - Attachment is obsolete: true
Created attachment 157843 [details] [diff] [review]
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
Attachment #157839 - Attachment is obsolete: true
this patch sucks, I'll fix it tomorrow, seriously.
Flags: blocking-aviary1.0? → blocking-aviary1.0+
Not a "blocker"
Flags: blocking-aviary1.0+ → blocking-aviary1.0-
Just for clarity, this would be rather a blocker to deploying Firefox at MIT as
a default browser (the current default is SeaMonkey).
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!
Flags: blocking-aviary1.0- → blocking-aviary1.0?
Created attachment 160424 [details] [diff] [review]
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.
Attachment #157843 - Attachment is obsolete: true
This mork endian dependency is ridiculous -- thanks, mconnor.  I say we want
this fixed for 1.0.

/be
Flags: blocking-aviary1.0? → blocking-aviary1.0+
(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*?

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). 
Flags: blocking-aviary1.0+ → blocking-aviary1.0-
(Reporter)

Comment 16

14 years ago
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.)
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...
Created attachment 160775 [details] [diff] [review]
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.
Attachment #160424 - Attachment is obsolete: true

Updated

14 years ago
Attachment #160775 - Flags: review?(shaver)
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...)
Attachment #160775 - Flags: review?(shaver) → superreview+

Comment 20

14 years ago
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
checked in a fix for the mingw bustage, this is probably mingw gcc 3.3.1, fixed
long after the initial fix in mozilla.

Status: ASSIGNED → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED

Comment 22

14 years ago
Actually, it was gcc 3.4.2
yeah, I should have added "or greater" there.  I'm assuming this fixed the bustage?

Comment 24

14 years ago
Yup, fixed the bustage....thanks for the quick reponse.

Comment 25

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

14 years ago
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.
*cough* sqlite handles endianness issues quite nicely *cough*

Reopening; we're apparently still endian-sensitive.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Comment 28

14 years ago
Created attachment 181453 [details] [diff] [review]
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

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

14 years ago
Created attachment 181561 [details] [diff] [review]
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.
Attachment #181453 - Attachment is obsolete: true

Comment 31

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

13 years ago
This will not be fixed for Firefox 1.5.0.x and FF 2 has the new places history implementation.
Status: REOPENED → RESOLVED
Last Resolved: 14 years ago13 years ago
Resolution: --- → WONTFIX
(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

13 years ago
Right, you could just as well mark this bug FIXED.
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
Need a better resolution status, and more certainty.

/be
Status: RESOLVED → REOPENED
Resolution: WONTFIX → ---
(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

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

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

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

Updated

12 years ago
Assignee: mconnor → nobody
Status: REOPENED → NEW
QA Contact: mozilla → history

Updated

10 years ago
Component: History → Bookmarks & History
QA Contact: history → bookmarks
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.
Status: NEW → RESOLVED
Last Resolved: 13 years ago10 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.