Bug 191391 – ASSERT(renderer()) under HTMLTextAreaElement::updateValue()

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Chris Dumez]

ASSERT(renderer()) under HTMLTextAreaElement::updateValue():
ASSERTION FAILED: renderer()
./html/HTMLTextAreaElement.cpp(349) : void WebCore::HTMLTextAreaElement::updateValue() const
1   0x5c13c4629 WTFCrash
2   0x5b000e0fb WTFCrashWithInfo(int, char const*, char const*, int)
3   0x5b2601b19 WebCore::HTMLTextAreaElement::updateValue() const
4   0x5b25ff247 WebCore::HTMLTextAreaElement::value() const
5   0x5b26021a8 WebCore::HTMLTextAreaElement::tooShort() const
6   0x5b2468495 WebCore::FormAssociatedElement::isValid() const
7   0x5b24ec622 WebCore::HTMLFormControlElement::updateValidity()
8   0x5b260160e WebCore::HTMLTextAreaElement::subtreeHasChanged()
9   0x5b26034d9 WebCore::HTMLTextFormControlElement::didEditInnerTextValue()
10  0x5b2379730 WebCore::notifyTextFromControls(WebCore::Element*, WebCore::Element*)
11  0x5b237930f WebCore::Editor::appliedEditing(WebCore::CompositeEditCommand&)
12  0x5b23424c3 WebCore::CompositeEditCommand::didApplyCommand()
13  0x5b232e13c WebCore::CompositeEditCommand::apply()
14  0x5b2398b89 WebCore::executeInsertFragment(WebCore::Frame&, WTF::Ref<WebCore::DocumentFragment, WTF::DumbPtrTraits<WebCore::DocumentFragment> >&&)
15  0x5b23945d3 WebCore::executeInsertHTML(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&)
16  0x5b237e1b3 WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const
17  0x5b21155f9 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&)
18  0x5b08d4a58 WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::ExecState*, WebCore::JSDocument*, JSC::ThrowScope&)
19  0x5b08b8750 long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunctionExecCommandBody(JSC::ExecState*, WebCore::JSDocument*, JSC::ThrowScope&)), (WebCore::CastedThisErrorBehavior)0>(JSC::ExecState&, char const*)
20  0x5b08b843c WebCore::jsDocumentPrototypeFunctionExecCommand(JSC::ExecState*)
21  0x26026f401177
22  0x5c187ee9c llint_entry
23  0x5c186bb92 vmEntryToJavaScript
24  0x5c24ad16e JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
25  0x5c24ad809 JSC::Interpreter::executeCall(JSC::ExecState*, JSC::JSObject*, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
26  0x5c277566c JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&)
27  0x5c277575a JSC::call(JSC::ExecState*, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
28  0x5c2775a4e JSC::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
29  0x5b1bd59ab WebCore::JSExecState::profiledCall(JSC::ExecState*, JSC::ProfilingReason, JSC::JSValue, JSC::CallType, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&)
30  0x5b1c19104 WebCore::JSEventListener::handleEvent(WebCore::ScriptExecutionContext&, WebCore::Event&)
31  0x5b21e832c WebCore::EventTarget::innerInvokeEventListeners(WebCore::Event&, WTF::Vector<WTF::RefPtr<WebCore::RegisteredEventListener, WTF::DumbPtrTraits<WebCore::RegisteredEventListener> >, 1ul, WTF::CrashOnOverflow, 16ul>, WebCore::EventTarget::EventInvokePhase)

[Chris Dumez]

<rdar://problem/34219633>

[Chris Dumez]

Created attachment 354126 [details]
WIP Patch

[EWS Watchlist]

Attachment 354126 [details] did not pass style-queue:


ERROR: Source/WebCore/editing/ReplaceSelectionCommand.cpp:1038:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 72 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[EWS Watchlist]

Comment on attachment 354126 [details]
WIP Patch

Attachment 354126 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/9897440

New failing tests:
editing/pasteboard/paste-line-endings-002.html
editing/pasteboard/paste-line-endings-004.html
editing/pasteboard/paste-4038267-fix.html
editing/pasteboard/paste-line-endings-003.html
editing/pasteboard/paste-line-endings-005.html

[EWS Watchlist]

Created attachment 354134 [details]
Archive of layout-test-results from ews101 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-sierra  Platform: Mac OS X 10.12.6

[EWS Watchlist]

Comment on attachment 354126 [details]
WIP Patch

Attachment 354126 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/9897743

New failing tests:
editing/pasteboard/paste-line-endings-002.html
editing/pasteboard/paste-line-endings-004.html
editing/pasteboard/paste-4038267-fix.html
editing/pasteboard/paste-line-endings-003.html
editing/pasteboard/paste-line-endings-005.html

[EWS Watchlist]

Created attachment 354141 [details]
Archive of layout-test-results from ews104 for mac-sierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-sierra-wk2  Platform: Mac OS X 10.12.6

[Chris Dumez]

Created attachment 354154 [details]
Patch

[Chris Dumez]

Created attachment 354161 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 354161 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354161&action=review

> Source/WebCore/ChangeLog:12
> +        (as is expected) but document.execCommand() would still return true (which did not match Firefox

I don't think anyone cares about the return value of execCommand.
The behavior isn't really interoperable and we have no plan to make it interoperable.
Let's just fix the assertion failure instead.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #10)
> Comment on attachment 354161 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=354161&action=review
> 
> > Source/WebCore/ChangeLog:12
> > +        (as is expected) but document.execCommand() would still return true (which did not match Firefox
> 
> I don't think anyone cares about the return value of execCommand.
> The behavior isn't really interoperable and we have no plan to make it
> interoperable.

Ok. I can keep returning true in execCommand().

> Let's just fix the assertion failure instead.

I do not understand this comment though.

[Ryosuke Niwa]

Comment on attachment 354161 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=354161&action=review

> Source/WebCore/ChangeLog:14
> +        HTMLTextAreaElement::subtreeHasChanged() even though it really did not change. This would call

This fix doesn't seem complete. It's totally possible for some scripts to hide the text area after modifying the sub tree during an editing command.
In that case, this asssetion would fire again.
Fundamentally, the failing assertion seems flawed. I don't think we can guarantee that the function won't be called when it doesn't have a renderer
short of checking whether the element has a tenderer or not before calling the function.

[Ryosuke Niwa]

I think the correct fix here is to just delete the assertion and add an early return.

[Chris Dumez]

Created attachment 354176 [details]
Patch

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #13)
> I think the correct fix here is to just delete the assertion and add an
> early return.

All other call sites of subtreeHasChanged() have an early return if !renderer(), except for HTMLTextFormControlElement::didEditInnerTextValue(). So I am now fixing HTMLTextFormControlElement::didEditInnerTextValue().

Note that this was my initial plan until I noticed that other browsers were returning false for execCommand() and we were returning true. If we do not care, then I'd much rather go with the simpler fix indeed.

[Geoffrey Garen]

Comment on attachment 354176 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 354176 [details]
Patch

Clearing flags on attachment: 354176

Committed r237975: <https://trac.webkit.org/changeset/237975>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.