RESOLVED FIXED 204332
Nullptr crash in Node::setTextContent via Document::setTitle if title element is removed before setTextContent call. 
https://bugs.webkit.org/show_bug.cgi?id=204332
Summary Nullptr crash in Node::setTextContent via Document::setTitle if title element...
Sunny He
Reported 2019-11-18 17:17:59 PST
Created attachment 383812 [details] Repro input Document::setTitle may append a new title element if none currently exists. If this triggers an event handler which deletes this new title element, the call to setTextContent will cause a crash.
Attachments
Repro input (440 bytes, text/html)
2019-11-18 17:17 PST, Sunny He 		no flags
Details
Patch (4.27 KB, patch)
2019-11-18 17:30 PST, Sunny He 		no flags
DetailsFormatted DiffDiff
Patch (5.91 KB, patch)
2019-11-19 14:15 PST, Sunny He 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2019-11-18 17:18:09 PST
<rdar://problem/57305579>
Sunny He
Comment 2 2019-11-18 17:19:43 PST
<rdar://56626071>
Ryosuke Niwa
Comment 3 2019-11-18 17:24:25 PST
This is not a security bug.
Sunny He
Comment 4 2019-11-18 17:30:23 PST
Created attachment 383816 [details] Patch
Ryosuke Niwa
Comment 5 2019-11-18 17:30:55 PST
WebKit's behavior of document.title's setter matches the current spec: https://html.spec.whatwg.org/multipage/dom.html#document.title which says: > Let element be the result of creating an element given the document element's node document, title, and the HTML namespace. > > Append element to the head element. Because the spec doesn't have mutation events they don't have to deal with the situation in which appending the element to the head element results in its removal :(
Ryosuke Niwa
Comment 6 2019-11-18 17:32:58 PST
Comment on attachment 383816 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383816&action=review > Source/WebCore/dom/Document.cpp:1601 > + // Another null check since insertBefore may have triggered event that deletes m_titleElement This is a bit verbose. I think we can just say: "insertBefore above may have ran scripts which removed m_titleElement" > Source/WebCore/dom/Document.cpp:1613 > + // Another null check since appendChild may have triggered event that deletes m_titleElement > + if (m_titleElement) Ditto. > LayoutTests/fast/dom/Document/title-property-set-with-dom-event.html:1 > +<html> Can we add another test case with a SVG document since you're also making the code change to SVG code path?
Ryosuke Niwa
Comment 7 2019-11-18 17:33:40 PST
I guess one thing we should check is what other browsers do. Do they set the value of title element despite of the fact they're no longer in the document? What do they do if they insert the removed element into a new document (e.g. into an iframe's contentDocument).
Sunny He
Comment 8 2019-11-18 17:53:26 PST
Firefox does update the text value, but Chrome does not. So this patch will align with Chrome's behavior.
Ryosuke Niwa
Comment 9 2019-11-18 21:17:35 PST
(In reply to Sunny He from comment #8) > Firefox does update the text value, but Chrome does not. So this patch will > align with Chrome's behavior. I don't think Firefox updates the text value per se. I think the behavior we observed is that Firefox would set the text content value of HTMLTitleElement before DOMNodeInserted event fires. In neither browsers, document's title was updated because the title element is ultimately removed.
Sunny He
Comment 10 2019-11-19 14:15:26 PST
Created attachment 383904 [details] Patch
Ryosuke Niwa
Comment 11 2019-11-19 16:52:45 PST
Comment on attachment 383904 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383904&action=review > LayoutTests/fast/dom/Document/title-property-set-with-dom-event-svg.html:5 > + if (window.testRunner) { WebKit style is to not use curly braces around a single line statement but it’s okay to keep it like this for tests.
WebKit Commit Bot
Comment 12 2019-11-19 17:27:42 PST
Comment on attachment 383904 [details] Patch Clearing flags on attachment: 383904 Committed r252667: <https://trac.webkit.org/changeset/252667>
WebKit Commit Bot
Comment 13 2019-11-19 17:27:44 PST
All reviewed patches have been landed. Closing bug.
Simon Fraser (smfr)
Comment 14 2019-11-19 21:07:44 PST
Comment on attachment 383904 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383904&action=review > Source/WebCore/dom/Document.cpp:1601 > + // insertBefore above may have ran scripts which removed m_titleElement ...may have run scripts... Also period at the end (we like comments to be full sentences). > Source/WebCore/dom/Document.cpp:1612 > + // appendChild above may have ran scripts which removed m_titleElement Same.
Ryosuke Niwa
Comment 15 2019-12-04 00:07:12 PST
Committed r253096: <https://trac.webkit.org/changeset/253096>
Note You need to log in before you can comment on or make changes to this bug.