Bug 191391 - ASSERT(renderer()) under HTMLTextAreaElement::updateValue()
Summary: ASSERT(renderer()) under HTMLTextAreaElement::updateValue()
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-11-07 12:06 PST by Chris Dumez
Modified: 2018-11-07 19:11 PST (History)
7 users (show)

See Also:


Attachments
WIP Patch (71.89 KB, patch)
2018-11-07 12:08 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-sierra (3.29 MB, application/zip)
2018-11-07 13:15 PST, EWS Watchlist
no flags Details
Archive of layout-test-results from ews104 for mac-sierra-wk2 (4.00 MB, application/zip)
2018-11-07 13:40 PST, EWS Watchlist
no flags Details
Patch (91.08 KB, patch)
2018-11-07 14:34 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (92.56 KB, patch)
2018-11-07 14:56 PST, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (4.41 KB, patch)
2018-11-07 16:21 PST, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2018-11-07 12:06:39 PST
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)
Comment 1 Chris Dumez 2018-11-07 12:06:56 PST
<rdar://problem/34219633>
Comment 2 Chris Dumez 2018-11-07 12:08:25 PST
Created attachment 354126 [details]
WIP Patch
Comment 3 EWS Watchlist 2018-11-07 12:10:06 PST
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.
Comment 4 EWS Watchlist 2018-11-07 13:15:27 PST
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
Comment 5 EWS Watchlist 2018-11-07 13:15:29 PST
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
Comment 6 EWS Watchlist 2018-11-07 13:40:25 PST
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
Comment 7 EWS Watchlist 2018-11-07 13:40:27 PST
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
Comment 8 Chris Dumez 2018-11-07 14:34:03 PST
Created attachment 354154 [details]
Patch
Comment 9 Chris Dumez 2018-11-07 14:56:28 PST
Created attachment 354161 [details]
Patch
Comment 10 Ryosuke Niwa 2018-11-07 16:07:07 PST
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.
Comment 11 Chris Dumez 2018-11-07 16:08:24 PST
(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.
Comment 12 Ryosuke Niwa 2018-11-07 16:12:43 PST
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.
Comment 13 Ryosuke Niwa 2018-11-07 16:13:34 PST
I think the correct fix here is to just delete the assertion and add an early return.
Comment 14 Chris Dumez 2018-11-07 16:21:43 PST
Created attachment 354176 [details]
Patch
Comment 15 Chris Dumez 2018-11-07 16:26:07 PST
(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.
Comment 16 Geoffrey Garen 2018-11-07 16:59:19 PST
Comment on attachment 354176 [details]
Patch

r=me
Comment 17 WebKit Commit Bot 2018-11-07 19:11:40 PST
Comment on attachment 354176 [details]
Patch

Clearing flags on attachment: 354176

Committed r237975: <https://trac.webkit.org/changeset/237975>
Comment 18 WebKit Commit Bot 2018-11-07 19:11:42 PST
All reviewed patches have been landed.  Closing bug.