27739 - keybindings break in browser window after using profile manager

Status: VERIFIED DUPLICATE of bug 24645

(Core :: XML, defect, P3)

Description

[mjudge]

Potentially useless call stack follows.
I get this assertion while debuggin at random times when opening new pages or 
starting browser
MJ

NTDLL! 77f76274()
nsDebug::Assertion(const char * 0x01ce5a90, const char * 0x100a0240, const char 
* 0x01ce5a58, int 189) line 189 + 13 bytes
nsDebug::NotReached(const char * 0x01ce5a90, const char * 0x01ce5a58, int 189) 
line 296 + 22 bytes
nsCachedChromeChannel::OpenInputStream(nsCachedChromeChannel * const 0x0f6229b0, 
unsigned int 0, int -1, nsIInputStream * * 0x0012ebbc) line 189 + 21 bytes
nsXULKeyListenerImpl::LoadKeyBindingDocument(nsXULKeyListenerImpl * const 
0x0f2622f0, nsIURI * 0x0f622bf0, nsIDOMXULDocument * * 0x0012ec74) line 1180 + 
66 bytes
nsXULKeyListenerImpl::GetKeyBindingDocument(nsXULKeyListenerImpl * const 
0x0f2622f0, nsCAutoString & {...}, nsIDOMXULDocument * * 0x0012ed40) line 1083 + 
45 bytes
nsXULKeyListenerImpl::DoKey(nsIDOMEvent * 0x0f622de4, eEventType eKeyDown) line 
569 + 55 bytes
nsXULKeyListenerImpl::KeyDown(nsIDOMEvent * 0x0f622de4) line 439
nsEventListenerManager::HandleEvent(nsIPresContext * 0x0f497060, nsEvent * 
0x0012fad4, nsIDOMEvent * * 0x0012f838, unsigned int 2, nsEventStatus * 
0x0012fa3c) line 993 + 17 bytes
nsXULDocument::HandleDOMEvent(nsXULDocument * const 0x0e9d7250, nsIPresContext * 
0x0f497060, nsEvent * 0x0012fad4, nsIDOMEvent * * 0x0012f838, unsigned int 2, 
nsEventStatus * 0x0012fa3c) line 1913
nsXULElement::HandleDOMEvent(nsXULElement * const 0x0f21b3c0, nsIPresContext * 
0x0f497060, nsEvent * 0x0012fad4, nsIDOMEvent * * 0x0012f838, unsigned int 2, 
nsEventStatus * 0x0012fa3c) line 2957 + 39 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x0f285b10, nsIPresContext * 
0x0f497060, nsEvent * 0x0012fad4, nsIDOMEvent * * 0x0012f838, unsigned int 2, 
nsEventStatus * 0x0012fa3c) line 2951 + 39 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x0f2857f0, nsIPresContext * 
0x0f497060, nsEvent * 0x0012fad4, nsIDOMEvent * * 0x0012f838, unsigned int 2, 
nsEventStatus * 0x0012fa3c) line 2951 + 39 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x0f2853c0, nsIPresContext * 
0x0f497060, nsEvent * 0x0012fad4, nsIDOMEvent * * 0x0012f838, unsigned int 2, 
nsEventStatus * 0x0012fa3c) line 2951 + 39 bytes
nsXULElement::HandleDOMEvent(nsXULElement * const 0x0f286f90, nsIPresContext * 
0x0f497060, nsEvent * 0x0012fad4, nsIDOMEvent * * 0x0012f838, unsigned int 2, 
nsEventStatus * 0x0012fa3c) line 2951 + 39 bytes
nsXULElement::HandleChromeEvent(nsXULElement * const 0x0f286fbc, nsIPresContext 
* 0x0f497060, nsEvent * 0x0012fad4, nsIDOMEvent * * 0x0012f838, unsigned int 2, 
nsEventStatus * 0x0012fa3c) line 3970
GlobalWindowImpl::HandleDOMEvent(GlobalWindowImpl * const 0x0f456034, 
nsIPresContext * 0x0f497060, nsEvent * 0x0012fad4, nsIDOMEvent * * 0x0012f838, 
unsigned int 2, nsEventStatus * 0x0012fa3c) line 387
nsDocument::HandleDOMEvent(nsDocument * const 0x0f4966f0, nsIPresContext * 
0x0f497060, nsEvent * 0x0012fad4, nsIDOMEvent * * 0x0012f838, unsigned int 2, 
nsEventStatus * 0x0012fa3c) line 2477
nsGenericElement::HandleDOMEvent(nsIPresContext * 0x0f497060, nsEvent * 
0x0012fad4, nsIDOMEvent * * 0x0012f838, unsigned int 1, nsEventStatus * 
0x0012fa3c) line 1027 + 39 bytes
nsHTMLHtmlElement::HandleDOMEvent(nsHTMLHtmlElement * const 0x0f497a1c, 
nsIPresContext * 0x0f497060, nsEvent * 0x0012fad4, nsIDOMEvent * * 0x00000000, 
unsigned int 1, nsEventStatus * 0x0012fa3c) line 191
PresShell::HandleEvent(PresShell * const 0x0f4bfe64, nsIView * 0x0f4d53c0, 
nsGUIEvent * 0x0012fad4, nsEventStatus * 0x0012fa3c) line 2941 + 39 bytes
nsView::HandleEvent(nsView * const 0x0f4d53c0, nsGUIEvent * 0x0012fad4, unsigned 
int 8, nsEventStatus * 0x0012fa3c, int & 0) line 799
nsView::HandleEvent(nsView * const 0x0f4d5ac0, nsGUIEvent * 0x0012fad4, unsigned 
int 8, nsEventStatus * 0x0012fa3c, int & 0) line 784
nsView::HandleEvent(nsView * const 0x0f4be3d0, nsGUIEvent * 0x0012fad4, unsigned 
int 28, nsEventStatus * 0x0012fa3c, int & 0) line 784
nsViewManager::DispatchEvent(nsViewManager * const 0x0f4beab0, nsGUIEvent * 
0x0012fad4, nsEventStatus * 0x0012fa3c) line 1704
HandleEvent(nsGUIEvent * 0x0012fad4) line 69
nsWindow::DispatchEvent(nsWindow * const 0x0f4be2b4, nsGUIEvent * 0x0012fad4, 
nsEventStatus & nsEventStatus_eIgnore) line 493 + 10 bytes
nsWindow::DispatchWindowEvent(nsGUIEvent * 0x0012fad4) line 514
nsWindow::DispatchKeyEvent(unsigned int 133, unsigned short 0, unsigned int 18) 
line 1884 + 15 bytes
nsWindow::OnKeyDown(unsigned int 18, unsigned int 8248) line 1913 + 25 bytes
nsWindow::ProcessMessage(unsigned int 260, unsigned int 18, long 540540929, long 
* 0x0012fd64) line 2200 + 32 bytes
nsWindow::WindowProc(HWND__ * 0x002704c4, unsigned int 260, unsigned int 18, 
long 540540929) line 673 + 27 bytes
USER32! 77e71268()

