180452 - Manage Bookmarks who is normally Pomme(COMMAND)+B is become B

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[AMBROISE JEAN-MARIE]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.2) Gecko/20021114
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X; en-US; rv:1.2) Gecko/20021115

The key for The Manager Bookmarks is normally on a Macintosh Pomme(COMMAND)+B
and is become B in version 20021115.

When you type B on your keyboard you couldn't have the letter B but The Manage
Bookmarks appear every time

Reproducible: Always

Steps to Reproduce:
1.write letter B in the browser
2.
3.

Actual Results:  
The manager Bookmarks appear

Expected Results:  
the letter B

Comment 1

[Jo Hermans]

Worksforme in build 2002111208 on Mac OS 9.2.2.

Reporter, is it just command-B that fails to work, or is it all command-keys
(command-c, command-w, ...). Can you also check Preferences -> Debug, to see
that the 'accesskey' is still 224 (= command-key) ?

Comment 2

[AMBROISE JEAN-MARIE]

All command keys works fine except in Edit Menu --> command Copy: POMME-C has 
deseappeared and in Bookmarks Menu --> Manage Bookmarks: symbol POMME has 
deseappeared, there is just the letter B and POMME-B don't work and does nothing. And 
letter B launch Manager Bookmarks. In debug Menu, Form Manager is write after the 
word Manage with asian symbols !!!

The Build of Mozilla who have these problems is: Mozilla/5.0 (Macintosh; U; PPC Mac 
OS X; en-US; rev:1.3a) Gecko/20021115. (latest version of Mozilla yesterday).

I don't understand about "check Preferences-> Debug, to see
that the 'accesskey' is still 224 (= command-key) ?". What I have to do In Preferences 
-->Debug ?
Excuse me, I am French and my english is not good and it is not easy to me to explain to 
you clearly all the problems. Je parle beaucoup mieux en français :-) Thanks and I am 
sorry but I try to help you to describe this bug.

Comment 3

[Jason Bassford]

It's just the Bookmark Manager accelerator that's busted.  I'm seeing this too
with 2002111504 under XP.  If you look at the Bookmarks menu it's listing itself
as simply "+B".

-> All/All (to catch Windows as well as MacOS) and confirming.

Comment 4

[Jason Bassford]

Should this be moved to Keyboard Navigation?

Comment 5

[AMBROISE JEAN-MARIE]

But the Command Copy in the Edit Menu as also deseappeared and the name of 
Debug-->Form Manager with asian symbols behind Manage....... ????

Comment 6

[Jason Bassford]

I can also confirm that.  To summarize, here's what's broken:

Edit -> Copy is missing.
Bookmarks -> Manage Bookmarks is set to "+B" only.  (No accelerator.)
Debug -> Form Manager display Asian characters.

This definitely doesn't belong in the Bookmarks component.  

-> XP Tookkit/Widgets: Menus

Comment 7

[Jason Bassford]

Suggest changing Summary of bug to "Dropdown menus and shortcuts are wrong /
missing."

Comment 8

[Shoshannah Forbes]

Build 2002111508 on osx: This is maddening- every time I hit "B", the bookmark
manager opens. I can't type url's that contain the letter b (for example,
bugzilla.mozilla.org ...), can't type b in forms etc.
Yuck.
Please fix this fast- it makes Mozilla unusable.

Comment 9

[Shoshannah Forbes]

Attachment: screenshot of broken menus under osx

Comment 10

[AMBROISE JEAN-MARIE]

I am totally agree with Shoshannah Forbes, and this is the reason why I enter this new 
bug. Thanks for the screenshot of broken menus under osx, I want to do this but I don't 
know how join it to the report of the bug (now I know that it is necessary to create an 
attachment with the screenshot).  I am not familiar with Bugzilla. Thanks et bonjour de la 
France :-)

Comment 11

[Shoshannah Forbes]

This is worse then it seems: When I switch to the Hebrew keyboard, and type in 
Hebrew, every time I hit the letter "nun" (which is at the same physical 
location as the "b" key when I am in English layout), the bookmrak manager pops 
up.
This should never happen- I am not typing in English at all- as long as the 
system is concerned, the letter "b" does not exsist there at all at that moment.

Comment 12

[Jo Hermans]

Just an idea : maybe it's a fallout of bug 177401. Can you try to delete the
'XUL fastload file', which can be found in the profile-directory? Depending on
your OS, it's XUL.mfl, XUL FastLoad File or XUL.mfasl. The file will be rebuild
when you restart.

Comment 13

[u60234]

*** Bug 180422 has been marked as a duplicate of this bug. ***

Comment 14

[Shoshannah Forbes]

In response to coment #12:
intresting. althugh this build was installed with a fresh profile (no leftovers
from older profiles) deleting XUL FastLoad File seems to fix the problems.

