Summary: | ASSERT(renderer()) under HTMLTextAreaElement::updateValue() | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||||||||
Component: | HTML Editing | Assignee: | Chris Dumez <cdumez> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | ap, commit-queue, ews-watchlist, ggaren, rniwa, webkit-bug-importer, wenson_hsieh | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||
OS: | Unspecified | ||||||||||||||||
Attachments: |
|
Description
Chris Dumez
2018-11-07 12:06:39 PST
Created attachment 354126 [details]
WIP Patch
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 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 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 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 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
Created attachment 354154 [details]
Patch
Created attachment 354161 [details]
Patch
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. (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 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. I think the correct fix here is to just delete the assertion and add an early return. Created attachment 354176 [details]
Patch
(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 on attachment 354176 [details]
Patch
r=me
Comment on attachment 354176 [details] Patch Clearing flags on attachment: 354176 Committed r237975: <https://trac.webkit.org/changeset/237975> All reviewed patches have been landed. Closing bug. |