Comment 1

[David Hyatt]

Can you just remove the assertion for now?  It doesnt' seem to cause any 
trouble to remove it.

Comment 2

[Chris Waterson]

mjudge: how do you reproduce the problem? I have never seen this.

Comment 3

[Chris Waterson]

I just hit this, although I'm not sure how.

What's happening here is that the platform keybindings 
(in "chrome://global/content/platformBrowserBindings.xul") has already been 
read in once, and for some reason, we're trying to re-read them to handle a 
keypress.

Since the keybindings are in a XUL document, the second time we read the 
document, the "chrome:" protocol handler will cough up a bogo-channel (the 
"cached chrome channel"). This is where the badness begins.

We end up terminating early, without ever having created the keybinding 
document (e.g., for the URL bar) in 
nsXULKeyListenerImpl::LoadKeyBindingDocument(). This seems bad, because the 
normal keybindings just won't work properly in this case.

My guess is that this *should* have been found in the keybindings hashtable, 
way up in nsXULKeyListenerImpl::GetKeyBindingDocument(), but it's not.

I took a quick look at the mKeyBindingTable, and was surprised to find that it 
has *zero* entries. Does it ever get flushed? If not, then who else would've 
loaded that URL?

Oooooh. I bet I know what happened.

1. We created a nsXULKeyListener. This created the global hashtable and loaded 
the platformBrowserBindings.xul.

