292833 - crash if i press any function key after i removed a parent of an xbl binding, where the binding has a keyset.

Status: VERIFIED FIXED

(Core :: XBL, defect)

Description

[Lukas Ramach]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; de-DE; rv:1.7.7) Gecko/20050414 Firefox/1.0.3
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b2) Gecko/20050503

I have an xbl binding which contains a keyset to enable binding specific
acceskey handling inside the content. for this purpose i added a keyset with a
key. this works pretty well. But after i remove the DOM Element where the
binding is attached to with the removeChild DOM operation, pressing any
acceskey(i always used F9) will lead to a crash. If i walk the stack it seems
for me that the window event system trys to evalute the keyset of the binding
wihch was removed previously.

Reproducible: Always

Steps to Reproduce:
1.Open the file accesskey_sample_dialog.xul from my attachment via the open file
dialog.
2.press the remove&crash button
3.press F9
4.view the crash

Actual Results:  
crash with null pointer exception

Expected Results:  
the keyset of the binding should also be removed for the window event system

final crash is in the xpcom_core module

Comment 1

[Lukas Ramach]

Attachment: testcase to reproduce. extract the archive somewher and open accesskey_sample_dialog.xul with the open file menu.

Comment 2

[[e-mail]]

nsXBLPrototypeHandler::GetHandlerElement crashes on NS_IF_ADDREF(result);

For some reason if you use a real element then there's no crash although
nsXBLWindowHandler::WalkHandlersInternal notices that the element was removed.

Poking into it deeper, when a keyset is added the xul document asks xbl to add
an event listener which never gets removed (until document teardown).

Comment 3

[Ben Fowler]

Attachment: Stacktrace

I have a crash, but I don't recall pressing a key. I think that I just pressed
the "Remove ..." button twice

Comment 4

[Boris Zbarsky [:bzbarsky]]

Neil's right on the money.  Not only are we adding an event listener to the
document that we never remove, but the event listener holds a "weak ref" (raw
nsIContent*) to an element that we _do_ remove from the DOM and that then gets
destroyed.  So the next time we need to check whether the <key> matches a key
event, we crash.

Holding a strong ref in the listener is probably undesirable, unless we release
that ref when the node is removed from the DOM.  But removing the listener when
the <key> is removed from the DOM is probably a very good idea...  Thoughts on
where we could stash a pointer to the event listener to do that?

Comment 5

[Martijn Wargers (dead)]

Attachment: Testcase

This testcase is directly available for crashing.

Comment 6

[[e-mail]]

Ah, the reason xbl is involved is that it makes it easier to GC the <key>.
You can, however, reproduce the crash without involving xbl:
1. Open browser
2. Open DOMI
3. Use DOMI to delete the F9 key (say) from browser
4. Close and open DOMI twice to force a GC
5. Hit F9 in the browser

Comment 7

[[e-mail]]

See also bug 101116 comment 2.

Comment 8

[Martijn Wargers (dead)]

Attachment: testcase2

With a testcase that removes a <keyset> and then pressing the spacebar or enter key, I get the same crash.

Comment 9

[Boris Zbarsky [:bzbarsky]]

We really need to fix this.  See comment 4 paragraph 1.

Comment 10

[Olli Pettay [:smaug][[e-mail]]]

Could we use boxObject as nsXBLWindowHandler::mElement
and weakReference to nsIDocument as mReceiver (not sure about the
latter one)

Comment 11

[Mike Schroepfer]

Bz are you volunteering to work on the fix? :-)

Comment 12

[Boris Zbarsky [:bzbarsky]]

I doubt I'll have a chance to work on this any time soon..  for one thing, I don't know enough about how <key>s are supposed to work.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

I can take a look at this.
Though, I don't know too much about <key>s either.

Comment 14

[Olli Pettay [:smaug][[e-mail]]]

Attachment: possible patch (for 1.8)

This is ugly - using boxobjects for something they really weren't made
for. But because boxobjects work as weak references to elements, they
work quite nicely here :)
For trunk nsIMutationObserver could be used to check when an node is deleted.

Bz, any comments on this?

Comment 15

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #14)
> Created an attachment (id=244601) [edit]
> possible patch (for 1.8)
> 
The patch should work on trunk too, but what I mean is that there is 
a better way to fix this bug there. Anyway, the patch could be
tested on trunk.

Comment 16

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 244601 [details] [diff] [review]
possible patch (for 1.8)

>Index: content/xbl/src/nsXBLWindowHandler.cpp
>+nsXBLWindowHandler::GetElement()
>+  nsIDOMElement* el = element;
>+  NS_IF_ADDREF(el);
>+  return el;

How about:

  nsIDOMElement* el = nsnull;
  element.swap(el);
  return el;

?

Other than that, looks ok if you add some documentation to mBoxObjectForElement explaining why we do things in this roundabout way and docs to GetElement() in the header explaining what it does.

Let's get this landed and baked as-is on trunk and do a followup (separate bug) for trunk improvements.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for trunk and 1.8

I'll check in this to trunk

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

Attachment: for 1.8.0

Comment 19

[Daniel Veditz [:dveditz]]

Comment on attachment 244614 [details] [diff] [review]
for trunk and 1.8

approved for 1.8/1.8.0 branches, a=dveditz for drivers

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

fixed1.8.1, fixed1.8.0.9

Comment 21

[Carsten Book [:Tomcat]]

verified fixed for 1.8.0.9
with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.0.9pre) Gecko/20061115 Firefox/1.5.0.9pre - No crash at the Testcases

Comment 22

[Martijn Wargers (dead)]

Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061120 Minefield/3.0a1

Comment 23

[Jay Patel [:jay]]

v.fixed on 1.8.1 branch with 
Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.1.1pre) Gecko/20061128 BonEcho/2.0.0.1pre

No crash with testcase (was crashing FF2.0)