Summary: | SVG TRef/Use NULL ptr | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Berend-Jan Wever <skylined> | ||||
Component: | SVG | Assignee: | Nikolas Zimmermann <zimmermann> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | abarth, cc-bugs, cgarcia, eric, fmalita, gustavo, jamesr, japhet, levin+threading, macpherson, menard, ojan, rakuco, rwlbuis, webkit.review.bot, zimmermann, zoltan | ||||
Priority: | P1 | ||||||
Version: | 528+ (Nightly build) | ||||||
Hardware: | PC | ||||||
OS: | Windows Vista | ||||||
Attachments: |
|
Description
Berend-Jan Wever
2012-02-15 05:23:42 PST
(In reply to comment #0) > <<<snip>>> > m_eventListener = SubtreeModificationEventListener::create(this, id); > ASSERT(target->parentNode()); > target->parentNode()->addEventListener(eventNames().DOMSubtreeModifiedEvent, m_eventListener.get(), false); > (*it) points to an SVGUseElement which doesn't have a shadowTreeElement, causing the NULL ptr. Oh dear, apparently <tref> doesn't even check if the target is valid, just attaching its event listener. This is a bad idea. CC'ing Rob, who wrote the current <tref> implementation. Reminds me of a similar <use> bug which is in the process of being fixed: white-list allowed targets, instead of black-listing disallowed ones. FWIW my patch for http://www.0 also fixes this particular crash (as the event listeners are moved onto the target instead of its parent). Sounds like we still need to add target validation though. (In reply to comment #2) > FWIW my patch for http://www.0 also fixes this particular crash (as the event listeners are moved onto the target instead of its parent). Sounds like we still need to add target validation though. Just looked at this again, Rob did it just correct according to SVG 1.1 2nd edition: "xlink:href of <tref>: An IRI reference to an element whose character data content shall be used as character data for this ‘tref’ element." There's no white or black-listing desired, any element should work. So probably this is just a dup of your bug. Created attachment 129693 [details]
Patch
Thanks for the patch. If this patch contains new public API please make sure it follows the guidelines for new WebKit2 GTK+ API. See http://trac.webkit.org/wiki/WebKitGTK/AddingNewWebKit2API Attachment 129693 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'ChangeLog', u'LayoutTests/ChangeLog', u'La..." exit_code: 1
WARNING: File exempt from style guide. Skipping: "Source/WebKit2/UIProcess/API/gtk/WebKitDefines.h"
Source/WebKit2/WebProcess/gtk/WebProcessMainGtk.cpp:71: Use 0 instead of NULL. [readability/null] [5]
Total errors found: 1 in 176 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #6) > If any of these errors are false positives, please file a bug against check-webkit-style. Not mine. Bug was already filed. Comment on attachment 129693 [details]
Patch
Adding test doesn't hurt. Could you update the ChangeLog to put diff to the head?
Committed r117229: <http://trac.webkit.org/changeset/117229> |