Bug 122571 – [GTK] Inspector protocol tests timing out on the bots

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Gustavo Noronha (kov)]

I couldn't reproduce locally. They started timing out on the bot when the following revisions were built:

r157146:
  http://trac.webkit.org/changeset/157146
  Bug: 122480 (https://bugs.webkit.org/show_bug.cgi?id=122480)
  Author: "Sam Weinig" <[e-mail]>
  Reviewer: "Darin Adler" <[e-mail]>
  Committer: "Sam Weinig" <[e-mail]>
r157147:
  http://trac.webkit.org/changeset/157147
  Bug: 122530 (https://bugs.webkit.org/show_bug.cgi?id=122530)
  Author: "Alex Christensen" <[e-mail]>
  Reviewer: "Brent Fulgham" <[e-mail]>
  Committer: None
r157148:
  http://trac.webkit.org/changeset/157148
  Bug: None (None)
  Author: "Gustavo Noronha Silva" <[e-mail]>
  Reviewer: None
  Committer: "Gustavo Noronha Silva" <[e-mail]>
r157149:
  http://trac.webkit.org/changeset/157149
  Bug: None (None)
  Author: "Gustavo Noronha Silva" <[e-mail]>
  Reviewer: None
  Committer: "Gustavo Noronha Silva" <[e-mail]>
r157150:
  http://trac.webkit.org/changeset/157150
  Bug: 122532 (https://bugs.webkit.org/show_bug.cgi?id=122532)
  Author: "Oliver Hunt" <[e-mail]>
  Reviewer: "Michael Saboff" <[e-mail]>
  Committer: "Oliver Hunt" <[e-mail]>

[Anton Obzhirov]

Time for me to have a look :)

[Carlos Alberto Lopez Perez]

I applied locally the patch on https://bugs.webkit.org/show_bug.cgi?id=131675 and now the test not longer times out.

However it fails:

TEST: WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestInspectorServer... (pid=30953)
**
ERROR:../../Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestInspectorServer.cpp:241:void openRemoteDebuggingSession(InspectorServerTest*, gconstpointer): assertion failed: (javascriptResult)
GTester: last random seed: R02S76a375c7b7e9a9fb04f17fded1d0cf78
(pid=31125)
FAIL: WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestInspectorServer
Tests failed (1): WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestInspectorServer

[Carlos Garcia Campos]

(In reply to comment #2)
> I applied locally the patch on https://bugs.webkit.org/show_bug.cgi?id=131675 and now the test not longer times out.
> 
> However it fails:
> 
> TEST: WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestInspectorServer... (pid=30953)
> **
> ERROR:../../Tools/TestWebKitAPI/Tests/WebKit2Gtk/TestInspectorServer.cpp:241:void openRemoteDebuggingSession(InspectorServerTest*, gconstpointer): assertion failed: (javascriptResult)
> GTester: last random seed: R02S76a375c7b7e9a9fb04f17fded1d0cf78
> (pid=31125)
> FAIL: WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestInspectorServer
> Tests failed (1): WebKitBuild/Release/bin/TestWebKitAPI/WebKit2Gtk/TestInspectorServer

I don't think this bug is about the unit tests, but the layout tests. The unit test started to fail recently, I'll open a new bug.

[Xabier Rodríguez Calvar]

*** Bug 132259 has been marked as a duplicate of this bug. ***

[Xabier Rodríguez Calvar]

Added inspector-protocol/debugger/setBreakpoint-dfg.html [ Failure Pass ]

[Xabier Rodríguez Calvar]

inspector-protocol/profiler/console-profile.html [ Failure Pass ]

[Carlos Alberto Lopez Perez]

I'm updating the expectation for inspector-protocol/dom/focus.html to [ Failure Timeout Pass ] since it fails sometimes.

The failure is this: http://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20%28Tests%29/r170902%20%281301%29/inspector-protocol/dom/focus-pretty-diff.html

[Marcos Chavarría Teijeiro (irc: chavaone)]

Updating expectation for inspector-protocol/debugger/setBreakpoint-actions.html:

inspector-protocol/debugger/setBreakpoint-actions.html [ Failure Timeout Pass ]

[Yury Semikhatsky]

Created attachment 382755 [details]
Patch

[Devin Rousso]

Comment on attachment 382755 [details]
Patch

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

> LayoutTests/ChangeLog:12
> +        window. Otherwise it triggers a WebPage::setActivityState which in turn triggers focus
> +        event on the page and on the focused element.

How does this cause a timeout/crash?  Based on your description, it seems like the real fix would be somewhere in the code that relates to handling focus, not the test file itself.  The code in inspector/dom/focus.html seems reasonable.

[Yury Semikhatsky]

Comment on attachment 382755 [details]
Patch

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

>> LayoutTests/ChangeLog:12
>> +        event on the page and on the focused element.
> 
> How does this cause a timeout/crash?  Based on your description, it seems like the real fix would be somewhere in the code that relates to handling focus, not the test file itself.  The code in inspector/dom/focus.html seems reasonable.

This only fixes text failure (two extra 'focus' events).

I don't know what fixed crash but running 10k iterations doesn't trigger it.

Timeout was likely fixed before by my changes to the event loop used in the local frontend client.

[Devin Rousso]

Comment on attachment 382755 [details]
Patch

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

>>> LayoutTests/ChangeLog:12
>>> +        event on the page and on the focused element.
>> 
>> How does this cause a timeout/crash?  Based on your description, it seems like the real fix would be somewhere in the code that relates to handling focus, not the test file itself.  The code in inspector/dom/focus.html seems reasonable.
> 
> This only fixes text failure (two extra 'focus' events).
> 
> I don't know what fixed crash but running 10k iterations doesn't trigger it.
> 
> Timeout was likely fixed before by my changes to the event loop used in the local frontend client.

If that's the case, please say so in the ChangeLog so it doesn't make this change look like a "let's hide the underlying issue by changing the test".

> LayoutTests/inspector/dom/focus.html:12
> +    document.querySelector("#second").addEventListener("focus", focusListener);

Rather than add and then remove the event listener, could we make it a single-fire event listener?
```
    document.querySelector("#second").addEventListener("focus", function() {
        log("focused");
    }, {once: true});
```

[Yury Semikhatsky]

Comment on attachment 382755 [details]
Patch

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

>>>> LayoutTests/ChangeLog:12
>>>> +        event on the page and on the focused element.
>>> 
>>> How does this cause a timeout/crash?  Based on your description, it seems like the real fix would be somewhere in the code that relates to handling focus, not the test file itself.  The code in inspector/dom/focus.html seems reasonable.
>> 
>> This only fixes text failure (two extra 'focus' events).
>> 
>> I don't know what fixed crash but running 10k iterations doesn't trigger it.
>> 
>> Timeout was likely fixed before by my changes to the event loop used in the local frontend client.
> 
> If that's the case, please say so in the ChangeLog so it doesn't make this change look like a "let's hide the underlying issue by changing the test".

Done.

>> LayoutTests/inspector/dom/focus.html:12
>> +    document.querySelector("#second").addEventListener("focus", focusListener);
> 
> Rather than add and then remove the event listener, could we make it a single-fire event listener?
> ```
>     document.querySelector("#second").addEventListener("focus", function() {
>         log("focused");
>     }, {once: true});
> ```

This way we wouldn't be able to catch scenario where DOM.focus erroneously triggers multiple focus events on the element.

[Yury Semikhatsky]

Created attachment 382776 [details]
Patch

[Devin Rousso]

Comment on attachment 382776 [details]
Patch

rs=me

[WebKit Commit Bot]

Comment on attachment 382776 [details]
Patch

Clearing flags on attachment: 382776

Committed r252027: <https://trac.webkit.org/changeset/252027>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.