52897 - Non "key" node in keyset causes Mozilla to hang.

Status: VERIFIED INVALID

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Description

[Mark Hammond [:markh] [:mhammond]]

A very simple XUL with a simple keyset definition causes Mozilla to hang.

Consider the attached .xul file.  Running "mozilla -
chrome "file://.../test2.xul" creates a small, blank window, as you expect.  
This works perfectly until you press any key, in which case Mozilla hangs, with 
CPU at 100%.

The problem is that the keyset is invalid.  It is missing an open "<" for the 
key element.  Thus, the Keyset ends up with a single generic "TextNode", rather 
than a Key node.

Such a simple error should not cause Mozilla to hang.

The problem is in nsXULKeyListener.cpp, in the loop starting at line 1410:
http://lxr.mozilla.org/mozilla/source/rdf/content/src/nsXULKeyListener.cpp#1410

1410   nsCOMPtr<nsIDOMNode> keyNode;
1411   aKeysetElement->GetFirstChild(getter_AddRefs(keyNode));
1412   while (keyNode) {
1413     nsCOMPtr<nsIDOMElement> keyElement(do_QueryInterface(keyNode));
1414     if (!keyElement)
1415       continue;

If the QueryInterface at 1413 fails, we attempt to ignore the item, and 
continue around the loop.  However, we have not moved to the next item in the 
iterator.  Thus, Mozilla is running hard from 1412-1415, over the exact same 
(failing) DOM item.

I have attached a patch that corrects the behaviour.

Comment 1

[Mark Hammond [:markh] [:mhammond]]

Attachment: xul file that demonstrates the hang - note the missing "<" on the "key" definition.

Comment 2

[Mark Hammond [:markh] [:mhammond]]

Attachment: Proposed patch that prevents the hang by moving to the next node on failure.

Comment 3

[Loco]

Updating QA Contact.

Comment 4

[Gervase Markham [:gerv]]

[e-mail] - thanks for this patch :-) However, the file you 
reference is no longer in the source tree. Could you possibly work out what 
has happened, and whether your patch is still relevant?

Thanks :-)

Gerv

Comment 5

[Mark Hammond [:markh] [:mhammond]]

This does appear to have vanished.  I can no longer repro, and found where the 
new equivilent code is (unfortunately, this is a second attempt at this note 
and I since lost it - not worth finding again) - anyway - the new code is 
implemented quite differently, and a visual check indicates it will not suffer 
the same problem.

I'm leaving all the status options alone, as I'm not sure of protocol there (or 
indeed if it will even work :-) - but feel free to close this.

Comment 6

[Gervase Markham [:gerv]]

Bug has been INVALIDated due to other changes in code :-) Closing...

Gerv

Comment 7

[Loco]

Sounds good to me.  VERIFIED.