Bug 207529 – [WPE][WebDriver] MiniBrowser should react to close session commands

• Component: WPE WebKit
• Status: RESOLVED FIXED

[Lauro Moura]

- 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

[Lauro Moura]

Created attachment 390336 [details]
Patch

[Lauro Moura]

(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.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 390336 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 390336 [details]:

editing/spelling/spellcheck-attribute.html bug 206178 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 390336 [details]
Patch

Clearing flags on attachment: 390336

Committed r256302: <https://trac.webkit.org/changeset/256302>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yury Semikhatsky]

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 ()

[Yury Semikhatsky]

Reopening to attach new patch.

[Yury Semikhatsky]

Created attachment 390685 [details]
Patch

[Yury Semikhatsky]

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?

[Carlos Garcia Campos]

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.

[Yury Semikhatsky]

Created attachment 390792 [details]
Patch

[Yury Semikhatsky]

(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)

[Carlos Garcia Campos]

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.

[Carlos Garcia Campos]

(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.

[Lauro Moura]

Created attachment 392662 [details]
Updated patch after cgarcia comments.

Updated with the weakref comments. Opened bug208689 for the closing of child views.

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 392662 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 392662 [details]
Updated patch after cgarcia comments.

Clearing flags on attachment: 392662

Committed r257973: <https://trac.webkit.org/changeset/257973>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.