Bug 73802 – [Qt] REGRESSION(101967): It made editing/style/iframe-onload-crash-mac.html timeout

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Csaba Osztrogonác]

After r101967 editing/style/iframe-onload-crash-mac.html fails with timeout.

[Csaba Osztrogonác]

I skipped the failing test on Qt: http://trac.webkit.org/changeset/101976

[Ryosuke Niwa]

Does the test time out when it's ran by itself?

[Csaba Osztrogonác]

(In reply to comment #2)
> Does the test time out when it's ran by itself?

Yes.

[Ryosuke Niwa]

But I don't understand why this is failing only on Qt. e.g. GTK must be using the same behavior as Qt (this test only behaves differently on GTK), but it's not timing out there.

Regardless, I don't have any machine that can build Qt at the moment, and I need to let someone from Qt port investigate this as this failure is happening only on Qt.

[Kristóf Kosztyó]

Created attachment 137288 [details]
proposed fix

Hi
I have found this test is falling into an infinite loop at ApplyStyleCommand::pushDownInlineStyleAroundNode

[Ryosuke Niwa]

Comment on attachment 137288 [details]
proposed fix

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

> Source/WebCore/editing/ApplyStyleCommand.cpp:968
> +        if (!(current->contains(targetNode)))
> +            break;

This should never be the case. When/why is this happening?

[Kristóf Kosztyó]

(In reply to comment #6)
> (From update of attachment 137288 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=137288&action=review
> 
> > Source/WebCore/editing/ApplyStyleCommand.cpp:968
> > +        if (!(current->contains(targetNode)))
> > +            break;
> 
> This should never be the case. When/why is this happening?

It happens when this test run, but I don't know why. This test fails into an infinite loop and it assert with debug build.

[Ryosuke Niwa]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 137288 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=137288&action=review
> > 
> > > Source/WebCore/editing/ApplyStyleCommand.cpp:968
> > > +        if (!(current->contains(targetNode)))
> > > +            break;
> > 
> > This should never be the case. When/why is this happening?
> 
> It happens when this test run, but I don't know why. This test fails into an infinite loop and it assert with debug build.

Could you step it through on the debugger and tell us how we fall into this bad state?

[Caio Marcelo de Oliveira Filho]

I'm investigating this.

[Caio Marcelo de Oliveira Filho]

Created attachment 142382 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 142382 [details]
Patch

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

> Source/WebCore/editing/ApplyStyleCommand.cpp:995
> +            Node* child = currentChildren.at(i).get();

Please do currentChildren[i].get(); instead.

> Source/WebCore/editing/ApplyStyleCommand.cpp:996
> +            if (!child->parentNode())

We could go with an even stricter check like child->parentNode() != current.

[Caio Marcelo de Oliveira Filho]

Created attachment 142491 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 142491 [details]
Patch for landing

Clearing flags on attachment: 142491

Committed r117463: <http://trac.webkit.org/changeset/117463>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.