Comment 15

[Jo Hermans]

Adding Alec, who might be able to help

Comment 16

[Boris Zbarsky [:bzbarsky]]

To alecf.  Alec, did you change the actual format of the file?  If so, you need
to bump the version number.  If not, your code has a bug.  ;)

Comment 17

[Jo Hermans]

Shoshannah, does the bug comes back after you restart Mozilla ?

Comment 18

[Patrick]

this is probably related to (or even dupe of) bug 180371

Comment 19

[Jo Hermans]

*** Bug 180371 has been marked as a duplicate of this bug. ***

Comment 20

[Patrick]

Not because i filed bug 180371 but why is the older bug (bug 180371) marked as a
duplicate, and not the one that's filed later?

Comment 21

[Boris Zbarsky [:bzbarsky]]

Because this one is assigned to the right person already and as such is closer
to being fixed.

Comment 22

[Alec Flett]

Boy, I'm pretty darn sure I didn't change the file format... besides, if it was
that broken, we'd have corruption issues and the XUL fastload file would just be
invalidated..the results would be something along the lines of lots of
assertions in debug builds, and dramatically slower startup times.. I'll
continue to investigate, but I just don't know how I could have cause this, if
everything else is working.

Comment 23

[Shoshannah Forbes]

in answer to comment #17: yep. Restarting mozilla brings the bug back. deleting
the file again get's rid of it. then restart again and it is back, etc. etc.

Comment 24

[Boris Zbarsky [:bzbarsky]]

okee.  I just tried locally backing out the checkin from
http://bonsai.mozilla.org/cvsquery.cgi?treeid=default&module=all&branch=HEAD&branchtype=match&dir=&file=&filetype=match&who=alecf%25netscape.com&whotype=match&sortby=Date&hours=2&date=explicit&mindate=2002-11-14+00%3A00%3A00&maxdate=2002-11-15+00%3A00%3A00&cvsroot=%2Fcvsroot
and that fixes the bug.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: "fixes" this bug

Oh, fun.  I added this:

    NS_ASSERTION(aCount % 2 == 0, "Odd segment length???");

to WriteSegmentToString() in nsBinaryStream.  Guess what? It fires a bunch of
times at startup. We're getting buffer boundaries falling in the middle of a
PRUnichar.

This attachment is a "fix" (leaking, using static globals, etc, etc, but it
makes the menus look right, which shows that the problem is indeed the byte
we're losing due to the 

    PRUint32 segmentLength = aCount / sizeof(PRUnichar);

that we do).

Comment 26

[Boris Zbarsky [:bzbarsky]]

ccing darin and dougt in case they have clean ideas for this. Perhaps the
closure needs to hold not just the string but also up to sizeof(PRUnichar) - 1
bytes, and a count of how many bytes it's holding. That way we can correctly do
carryover... (note that the code assumes that sizeof(PRUnichar) == 2 anyway in
its use of NS_SWAP16(), so we could just store 1 byte and a PRPackedBool)

Comment 27

[Boris Zbarsky [:bzbarsky]]

So the upshot is the nsBinaryStream::ReadString is broken.  Of course on
big-endian platforms we don't need to swap the carryover, but we still have to
carry it over.

Comment 28

[Darin Fisher]

bz: yeah, there's probably no way around having to maintain some state between
calls to WriteSegmentToString.  perhaps a struct like this would do the trick:

struct {
  nsAString   *mStr;
  PRPackedBool mHaveCarryByte;
  char         mCarryByte;
};

a pain, but i don't see any cleaner way.  hmm... what about
nsIUnicharInputStream... could we leverage any of that code?

Comment 29

[Boris Zbarsky [:bzbarsky]]

nsIUnicharInputStream impls either need an nsString or do encoding
conversion.... so I'm not sure how we could just use them here.

Comment 30

[R.K.Aa.]

is bug 180355 a dup?

Comment 31

[Shoshannah Forbes]

*** Bug 180355 has been marked as a duplicate of this bug. ***

Comment 32

[Jason Bassford]

> deleting the file again get's rid of it. then restart again
> and it is back, etc.

Here's a permanent workaround.

1. Exit Mozilla and delete the xul.mlf file.
2. Create a new, empty file called xul.mlf - in Windows use "copy nul xul.mlf"
(or in Explorer right-click -> New -> Text file and rename it); in Linux "touch
xul.mlf".
3. Mark it read only.  (In Linux "chmod 444 xul.mlf".)

Mozilla will be never be able to (re)create its own version of the file and
everything will work as it should.

This, of course, begs the question: What good is the file if Mozilla works just
fine without it?  I'm not seeing any functionality loss by have a 0-byte file by
that name.  In what situation WOULD I be seeing something not working properly
without it?

