Bug 122571

Summary: [GTK] Inspector protocol tests timing out on the bots
Product: WebKit Reporter: Gustavo Noronha (kov) <gns>
Component: WebKitGTKAssignee: Yury Semikhatsky <yurys>
Status: RESOLVED FIXED    
Severity: Normal CC: a.obzhirov, bugs-noreply, calvaris, cgarcia, chavarria1991, clopez, commit-queue, drousso, yurys
Priority: P2 Keywords: LayoutTestFailure
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Gustavo Noronha (kov) 2013-10-09 13:34:58 PDT
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" <sam@webkit.org>
  Reviewer: "Darin Adler" <darin@apple.com>
  Committer: "Sam Weinig" <sam@webkit.org>
r157147:
  http://trac.webkit.org/changeset/157147
  Bug: 122530 (https://bugs.webkit.org/show_bug.cgi?id=122530)
  Author: "Alex Christensen" <achristensen@apple.com>
  Reviewer: "Brent Fulgham" <bfulgham@webkit.org>
  Committer: None
r157148:
  http://trac.webkit.org/changeset/157148
  Bug: None (None)
  Author: "Gustavo Noronha Silva" <gns@gnome.org>
  Reviewer: None
  Committer: "Gustavo Noronha Silva" <gns@gnome.org>
r157149:
  http://trac.webkit.org/changeset/157149
  Bug: None (None)
  Author: "Gustavo Noronha Silva" <gns@gnome.org>
  Reviewer: None
  Committer: "Gustavo Noronha Silva" <gns@gnome.org>
r157150:
  http://trac.webkit.org/changeset/157150
  Bug: 122532 (https://bugs.webkit.org/show_bug.cgi?id=122532)
  Author: "Oliver Hunt" <oliver@apple.com>
  Reviewer: "Michael Saboff" <msaboff@apple.com>
  Committer: "Oliver Hunt" <oliver@apple.com>
Comment 1 Anton Obzhirov 2013-10-23 05:56:42 PDT
Time for me to have a look :)
Comment 2 Carlos Alberto Lopez Perez 2014-04-16 10:08:32 PDT
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
Comment 3 Carlos Garcia Campos 2014-04-23 00:53:56 PDT
(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.
Comment 4 Xabier Rodríguez Calvar 2014-04-28 04:29:41 PDT
*** Bug 132259 has been marked as a duplicate of this bug. ***
Comment 5 Xabier Rodríguez Calvar 2014-04-28 04:30:28 PDT
Added inspector-protocol/debugger/setBreakpoint-dfg.html [ Failure Pass ]
Comment 6 Xabier Rodríguez Calvar 2014-04-29 02:24:41 PDT
inspector-protocol/profiler/console-profile.html [ Failure Pass ]
Comment 7 Carlos Alberto Lopez Perez 2014-07-09 03:29:26 PDT
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
Comment 8 Marcos Chavarría Teijeiro (irc: chavaone) 2015-02-17 06:53:56 PST
Updating expectation for inspector-protocol/debugger/setBreakpoint-actions.html:

inspector-protocol/debugger/setBreakpoint-actions.html [ Failure Timeout Pass ]
Comment 9 Yury Semikhatsky 2019-11-04 11:58:58 PST
Created attachment 382755 [details]
Patch
Comment 10 Devin Rousso 2019-11-04 13:23:14 PST
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.
Comment 11 Yury Semikhatsky 2019-11-04 14:27:03 PST
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.
Comment 12 Devin Rousso 2019-11-04 14:34:32 PST
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});
```
Comment 13 Yury Semikhatsky 2019-11-04 14:49:04 PST
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.
Comment 14 Yury Semikhatsky 2019-11-04 14:49:26 PST
Created attachment 382776 [details]
Patch
Comment 15 Devin Rousso 2019-11-04 14:58:42 PST
Comment on attachment 382776 [details]
Patch

rs=me
Comment 16 WebKit Commit Bot 2019-11-04 16:26:39 PST
Comment on attachment 382776 [details]
Patch

Clearing flags on attachment: 382776

Committed r252027: <https://trac.webkit.org/changeset/252027>
Comment 17 WebKit Commit Bot 2019-11-04 16:26:41 PST
All reviewed patches have been landed.  Closing bug.