Bug 224893 – Nullptr crash in DeleteSelectionCommand::removeNode

• Component: HTML Editing
• Status: RESOLVED FIXED

[Ryosuke Niwa]

Created attachment 426741 [details]
Test

e.g.

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.WebCore             	0x00000005693d8608 WebCore::Node::treeScope() const + 24 (Node.h:358)
1   com.apple.WebCore             	0x00000005693bfbc5 WebCore::Node::document() const + 21 (Node.h:354)
2   com.apple.WebCore             	0x000000056c2bd562 WebCore::Node::computeEditability(WebCore::Node::UserSelectAllTreatment, WebCore::Node::ShouldUpdateStyle) const + 34 (Node.cpp:783)
3   com.apple.WebCore             	0x000000056b93fde0 WebCore::Node::hasEditableStyle(WebCore::Node::UserSelectAllTreatment) const + 32 (Node.h:331)
4   com.apple.WebCore             	0x000000056c4290cf WebCore::DeleteSelectionCommand::removeNode(WebCore::Node&, WebCore::ShouldAssumeContentIsAlwaysEditable) + 207 (DeleteSelectionCommand.cpp:438)
5   com.apple.WebCore             	0x000000056c429fe5 WebCore::DeleteSelectionCommand::handleGeneralDelete() + 1861 (DeleteSelectionCommand.cpp:611)
6   com.apple.WebCore             	0x000000056c42ce5d WebCore::DeleteSelectionCommand::doApply() + 1181 (DeleteSelectionCommand.cpp:939)
7   com.apple.WebCore             	0x000000056c40b44f WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::EditCommand, WTF::RawPtrTraits<WebCore::EditCommand> >&&) + 63 (CompositeEditCommand.cpp:488)
8   com.apple.WebCore             	0x000000056c408007 WebCore::CompositeEditCommand::deleteSelection(bool, bool, bool, bool, bool) + 311 (CompositeEditCommand.cpp:857)
9   com.apple.WebCore             	0x000000056c494fbf WebCore::InsertTextCommand::doApply() + 319 (InsertTextCommand.cpp:143)
10  com.apple.WebCore             	0x000000056c40b7a5 WebCore::CompositeEditCommand::applyCommandToComposite(WTF::Ref<WebCore::CompositeEditCommand, WTF::RawPtrTraits<WebCore::CompositeEditCommand> >&&, WebCore::VisibleSelection const&) + 165 (CompositeEditCommand.cpp:503)
11  com.apple.WebCore             	0x000000056c4d7354 WebCore::TypingCommand::insertTextRunWithoutNewlines(WTF::String const&, bool) + 244 (TypingCommand.cpp:540)
12  com.apple.WebCore             	0x000000056c4fb2da WebCore::TypingCommandLineOperation::operator()(unsigned long, unsigned long, bool) const + 138 (TypingCommand.cpp:69)
13  com.apple.WebCore             	0x000000056c4d721a void WebCore::forEachLineInString<WebCore::TypingCommandLineOperation>(WTF::String const&, WebCore::TypingCommandLineOperation const&) + 154 (TextInsertionBaseCommand.h:60)
14  com.apple.WebCore             	0x000000056c4d712c WebCore::TypingCommand::insertText(WTF::String const&, bool) + 60 (TypingCommand.cpp:519)
15  com.apple.WebCore             	0x000000056c4d5c34 WebCore::TypingCommand::insertTextAndNotifyAccessibility(WTF::String const&, bool) + 164 (TypingCommand.cpp:527)
16  com.apple.WebCore             	0x000000056c4d67e2 WebCore::TypingCommand::doApply() + 354 (TypingCommand.cpp:380)
17  com.apple.WebCore             	0x000000056c3f6a2c WebCore::CompositeEditCommand::apply() + 412 (CompositeEditCommand.cpp:397)
18  com.apple.WebCore             	0x000000056c4c3763 WebCore::TextInsertionBaseCommand::applyTextInsertionCommand(WebCore::Frame*, WebCore::TextInsertionBaseCommand&, WebCore::VisibleSelection const&, WebCore::VisibleSelection const&) + 99 (TextInsertionBaseCommand.cpp:50)
19  com.apple.WebCore             	0x000000056c4d5b27 WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, WebCore::VisibleSelection const&, unsigned int, WebCore::TypingCommand::TextCompositionType) + 631 (TypingCommand.cpp:259)
20  com.apple.WebCore             	0x000000056c4d58a3 WebCore::TypingCommand::insertText(WebCore::Document&, WTF::String const&, unsigned int, WebCore::TypingCommand::TextCompositionType) + 147 (TypingCommand.cpp:229)
21  com.apple.WebCore             	0x000000056c47c924 WebCore::executeInsertText(WebCore::Frame&, WebCore::Event*, WebCore::EditorCommandSource, WTF::String const&) + 52 (EditorCommand.cpp:525)
22  com.apple.WebCore             	0x000000056c454f3b WebCore::Editor::Command::execute(WTF::String const&, WebCore::Event*) const + 235 (EditorCommand.cpp:1860)
23  com.apple.WebCore             	0x000000056c1569a5 WebCore::Document::execCommand(WTF::String const&, bool, WTF::String const&) + 85 (Document.cpp:5708)
24  com.apple.WebCore             	0x0000000569b2c454 WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*) + 1636 (JSDocument.cpp:5869)
25  com.apple.WebCore             	0x0000000569b2bdbc long long WebCore::IDLOperation<WebCore::JSDocument>::call<&(WebCore::jsDocumentPrototypeFunction_execCommandBody(JSC::JSGlobalObject*, JSC::CallFrame*, WebCore::JSDocument*)), (WebCore::CastedThisErrorBehavior)0>(JSC::JSGlobalObject&, JSC::CallFrame&, char const*) + 700 (JSDOMOperation.h:55)
26  com.apple.WebCore             	0x0000000569b14924 WebCore::jsDocumentPrototypeFunction_execCommand(JSC::JSGlobalObject*, JSC::CallFrame*) + 36 (JSDocument.cpp:5874)
27  ???                           	0x00003c836b8011d8 0 + 66535141937624
28  com.apple.JavaScriptCore      	0x000000058711f80c llint_entry + 138578 (LowLevelInterpreter.asm:1097)
29  com.apple.JavaScriptCore      	0x00000005870fd7c0 vmEntryToJavaScript + 289 (LowLevelInterpreter64.asm:316)
30  com.apple.JavaScriptCore      	0x0000000587fdd48b JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*) + 235 (JITCodeInlines.h:42)
31  com.apple.JavaScriptCore      	0x0000000587fddbec JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 1724 (Interpreter.cpp:902)
32  com.apple.JavaScriptCore      	0x000000058836278d JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) + 221 (CallData.cpp:57)
33  com.apple.JavaScriptCore      	0x000000058836286f JSC::call(JSC::JSGlobalObject*, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) + 207 (CallData.cpp:64)
34  com.apple.JavaScriptCore      	0x0000000588362b52 JSC::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) + 130 (CallData.cpp:85)
35  com.apple.WebCore             	0x000000056bb0c51e WebCore::JSExecState::profiledCall(JSC::JSGlobalObject*, JSC::ProfilingReason, JSC::JSValue, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&, WTF::NakedPtr<JSC::Exception>&) + 110 (JSExecState.h:73)
36  com.apple.WebCore             	0x000000056bbbf6b0 WebCore::ScheduledAction::executeFunctionInContext(JSC::JSGlobalObject*, JSC::JSValue, WebCore::ScriptExecutionContext&) + 992 (ScheduledAction.cpp:121)
37  com.apple.WebCore             	0x000000056bbbf0e5 WebCore::ScheduledAction::execute(WebCore::Document&) + 277 (ScheduledAction.cpp:141)
38  com.apple.WebCore             	0x000000056bbbefa3 WebCore::ScheduledAction::execute(WebCore::ScriptExecutionContext&) + 67 (ScheduledAction.cpp:86)
39  com.apple.WebCore             	0x000000056ceab5c7 WebCore::DOMTimer::fired() + 1063 (DOMTimer.cpp:337)