Comment 33

[timeless]

fastload is a cache. it makes things start a lot faster. but if it happens to be
corrupt, then ...

My suggestion is that we round up instead of worrying about bits, i think that
it makes endian games simpler. at worst there's an extra null field being
allocated and ignored. however i haven't spent time investigating.

Comment 34

[R.K.Aa.]

i have to delete XUL.mfasl 5 times a day now. It gets corrupted all the time,
leading to mailnews not starting, sidebar not showing, mail crashing, weird
menus with items that come and go. I think severity is worse than "major"..

Comment 35

[Jason Bassford]

> i have to delete XUL.mfasl 5 times a day now.

Read comment 32. (And substitue XUL.mfasl in your case.)  There is no need for
multiple deletions.

Comment 36

[Alec Flett]

sorry for the delay, I've started working on this right now. it shouldn't be too
hard to keep the extra byte around for the next read/write, so I'm going to do that.

Comment 37

[Alec Flett]

Attachment: handle all the edge cases

ok, here's a better solution, which covers all the edge cases I came up with
(and I documented them, they're in the comments)

can I get r=darin/sr=bzbarsky?

Comment 38

[Alec Flett]

Comment on attachment 106690 [details] [diff] [review]
handle all the edge cases

I have to be in a ton of meetings all day today, and may not be able to check
this in myself. I'm also not 100% sure of the endian issues.

So my request is: after I get reviews, can someone try this on OSX, and then if
it is working, check it in? If not, can you try swapping the IS_LITTLE_ENDIAN
lines and see if that makes it work?

Comment 39

[Darin Fisher]

Comment on attachment 106690 [details] [diff] [review]
handle all the edge cases

>Index: nsBinaryStream.cpp

>+    if (closure->mHasCarryoverByte) {
>+        char firstByte = *aFromSegment;
>+#ifdef IS_LITTLE_ENDIAN
>+        PRUnichar newByte = (closure->mCarryoverByte << 8) & firstByte;
>+#else
>+        PRUnichar newByte = (firstByte << 8) & closure->mCarryoverByte;
>+#endif
>+        outString->Append(newByte);

shouldn't these use OR instead of AND?	also, is there a problem shifting
a char by 8 bytes?  can you be certain that it wouldn't use a byte sized
register for this?  (linux gcc seems to be ok with this.)

also, maybe newChar would be a better name instead of newByte since it's
not a byte.

Comment 40

[Boris Zbarsky [:bzbarsky]]

*** Bug 180788 has been marked as a duplicate of this bug. ***

Comment 41

[timeless]

*** Bug 180790 has been marked as a duplicate of this bug. ***

Comment 42

[Jean-Francois Ducarroz]

I tested Alecf patch (but using a or instead of a AND) and it did not solve the
problem on Mac OS X for me. I even try to revers the LITTLE_INDIAN test as
suggested! I deleted my chome directory and rebuilt it each time to be sure I
was in a clean state.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 106690 [details] [diff] [review]
handle all the edge cases

+	 char firstByte = *aFromSegment;

at least assert tht aCount != 0 here!  ;)

bit-shifting chars is probably a bad idea. At least cast to PRUnichar before
doing so.

the logic for little vs big endian is flipped -- for big endian we want to have
carryover byte first, then firstByte.

Mac is big endian.

This is a problem on all platforms, not just OSX or only Mac.

The logic for determining whether we have a carryover byte is wrong; instead of
using *aWriteCount != aCount (which is true if you had a carryover the
_previous_ time, for instance) it should be checking whether the adjusted count
(after previous carryover is taken care of) is 1 mod 2.  If it is, we have a
new carryover.

Comment 44

[Boris Zbarsky [:bzbarsky]]

Attachment: slight modifications

OK.  I think I was actually wrong about the logic being flipped.  But I've
revamped that part to be clear anyway.	I think this should work, but I have no
building tree, so someone needs to build & test...

Comment 45

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Comment on attachment 106750 [details] [diff] [review]
slight modifications

Applying this patch caused b to return to its old behavior.  (I still have the
old fastload file intact, too.)

Comment 46

[Alec Flett]

Comment on attachment 106750 [details] [diff] [review]
slight modifications

"b"? :)

I assume you mean that the new patch worked? 
if so, sr=alecf

Comment 47

[Boris Zbarsky [:bzbarsky]]

checked in.

Comment 48

[timeless]

*** Bug 181247 has been marked as a duplicate of this bug. ***

Comment 49

[timeless]

*** Bug 181856 has been marked as a duplicate of this bug. ***

Comment 50

[Jo Hermans]

*** Bug 181990 has been marked as a duplicate of this bug. ***

Comment 51

[Sebastian Biallas]

*** Bug 181992 has been marked as a duplicate of this bug. ***