- Many WebDriver tests require the current WebDriver session to be closed and a new one to be created. - When a session is closed, it causes the associated WebView to trigger the "close" signal. - Currently, WPE's MiniBrowser only listens to "close" on the child WebViews and not in the main one, which is registered as the automation WebView - This causes lots of timeouts when running the WebDriver tests. Delete session command: https://www.w3.org/TR/webdriver/#delete-session
Created attachment 390336 [details] Patch
(In reply to Lauro Moura from comment #1) > Created attachment 390336 [details] > Patch With this, the test run went from ~26min to 14min. Output comparing upstream (baseline) and patched (current): Summary of changes from baseline: ERROR -> PASS : 74 FAIL -> PASS : 1 TIMEOUT -> PASS : 1 This should also improve the results for bug207372.
The commit-queue encountered the following flaky tests while processing attachment 390336 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.
The commit-queue encountered the following flaky tests while processing attachment 390336 [details]: editing/spelling/spellcheck-attribute.html bug 206178 (authors: g.czajkowski@samsung.com, mark.lam@apple.com, and rniwa@webkit.org) The commit-queue is continuing to process your patch.
Comment on attachment 390336 [details] Patch Clearing flags on attachment: 390336 Committed r256302: <https://trac.webkit.org/changeset/256302>
All reviewed patches have been landed. Closing bug.
Comment on attachment 390336 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390336&action=review > Tools/MiniBrowser/wpe/main.cpp:303 > + g_signal_connect(webView, "close", G_CALLBACK(webViewClose), nullptr); This handler unrefs the webView and may cause it to be deleted, in which case g_object_unref(webView); call below (https://trac.webkit.org/browser/webkit/trunk/Tools/MiniBrowser/wpe/main.cpp?rev=256302#L329) will cause use after free. I'm observing the following crash: Thread 1 (Thread 0x7fb9572f99c0 (LWP 25208)): #0 g_type_check_instance_is_fundamentally_a (type_instance=0x2188130, fundamental_type=80) at ../../Source/glib-2.58.1/gobject/gtype.c:4025 node = <optimized out> #1 0x00007fb95d295785 in g_object_unref (_object=0x2188130) at ../../Source/glib-2.58.1/gobject/gobject.c:3243 _g_boolean_var_ = <optimized out> object = 0x2188130 old_ref = <optimized out> #2 0x0000000000405ccd in main ()
Reopening to attach new patch.
Created attachment 390685 [details] Patch
The patch removes now redundant g_object_unref but I'm not entirely sure about what is expected to happen to the existing WebKitWebViews should g_main_loop_run terminate before they are all closed. It looks like if Ctrl+Q is pressed g_main_loop_quit will be called and the process may exit with a bunch of WebKitWebViews still alive. In GTK the loop is only terminated once all mini browser windows are gone so maybe WPE mini browser should also keep track of all created views and close them before exiting?
Comment on attachment 390685 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390685&action=review > Tools/MiniBrowser/wpe/main.cpp:-329 > - g_object_unref(webView); hmm, I don't think close is emitted for the main web view when it's closed with CTRL+Q. We might need to use a weak pointer.
Created attachment 390792 [details] Patch
(In reply to Carlos Garcia Campos from comment #11) > Comment on attachment 390685 [details] > hmm, I don't think close is emitted for the main web view when it's closed > with CTRL+Q. We might need to use a weak pointer. Updated the fix to use weak ref instead. Do you think it'd make sense to also close child views if they are still alive? (something like Comment#10)
Comment on attachment 390792 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=390792&action=review > Tools/MiniBrowser/wpe/main.cpp:179 > +void webViewFinalized(gpointer data, GObject*) > +{ > + auto viewPointer = static_cast<WebKitWebView**>(data); > + *viewPointer = nullptr; > +} You don't need this ifg you use a weak pointer. > Tools/MiniBrowser/wpe/main.cpp:297 > + g_object_weak_ref(G_OBJECT(webView), webViewFinalized, &webView); Use g_object_add_weak_pointer() instead. > Tools/MiniBrowser/wpe/main.cpp:337 > + g_object_weak_unref(G_OBJECT(webView), webViewFinalized, &webView); And g_object_remove_weak_pointer here.
(In reply to Yury Semikhatsky from comment #13) > (In reply to Carlos Garcia Campos from comment #11) > > Comment on attachment 390685 [details] > > hmm, I don't think close is emitted for the main web view when it's closed > > with CTRL+Q. We might need to use a weak pointer. > > Updated the fix to use weak ref instead. Do you think it'd make sense to > also close child views if they are still alive? (something like Comment#10) Yes, I guess so.
Created attachment 392662 [details] Updated patch after cgarcia comments. Updated with the weakref comments. Opened bug208689 for the closing of child views.
The commit-queue encountered the following flaky tests while processing attachment 392662 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.
Comment on attachment 392662 [details] Updated patch after cgarcia comments. Clearing flags on attachment: 392662 Committed r257973: <https://trac.webkit.org/changeset/257973>