<rdar://76946927>

[Carlos Garcia Campos]

The crash is in DeleteSelectionCommand::removeNode() in line:

if (!node.parentNode()->hasEditableStyle()) {

because parentNode() is nullptr. This is happening because CompositeEditCommand::removeNode() is called twice for both the embed element and the input element. The second time it's called for the input element, it has already been removed from the tree, so parent is nullptr. And the reason why it's called twice for the same nodes is the weird thing here. There are two calls to InsertText, but the second one happens before the first one has finished, so the tree is in inconsistent state. I suspect it might be a bug in WTR because that doesn't happen when running in MiniBrowser. From MiniBrowser the second InsertText command is executed after the first one (as expected) and doesn't clear the selection, because it was already cleared by the previous one. Note that it doesn't happen from WTR if we add a waitUntilDone() at the end of the script and call notifyDone() after the InsertText in the timeout function.

[Ryosuke Niwa]

(In reply to Carlos Garcia Campos from comment #1)
> The crash is in DeleteSelectionCommand::removeNode() in line:
> 
> if (!node.parentNode()->hasEditableStyle()) {
> 
> because parentNode() is nullptr. This is happening because
> CompositeEditCommand::removeNode() is called twice for both the embed
> element and the input element. The second time it's called for the input
> element, it has already been removed from the tree, so parent is nullptr.
> And the reason why it's called twice for the same nodes is the weird thing
> here. There are two calls to InsertText, but the second one happens before
> the first one has finished, so the tree is in inconsistent state.

That happens all the time with editing tests because various editing code triggers a synchronous layout update, which can end up executing scripts.

> I suspect it might be a bug in WTR because that doesn't happen when running in
> MiniBrowser. From MiniBrowser the second InsertText command is executed
> after the first one (as expected) and doesn't clear the selection, because
> it was already cleared by the previous one. Note that it doesn't happen from
> WTR if we add a waitUntilDone() at the end of the script and call
> notifyDone() after the InsertText in the timeout function.

Hm... maybe? If it's the timing of when notifyDone is called, then I'm not sure if there is an easy way to fix that since a lot of tests depend on the exact timing of notifyDone.

[Carlos Garcia Campos]

It's not exactly an problem of when notifyDone is called, current test doesn't call notifyDone at all, since a timer is used, the timer is executed after the page has loaded. WTR finishes the test on page loaded when there's no waitUntilDone/notifyDone.

[Ryosuke Niwa]

(In reply to Carlos Garcia Campos from comment #3)
> It's not exactly an problem of when notifyDone is called, current test
> doesn't call notifyDone at all, since a timer is used, the timer is executed
> after the page has loaded. WTR finishes the test on page loaded when there's
> no waitUntilDone/notifyDone.

Ok, then I don't think this is an issue with WTR. It's well understood that invoking execCommand can invoke another execCommand in the middle of it.

[Carlos Garcia Campos]

What makes the difference in WTR is the force repaint that happens before dumping the results. That's the one causing the second InsertText to be executed while the first one hasn't finished yet.

[Carlos Garcia Campos]

Created attachment 427253 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 427253 [details]
Patch

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

> Source/WebCore/editing/DeleteSelectionCommand.cpp:515
> +    if (!node.nonShadowBoundaryParentNode())
> +        return;

Why nonShadowBoundaryParentNode() instead of parentNode()?

[Carlos Garcia Campos]

(In reply to Ryosuke Niwa from comment #7)
> Comment on attachment 427253 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427253&action=review
> 
> > Source/WebCore/editing/DeleteSelectionCommand.cpp:515
> > +    if (!node.nonShadowBoundaryParentNode())
> > +        return;
> 
> Why nonShadowBoundaryParentNode() instead of parentNode()?

It's what CompositeEditCommand::removeNode() does, so I assumed that's what we want here too.

[Ryosuke Niwa]

(In reply to Carlos Garcia Campos from comment #8)
> (In reply to Ryosuke Niwa from comment #7)
> > Comment on attachment 427253 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=427253&action=review
> > 
> > > Source/WebCore/editing/DeleteSelectionCommand.cpp:515
> > > +    if (!node.nonShadowBoundaryParentNode())
> > > +        return;
> > 
> > Why nonShadowBoundaryParentNode() instead of parentNode()?
> 
> It's what CompositeEditCommand::removeNode() does, so I assumed that's what
> we want here too.

Hm... the only difference will be that this will return nullptr when the parent is a shadow root. I don't think that'll make a difference here. That function was introduced back when Google was first implementing the shadow DOM API as a blanket replacement for parentNode to avoid issues in the editing code.

[Ryosuke Niwa]

Comment on attachment 427253 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

This is not a security bug, right? Can we add a test?

[Carlos Garcia Campos]

(In reply to Ryosuke Niwa from comment #9)
> (In reply to Carlos Garcia Campos from comment #8)
> > (In reply to Ryosuke Niwa from comment #7)
> > > Comment on attachment 427253 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=427253&action=review
> > > 
> > > > Source/WebCore/editing/DeleteSelectionCommand.cpp:515
> > > > +    if (!node.nonShadowBoundaryParentNode())
> > > > +        return;
> > > 
> > > Why nonShadowBoundaryParentNode() instead of parentNode()?
> > 
> > It's what CompositeEditCommand::removeNode() does, so I assumed that's what
> > we want here too.
> 
> Hm... the only difference will be that this will return nullptr when the
> parent is a shadow root. I don't think that'll make a difference here. That
> function was introduced back when Google was first implementing the shadow
> DOM API as a blanket replacement for parentNode to avoid issues in the
> editing code.

Ok, I'll use parentNode() then.

[Carlos Garcia Campos]

(In reply to Ryosuke Niwa from comment #10)
> Comment on attachment 427253 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=427253&action=review
> 
> > Source/WebCore/ChangeLog:7
> > +
> 
> This is not a security bug, right? Can we add a test?

It's a use after free

[Carlos Garcia Campos]

(In reply to Carlos Garcia Campos from comment #12)
> (In reply to Ryosuke Niwa from comment #10)
> > Comment on attachment 427253 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=427253&action=review
> > 
> > > Source/WebCore/ChangeLog:7
> > > +
> > 
> > This is not a security bug, right? Can we add a test?
> 
> It's a use after free

I'm not sure why though, but looking at the backtrace the hasEditableStyle() is called.

[Carlos Garcia Campos]

Created attachment 427331 [details]
Patch

[Ryosuke Niwa]

(In reply to Carlos Garcia Campos from comment #13)
> (In reply to Carlos Garcia Campos from comment #12)
> > (In reply to Ryosuke Niwa from comment #10)
> > > Comment on attachment 427253 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=427253&action=review
> > > 
> > > > Source/WebCore/ChangeLog:7
> > > > +
> > > 
> > > This is not a security bug, right? Can we add a test?
> > 
> > It's a use after free
> 
> I'm not sure why though, but looking at the backtrace the hasEditableStyle()
> is called.

Could you figure out why it's a use-after-free? We probably need to put more Ref/RefPtr somewhere.

[Carlos Garcia Campos]

(In reply to Ryosuke Niwa from comment #15)
> (In reply to Carlos Garcia Campos from comment #13)
> > (In reply to Carlos Garcia Campos from comment #12)
> > > (In reply to Ryosuke Niwa from comment #10)
> > > > Comment on attachment 427253 [details]
> > > > Patch
> > > > 
> > > > View in context:
> > > > https://bugs.webkit.org/attachment.cgi?id=427253&action=review
> > > > 
> > > > > Source/WebCore/ChangeLog:7
> > > > > +
> > > > 
> > > > This is not a security bug, right? Can we add a test?
> > > 
> > > It's a use after free
> > 
> > I'm not sure why though, but looking at the backtrace the hasEditableStyle()
> > is called.
> 
> Could you figure out why it's a use-after-free? We probably need to put more
> Ref/RefPtr somewhere.

In my case the bt finishes in:

#0  0x00007f0d04748db0 in WebCore::Node::computeEditability(WebCore::Node::UserSelectAllTreatment, WebCore::Node::ShouldUpdateStyle) const ()

it doesn't get that far, so the node isn't actually used. Maybe it depends on the compiler?

[Ryosuke Niwa]

(In reply to Carlos Garcia Campos from comment #16)
>
> > Could you figure out why it's a use-after-free? We probably need to put more
> > Ref/RefPtr somewhere.
> 
> In my case the bt finishes in:
> 
> #0  0x00007f0d04748db0 in
> WebCore::Node::computeEditability(WebCore::Node::UserSelectAllTreatment,
> WebCore::Node::ShouldUpdateStyle) const ()
> 
> it doesn't get that far, so the node isn't actually used. Maybe it depends
> on the compiler?

So presumably you're seeing an ASAN failures? Could you post the full ASAN stack?

[Carlos Garcia Campos]

(In reply to Ryosuke Niwa from comment #17)
> (In reply to Carlos Garcia Campos from comment #16)
> >
> > > Could you figure out why it's a use-after-free? We probably need to put more
> > > Ref/RefPtr somewhere.
> > 
> > In my case the bt finishes in:
> > 
> > #0  0x00007f0d04748db0 in
> > WebCore::Node::computeEditability(WebCore::Node::UserSelectAllTreatment,
> > WebCore::Node::ShouldUpdateStyle) const ()
> > 
> > it doesn't get that far, so the node isn't actually used. Maybe it depends
> > on the compiler?
> 
> So presumably you're seeing an ASAN failures? Could you post the full ASAN
> stack?

Aha, that must be the difference, my build is a normal release build, not asan.

[Ryosuke Niwa]

(In reply to Carlos Garcia Campos from comment #18)
> (In reply to Ryosuke Niwa from comment #17)
> > (In reply to Carlos Garcia Campos from comment #16)
> > >
> > > > Could you figure out why it's a use-after-free? We probably need to put more
> > > > Ref/RefPtr somewhere.
> > > 
> > > In my case the bt finishes in:
> > > 
> > > #0  0x00007f0d04748db0 in
> > > WebCore::Node::computeEditability(WebCore::Node::UserSelectAllTreatment,
> > > WebCore::Node::ShouldUpdateStyle) const ()
> > > 
> > > it doesn't get that far, so the node isn't actually used. Maybe it depends
> > > on the compiler?
> > 
> > So presumably you're seeing an ASAN failures? Could you post the full ASAN
> > stack?
> 
> Aha, that must be the difference, my build is a normal release build, not
> asan.

If you're using non-ASAN builds, how did you determine that this is a use-after-free?

[Carlos Garcia Campos]

(In reply to Ryosuke Niwa from comment #19)
> (In reply to Carlos Garcia Campos from comment #18)
> > (In reply to Ryosuke Niwa from comment #17)
> > > (In reply to Carlos Garcia Campos from comment #16)
> > > >
> > > > > Could you figure out why it's a use-after-free? We probably need to put more
> > > > > Ref/RefPtr somewhere.
> > > > 
> > > > In my case the bt finishes in:
> > > > 
> > > > #0  0x00007f0d04748db0 in
> > > > WebCore::Node::computeEditability(WebCore::Node::UserSelectAllTreatment,
> > > > WebCore::Node::ShouldUpdateStyle) const ()
> > > > 
> > > > it doesn't get that far, so the node isn't actually used. Maybe it depends
> > > > on the compiler?
> > > 
> > > So presumably you're seeing an ASAN failures? Could you post the full ASAN
> > > stack?
> > 
> > Aha, that must be the difference, my build is a normal release build, not
> > asan.
> 
> If you're using non-ASAN builds, how did you determine that this is a
> use-after-free?

Because fo the backtrace you posted in bug description.

[Ryosuke Niwa]

(In reply to Carlos Garcia Campos from comment #20)
> (In reply to Ryosuke Niwa from comment #19)
> > (In reply to Carlos Garcia Campos from comment #18)
> > > (In reply to Ryosuke Niwa from comment #17)
> > > > (In reply to Carlos Garcia Campos from comment #16)
> > > > >
> > > > > > Could you figure out why it's a use-after-free? We probably need to put more
> > > > > > Ref/RefPtr somewhere.
> > > > > 
> > > > > In my case the bt finishes in:
> > > > > 
> > > > > #0  0x00007f0d04748db0 in
> > > > > WebCore::Node::computeEditability(WebCore::Node::UserSelectAllTreatment,
> > > > > WebCore::Node::ShouldUpdateStyle) const ()
> > > > > 
> > > > > it doesn't get that far, so the node isn't actually used. Maybe it depends
> > > > > on the compiler?
> > > > 
> > > > So presumably you're seeing an ASAN failures? Could you post the full ASAN
> > > > stack?
> > > 
> > > Aha, that must be the difference, my build is a normal release build, not
> > > asan.
> > 
> > If you're using non-ASAN builds, how did you determine that this is a
> > use-after-free?
> 
> Because of the backtrace you posted in bug description.

I don't understand. I don't think there is any indiction that this is a UAF based on the backtrace.

We're crashing here:

void DeleteSelectionCommand::removeNode(Node& node, ShouldAssumeContentIsAlwaysEditable shouldAssumeContentIsAlwaysEditable)
{
    Ref<Node> protectedNode = node;
    if (m_startRoot != m_endRoot && !(node.isDescendantOf(m_startRoot.get()) && node.isDescendantOf(m_endRoot.get()))) {
        // If a node is not in both the start and end editable roots, remove it only if its inside an editable region.
        if (!node.parentNode()->hasEditableStyle()) { // <- this line.


There is protectedNode which refs this node. Here, the issue is that parentNode() is null. treeScope() is where we crash since that's the first place in computeEditability that dereferences "this" inside document().


Node::Editability Node::computeEditability(UserSelectAllTreatment treatment, ShouldUpdateStyle shouldUpdateStyle) const
{
    if (!document().hasLivingRenderTree() || isPseudoElement())
        return Editability::ReadOnly;

[Ryosuke Niwa]

Comment on attachment 427331 [details]
Patch

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

> Source/WebCore/ChangeLog:7
> +

At this point, I'm pretty sure there is no use-after-free here.
Please add a test.

[Carlos Garcia Campos]

Ok, the backtrace confused me, sorry. The problem with the test is that this is not reproducible when using waitUntilDone/notifyDone, because it's the UI process the one triggering the force repaint at a very specific time when he load finishes. If we don't use the waitUntilDone, with the crash fixed, the timeout will be executed after the test has finished, so I wonder if it might affect the next test run.

[Ryosuke Niwa]

(In reply to Carlos Garcia Campos from comment #23)
> Ok, the backtrace confused me, sorry. The problem with the test is that this
> is not reproducible when using waitUntilDone/notifyDone, because it's the UI
> process the one triggering the force repaint at a very specific time when he
> load finishes. If we don't use the waitUntilDone, with the crash fixed, the
> timeout will be executed after the test has finished, so I wonder if it
> might affect the next test run.

Can we add two test files ~-1.html & ~-2.html, and add a comment saying that ~-1.html passes if we didn't hit a crash in ~-2.html?

[Carlos Garcia Campos]

(In reply to Ryosuke Niwa from comment #24)
> (In reply to Carlos Garcia Campos from comment #23)
> > Ok, the backtrace confused me, sorry. The problem with the test is that this
> > is not reproducible when using waitUntilDone/notifyDone, because it's the UI
> > process the one triggering the force repaint at a very specific time when he
> > load finishes. If we don't use the waitUntilDone, with the crash fixed, the
> > timeout will be executed after the test has finished, so I wonder if it
> > might affect the next test run.
> 
> Can we add two test files ~-1.html & ~-2.html, and add a comment saying that
> ~-1.html passes if we didn't hit a crash in ~-2.html?

I'm not sure I get how that could work. How does WTR know it has run two files?

[Ryosuke Niwa]

(In reply to Carlos Garcia Campos from comment #25)
> (In reply to Ryosuke Niwa from comment #24)
> > (In reply to Carlos Garcia Campos from comment #23)
> > > Ok, the backtrace confused me, sorry. The problem with the test is that this
> > > is not reproducible when using waitUntilDone/notifyDone, because it's the UI
> > > process the one triggering the force repaint at a very specific time when he
> > > load finishes. If we don't use the waitUntilDone, with the crash fixed, the
> > > timeout will be executed after the test has finished, so I wonder if it
> > > might affect the next test run.
> > 
> > Can we add two test files ~-1.html & ~-2.html, and add a comment saying that
> > ~-1.html passes if we didn't hit a crash in ~-2.html?
> 
> I'm not sure I get how that could work. How does WTR know it has run two
> files?

Tests are always ordered in the lexicological order.

[Carlos Garcia Campos]

(In reply to Ryosuke Niwa from comment #26)
> (In reply to Carlos Garcia Campos from comment #25)
> > (In reply to Ryosuke Niwa from comment #24)
> > > (In reply to Carlos Garcia Campos from comment #23)
> > > > Ok, the backtrace confused me, sorry. The problem with the test is that this
> > > > is not reproducible when using waitUntilDone/notifyDone, because it's the UI
> > > > process the one triggering the force repaint at a very specific time when he
> > > > load finishes. If we don't use the waitUntilDone, with the crash fixed, the
> > > > timeout will be executed after the test has finished, so I wonder if it
> > > > might affect the next test run.
> > > 
> > > Can we add two test files ~-1.html & ~-2.html, and add a comment saying that
> > > ~-1.html passes if we didn't hit a crash in ~-2.html?
> > 
> > I'm not sure I get how that could work. How does WTR know it has run two
> > files?
> 
> Tests are always ordered in the lexicological order.

But you should be able to run tests individually. They are supposed to be independent from each other

[Ryosuke Niwa]

(In reply to Carlos Garcia Campos from comment #27)
> (In reply to Ryosuke Niwa from comment #26)
> > (In reply to Carlos Garcia Campos from comment #25)
> > > (In reply to Ryosuke Niwa from comment #24)
> > > > (In reply to Carlos Garcia Campos from comment #23)
> > > > > Ok, the backtrace confused me, sorry. The problem with the test is that this
> > > > > is not reproducible when using waitUntilDone/notifyDone, because it's the UI
> > > > > process the one triggering the force repaint at a very specific time when he
> > > > > load finishes. If we don't use the waitUntilDone, with the crash fixed, the
> > > > > timeout will be executed after the test has finished, so I wonder if it
> > > > > might affect the next test run.
> > > > 
> > > > Can we add two test files ~-1.html & ~-2.html, and add a comment saying that
> > > > ~-1.html passes if we didn't hit a crash in ~-2.html?
> > > 
> > > I'm not sure I get how that could work. How does WTR know it has run two
> > > files?
> > 
> > Tests are always ordered in the lexicological order.
> 
> But you should be able to run tests individually. They are supposed to be
> independent from each other

Yeah, so it's not great but we may not have much choice in this case. It's definitely a lot better than not adding a test. Have you tried running testRunner.display() ?

[Carlos Garcia Campos]

(In reply to Ryosuke Niwa from comment #28)
> (In reply to Carlos Garcia Campos from comment #27)
> > (In reply to Ryosuke Niwa from comment #26)
> > > (In reply to Carlos Garcia Campos from comment #25)
> > > > (In reply to Ryosuke Niwa from comment #24)
> > > > > (In reply to Carlos Garcia Campos from comment #23)
> > > > > > Ok, the backtrace confused me, sorry. The problem with the test is that this
> > > > > > is not reproducible when using waitUntilDone/notifyDone, because it's the UI
> > > > > > process the one triggering the force repaint at a very specific time when he
> > > > > > load finishes. If we don't use the waitUntilDone, with the crash fixed, the
> > > > > > timeout will be executed after the test has finished, so I wonder if it
> > > > > > might affect the next test run.
> > > > > 
> > > > > Can we add two test files ~-1.html & ~-2.html, and add a comment saying that
> > > > > ~-1.html passes if we didn't hit a crash in ~-2.html?
> > > > 
> > > > I'm not sure I get how that could work. How does WTR know it has run two
> > > > files?
> > > 
> > > Tests are always ordered in the lexicological order.
> > 
> > But you should be able to run tests individually. They are supposed to be
> > independent from each other
> 
> Yeah, so it's not great but we may not have much choice in this case. It's
> definitely a lot better than not adding a test. Have you tried running
> testRunner.display() ?

Yes, I tried that, at different places, but when triggered from the web process, it's always done when the InsertText command has completed.

[Ryosuke Niwa]

(In reply to Carlos Garcia Campos from comment #29)
> (In reply to Ryosuke Niwa from comment #28)
> > (In reply to Carlos Garcia Campos from comment #27)
> >
> > Yeah, so it's not great but we may not have much choice in this case. It's
> > definitely a lot better than not adding a test. Have you tried running
> > testRunner.display() ?
> 
> Yes, I tried that, at different places, but when triggered from the web
> process, it's always done when the InsertText command has completed.

Yeah, so in that case, we probably just need to add two files.

[Carlos Garcia Campos]

I have debugged this further. It's not the UI process causing the force repaint, it happens inside the web process when the main resource load finishes. And the main load finish si caused by the embed element removal caused by the delete selection command. During the command executions, there's an event scope, so we can't add a listener for DOMNodeRemoved to force the repaint at that point. I think this bug is impossible to reproduce outside of WTR. What we can do to make a test is to add testRunner API to tell WTR to force a repaint on load finished.

[Carlos Garcia Campos]

Created attachment 427878 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 427878 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Test: editing/inserting/insert-text-force-repaint-on-load-crash.html

Skip the test on WK1 maybe?
Alternatively, you can call testRunner.displayOnLoadFinish conditionally.

[Carlos Garcia Campos]

Created attachment 427992 [details]
Patch for landing

[EWS]

Downloading mechanize-0.4.5...
Installing mechanize-0.4.5...
Installed mechanize-0.4.5!
ChangeLog entry in Tools/ChangeLog contains OOPS!.

[EWS]

Committed r277202 (237474@main): <https://commits.webkit.org/237474@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 427992 [details].