Bug 56642

Summary: Null pointer crash in StyleChange::init inside an empty document
Product: WebKit Reporter: Berend-Jan Wever <skylined>
Component: HTML EditingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, darin, hyatt, ojan, rniwa, simon.fraser
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: Windows Vista   
Bug Depends on: 56771    
Bug Blocks:    
Attachments:
Description Flags
fixes the crash
none
work in progress; hits an assertion none

Berend-Jan Wever
Reported 2011-03-18 06:01:25 PDT
Chromium: http://code.google.com/p/chromium/issues/detail?id=76672 Repro: <body onload="go()"></body> <script> function go() { var oSelection=window.getSelection(); document.designMode="on"; document.open(); oSelection.addRange(document.createRange()); document.execCommand("justifyleft"); } </script> id: chrome.dll!WebCore::CSSStyleDeclaration::diff ReadAV@NULL (1933558aaf23e4d9cc2ec3bc22fad247) description: Attempt to read from unallocated NULL pointer in chrome.dll!WebCore::CSSStyleDeclaration::diff stack: chrome.dll!WebCore::CSSStyleDeclaration::diff chrome.dll!WebCore::getPropertiesNotIn chrome.dll!WebCore::StyleChange::init chrome.dll!WebCore::ApplyStyleCommand::applyBlockStyle chrome.dll!WebCore::ApplyStyleCommand::doApply chrome.dll!WebCore::EditCommand::apply chrome.dll!WebCore::applyCommand chrome.dll!WebCore::Editor::applyParagraphStyle chrome.dll!WebCore::executeApplyParagraphStyle chrome.dll!WebCore::executeJustifyLeft chrome.dll!WebCore::Editor::Command::execute chrome.dll!WebCore::Document::execCommand chrome.dll!WebCore::DocumentInternal::execCommandCallback chrome.dll!v8::internal::HandleApiCallHelper<...> chrome.dll!v8::internal::Builtin_HandleApiCall chrome.dll!v8::internal::Invoke chrome.dll!v8::internal::Execution::Call ...
Attachments
fixes the crash (3.28 KB, patch)
2011-03-18 15:48 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
work in progress; hits an assertion (4.97 KB, patch)
2011-03-18 16:36 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Berend-Jan Wever
Comment 1 2011-03-18 07:12:56 PDT
The problem is that the selection contains only the HTMLDocument when this code is executed: \source\webcore\editing\applystylecommand.cpp: void StyleChange::init(EditingStyle* style, const Position& position) { Document* document = position.anchorNode() ? position.anchorNode()->document() : 0; if (!style || !style->style() || !document || !document->frame()) return; RefPtr<CSSComputedStyleDeclaration> computedStyle = position.computedStyle(); <snip> This calls the following code: \source\webcore\dom\Position.cpp: PassRefPtr<CSSComputedStyleDeclaration> Position::computedStyle() const { Element* elem = element(); if (!elem) return 0; return WebCore::computedStyle(elem); } Where "element()" is the following code in the same file: Element* Position::element() const { Node* n = anchorNode(); while (n && !n->isElementNode()) n = n->parentNode(); return static_cast<Element*>(n); } a HTMLDocument is not an ElementNode, so this returns 0, and "Position::computedStyle" returns 0 as well. "StyleChange::init" does not expect this.
Ryosuke Niwa
Comment 2 2011-03-18 15:48:58 PDT
Created attachment 86232 [details] fixes the crash
Ryosuke Niwa
Comment 3 2011-03-18 16:01:42 PDT
Comment on attachment 86232 [details] fixes the crash Thanks for the review, Darin. But I realized that my fix doesn't allow styles to be applied properly. So I'm deprecating this patch and uploading new one that actually fixes the feature and after which JustifyRight is applied properly.
Ryosuke Niwa
Comment 4 2011-03-18 16:34:02 PDT
I'm now running into the assertion in: void Document::setCompatibilityMode(CompatibilityMode mode) { if (m_compatibilityModeLocked || mode == m_compatibilityMode) return; ASSERT(!documentElement() && !m_styleSheets->length()); bool wasInQuirksMode = inQuirksMode(); m_compatibilityMode = mode; if (inQuirksMode() != wasInQuirksMode) { // All user stylesheets have to reparse using the different mode. clearPageUserSheet(); clearPageGroupUserSheets(); } } Maybe we shouldn't even let execCommand run when there's no body element.
Ryosuke Niwa
Comment 5 2011-03-18 16:34:23 PDT
The assertion was added in http://trac.webkit.org/changeset/66247.
Ryosuke Niwa
Comment 6 2011-03-18 16:36:02 PDT
Created attachment 86241 [details] work in progress; hits an assertion
Ryosuke Niwa
Comment 7 2011-04-03 08:27:42 PDT
Fixed in http://trac.webkit.org/changeset/82791.
Note You need to log in before you can comment on or make changes to this bug.