Closed Bug 292833 Opened 15 years ago Closed 13 years ago

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

Categories

(Core :: XBL, defect, critical)

defect
Not set
critical

Tracking

()

VERIFIED FIXED

People

(Reporter: ramach, Assigned: smaug)

References

()

Details

(4 keywords)

Attachments

(7 files)

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
Attachment #182567 - Attachment description: testcase to reproduce. open accesskey_sample_dialog.xul → testcase to reproduce. extract the archive somewher and open accesskey_sample_dialog.xul with the open file menu.
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).
Attached file Stacktrace
I have a crash, but I don't recall pressing a key. I think that I just pressed
the "Remove ..." button twice
Attachment #182567 - Attachment mime type: application/octet-stream → application/zip
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?
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Attached file Testcase
This testcase is directly available for crashing.
Keywords: testcase
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
Attached file testcase2
With a testcase that removes a <keyset> and then pressing the spacebar or enter key, I get the same crash.
We really need to fix this.  See comment 4 paragraph 1.
Flags: blocking1.9?
Flags: blocking1.8.1.1?
Flags: blocking1.8.0.9?
Could we use boxObject as nsXBLWindowHandler::mElement
and weakReference to nsIDocument as mReceiver (not sure about the
latter one)
Bz are you volunteering to work on the fix? :-)
Flags: blocking1.8.1.1?
Flags: blocking1.8.1.1+
Flags: blocking1.8.0.9?
Flags: blocking1.8.0.9+
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.
I can take a look at this.
Though, I don't know too much about <key>s either.
Status: NEW → ASSIGNED
Assignee: general → Olli.Pettay
Status: ASSIGNED → NEW
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?
Attachment #244601 - Flags: review?(bzbarsky)
(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 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.
Attachment #244601 - Flags: superreview+
Attachment #244601 - Flags: review?(bzbarsky)
Attachment #244601 - Flags: review+
I'll check in this to trunk
Blocks: 359494
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Attachment #244601 - Flags: approval1.8.1.1?
Attachment #244601 - Flags: approval1.8.1.1?
Attachment #244614 - Attachment description: for trunk → for trunk and 1.8
Attachment #244614 - Flags: approval1.8.1.1?
Attached patch for 1.8.0Splinter Review
Attachment #245411 - Flags: approval1.8.0.9?
Comment on attachment 244614 [details] [diff] [review]
for trunk and 1.8

approved for 1.8/1.8.0 branches, a=dveditz for drivers
Attachment #244614 - Flags: approval1.8.1.1? → approval1.8.1.1+
Attachment #245411 - Flags: approval1.8.0.9? → approval1.8.0.9+
fixed1.8.1, fixed1.8.0.9
Flags: blocking1.9?
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
Verified fixed, using:
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20061120 Minefield/3.0a1
Status: RESOLVED → VERIFIED
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)
You need to log in before you can comment on or make changes to this bug.