2. We destroyed the one-and-only nsXULKeyListener. This destroyed the global 
hashtable.

3. We created another nsXULKeyListener. It created a brand new EMPTY hashtable. 
So when we try to look up the keypress doc, it's not there, but has already 
been loaded once.

If I'm right, we can do one of two things:

1. Turn the keybinding cache into a bona fide service that will live for the 
lifetime of the app. Kinda wasteful, because a bunch of keybinding docs will 
live forever.

2. Make LoadKeyBindingDocument() smart enough to deal with the XUL prototype 
cache directly, and look there first before trying to load a document by hand.

I kinda like approach (2). Who wants to do the honors? Or is this code all 
going to be obviated by some rewrite brain fart, hyatt? ;-)

Comment 4

[Chris Waterson]

Ok, talked to hyatt. He is rewriting everything anyway. So I'm marking this as 
WONTFIX.

Comment 5

[Christine Hoffman]

Verified WONTFIX

Comment 6

[Chris Waterson]

Re-opening because so many people bitch about it.

Comment 7

[Chris Waterson]

hyatt: are you going to re-write the key binding stuff soon? If not, reassign
the bug back to me and I will take 30 minutes and teach the XUL key code about
the chrome cache, and check it in when M15 opens.

Comment 8

[Chris Waterson]

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

Comment 9

[Chris Waterson]

PDT: This bug used to be called "random assertions while debugging", but it 
actually is indicative of a much more serious problem. Here is the impact of 
this bug.

If you ever press a key in the (other than "enter") in the profile manager, or 
profile manager wizard, then keybindings will be broken in the browser; e.g., 
Page Up/Down won't work, the back arrow will not work in the URL bar, etc.

The analysis above explains exactly why this happens, but a cheap and dirty fix 
(which I'll attach momentarily) is to simply leak the XUL keybindings cache, 
and never free it when the number of open XUL documents goes to zero.

This will introduce a bit of extra leak at shutdown, but will get rid of a lot 
of these flaky "keys don't work" bugs, and will also avoid the dreaded 
slew-o'-assertions that accompanies this situation in a debug build. 
Furthermore, it *won't* affect the "leak/bloat" numbers on Tinderbox, because 
no keybinding documents are ever loaded in the automated run.

Hyatt is eventually (B2) going to replace this code, so I don't feel bad about 
introducing some leak in the short term.

Comment 10

[Chris Waterson]

Attachment: force the keybinding table to leak

Comment 11

[leger]

Putting on PDT+ radar for beta1.  MUST FIX.

Comment 12

[David Hyatt]

I've reviewed it.  waterson, check it in baby.  jar gave approval.

Comment 13

[David Hyatt]

waterson has the fix....

Comment 14

[Jim Roskind]

Chris: Can you update the status whiteboard with a landing date... get chofmann
to allow the lading (page-chofmann) and be done with this bug :-).
Thanks,
Jim

Comment 15

[Chris Waterson]

fix checked in, r=hyatt, a=jar

Comment 16

[Christine Hoffman]

[e-mail]: I do not have sufficient information to verify this bug. 
Since this is marked PDT+ it needs to get verified ASAP. Can you take a look at 
it and if you agree it's fixed, mark it verified? Thanks.

Comment 17

[Chris Waterson]

chrisd: to verify

1. launch mozilla with the profile manger
2. create a new profile.
3. select the new profile
4. run the app

If you can page up/down in the browser content area using the keyboard, it's 
fixed.

Comment 18

[Chris Waterson]

hyatt: this is a friendly reminder to fix the leak we hacked into 
nsXULKeyListener.cpp. At one point, you'd said "we're gonna rewrite all this." 
Here's your chance. 

Comment 19

[Chris Waterson]

to hyatt, love waterson.

Comment 20

[David Hyatt]

*** This bug has been marked as a duplicate of 24645 ***

Comment 21

[Yixiong Zou]

Waterson: do you know when the rewrite of this keylistener class will be done? 
any plans on that? 

Comment 22

[Christine Hoffman]

Adding keyword 'verifyme' to bug.

Comment 23

[Scott Brodmerkle]

Verified dupe. The conversion to XBL should make this moot.