Summary: | chrome.dll!WebCore::HTMLAreaElement::setFocus ReadAV@NULL (2f8e4496f8ea69da13971f5fddd09753) | ||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Berend-Jan Wever <skylined> | ||||||||||||
Component: | DOM | Assignee: | Berend-Jan Wever <skylined> | ||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||
Severity: | Normal | CC: | abarth, commit-queue, eric, webkit.review.bot | ||||||||||||
Priority: | P1 | ||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||
Hardware: | PC | ||||||||||||||
OS: | Windows Vista | ||||||||||||||
Attachments: |
|
Description
Berend-Jan Wever
2011-02-21 06:06:22 PST
Sounds like a trivial fix. Created attachment 83182 [details]
Layout test
I'm still trying to figure out hwo to submit a patch properly, but here's my suggestion: Index: HTMLAreaElement.cpp =================================================================== --- HTMLAreaElement.cpp (revision 79111) +++ HTMLAreaElement.cpp (working copy) @@ -231,7 +231,9 @@ // If the AREA element was a link, it should support focus. // The inherited method is not used because it assumes that a render object must exist // for the element to support focus. AREA elements do not have render objects. - return isLink(); + // skylined@chromium.org, Chromium issue 73650, WebKit issue 54877: + // An element that is not in the DOM tree cannot be focussed. + return isLink() && parentNode(); } String HTMLAreaElement::target() const I'm working on a patch Created attachment 84223 [details]
Patch
Comment on attachment 84223 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=84223&action=review Otherwise seems fine to me. > LayoutTests/ChangeLog:16 > +2011-03-01 BJ Wever <skylined@chromium.org> > + Double ChangeLog. > LayoutTests/fast/dom/HTMLAreaElement/area-islink-focus-null-ptr-crash.html:31 > + <body onload="go()"> This doens't need to be in on onload, and doesn't need to be in the head. You don't even need the rest of the document. I woul djust have a <script> tag with your 3 lines of JS. Something like this: <div id="log">FAIL</div> <script> var area = document.createElement("area"); area.href = 0; area.focus(); log.innerHTML = "PASS, did not crash"; </script> And that should be your whole test. :) I guess in my "whole test" example, I forgot the dumpAsText(), but i think you get the picture. :) Created attachment 84380 [details]
Patch attempt #2
Comment on attachment 84380 [details] Patch attempt #2 View in context: https://bugs.webkit.org/attachment.cgi?id=84380&action=review Thanks for tackling this. > Source/WebCore/html/HTMLAreaElement.cpp:234 > - // If the AREA element was a link, it should support focus. > + // If the AREA element was a link and has a parent, it should support focus. > // The inherited method is not used because it assumes that a render object must exist > // for the element to support focus. AREA elements do not have render objects. > - return isLink(); > + return isLink() && parentNode(); This check seems incorrect, too specific, and in the wrong place. For example, an area element might have a parent, but its parent might in turn not be in the document’s DOM tree. I think the correct check would be not in the supportsFocus function, but rather at the call sites. And the check would probably be inDocument, or possibly a walk up the parent node chain until we reach the document that's trying to do the focus or don’t. I’m pretty sure that the function that needs the fix is Element::focus. It doesn’t have the inDocument check that Node::isFocusable has, and the fix could be as simple as adding that. Thanks, that makes sense Darin. In addition, there seems to be another problem: HTMLAreaElement::imageElement assumes the area element has a parent, which is not true in this case, and may not be true in others. I think we should address this as a defense in depth as well: HTMLImageElement* HTMLAreaElement::imageElement() const { Node* mapElement = parentNode(); if (!mapElement->hasTagName(mapTag)) return 0; return static_cast<HTMLMapElement*>(mapElement)->imageElement(); } Patch: if (!mapElement || !mapElement->hasTagName(mapTag)) Created attachment 84765 [details]
Patch
The changes to HTMLAreaElement::imageElement are not strictly needed, so let me know if it makes sense to add them as a defense in depth or not.
Comment on attachment 84765 [details] Patch Clearing flags on attachment: 84765 Committed r80779: <http://trac.webkit.org/changeset/80779> All reviewed patches have been landed. Closing bug. http://trac.webkit.org/changeset/80779 might have broken GTK Linux 32-bit Release |