Bug 73802 - [Qt] REGRESSION(101967): It made editing/style/iframe-onload-crash-mac.html timeout
: [Qt] REGRESSION(101967): It made editing/style/iframe-onload-crash-mac.html t...
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: Tools / Tests
: 528+ (Nightly build)
: All All
: P1 Critical
Assigned To: Caio Marcelo de Oliveira Filho
: Qt, QtTriaged
Depends on:
Blocks: 73737 79666 79668
  Show dependency treegraph
 
Reported: 2011-12-05 00:10 PST by Csaba Osztrogonác
Modified: 2012-05-17 10:54 PDT (History)
9 users (show)

See Also:


Attachments
proposed fix (1.39 KB, patch)
2012-04-16 00:01 PDT, Kristóf Kosztyó
no flags Details | Formatted Diff | Diff
Patch (6.54 KB, patch)
2012-05-16 17:55 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff
Patch for landing (6.65 KB, patch)
2012-05-17 09:48 PDT, Caio Marcelo de Oliveira Filho
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Csaba Osztrogonác 2011-12-05 00:10:20 PST
After r101967 editing/style/iframe-onload-crash-mac.html fails with timeout.
Comment 1 Csaba Osztrogonác 2011-12-05 00:21:32 PST
I skipped the failing test on Qt: http://trac.webkit.org/changeset/101976
Comment 2 Ryosuke Niwa 2011-12-05 08:12:13 PST
Does the test time out when it's ran by itself?
Comment 3 Csaba Osztrogonác 2011-12-05 08:13:19 PST
(In reply to comment #2)
> Does the test time out when it's ran by itself?

Yes.
Comment 4 Ryosuke Niwa 2011-12-05 08:19:11 PST
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.
Comment 5 Kristóf Kosztyó 2012-04-16 00:01:42 PDT
Created attachment 137288 [details]
proposed fix

Hi
I have found this test is falling into an infinite loop at ApplyStyleCommand::pushDownInlineStyleAroundNode
Comment 6 Ryosuke Niwa 2012-04-16 00:04:49 PDT
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?
Comment 7 Kristóf Kosztyó 2012-04-16 00:39:45 PDT
(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.
Comment 8 Ryosuke Niwa 2012-04-16 00:42:29 PDT
(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?
Comment 9 Caio Marcelo de Oliveira Filho 2012-05-16 12:51:23 PDT
I'm investigating this.
Comment 10 Caio Marcelo de Oliveira Filho 2012-05-16 17:55:21 PDT
Created attachment 142382 [details]
Patch
Comment 11 Ryosuke Niwa 2012-05-16 17:59:45 PDT
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.
Comment 12 Caio Marcelo de Oliveira Filho 2012-05-17 09:48:59 PDT
Created attachment 142491 [details]
Patch for landing
Comment 13 WebKit Review Bot 2012-05-17 10:53:57 PDT
Comment on attachment 142491 [details]
Patch for landing

Clearing flags on attachment: 142491

Committed r117463: <http://trac.webkit.org/changeset/117463>
Comment 14 WebKit Review Bot 2012-05-17 10:54:06 PDT
All reviewed patches have been landed.  Closing bug.