Summary: | [GTK] Inspector protocol tests timing out on the bots | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Gustavo Noronha (kov) <gustavo> | ||||||
Component: | WebKitGTK | Assignee: | Yury Semikhatsky <yurys> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | bugs-noreply, calvaris, cgarcia, chavarria1991, clopez, commit-queue, hi, obzhirov, yurys | ||||||
Priority: | P2 | Keywords: | LayoutTestFailure | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Gustavo Noronha (kov)
2013-10-09 13:34:58 PDT
Time for me to have a look :) 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 (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. *** Bug 132259 has been marked as a duplicate of this bug. *** Added inspector-protocol/debugger/setBreakpoint-dfg.html [ Failure Pass ] inspector-protocol/profiler/console-profile.html [ Failure Pass ] 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 Updating expectation for inspector-protocol/debugger/setBreakpoint-actions.html: inspector-protocol/debugger/setBreakpoint-actions.html [ Failure Timeout Pass ] Created attachment 382755 [details]
Patch
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 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 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 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. Created attachment 382776 [details]
Patch
Comment on attachment 382776 [details]
Patch
rs=me
Comment on attachment 382776 [details] Patch Clearing flags on attachment: 382776 Committed r252027: <https://trac.webkit.org/changeset/252027> All reviewed patches have been landed. Closing bug. |