Bug 51686 – [GTK] Closing a window during an onload event can trigger serious GLib warnings

• Component: WebKitGTK
• Status: RESOLVED FIXED

[Martin Robinson]

This can be seen by looking at the stderr output of fast/dom/Window/window-early-properties-stderr.txt:

GSettings schema not found - settings will not be used or saved.
GSettings schema not found - settings will not be used or saved.
GSettings schema not found - settings will not be used or saved.
instance with invalid (NULL) class pointer
g_signal_emit_by_name: assertion `G_TYPE_CHECK_INSTANCE (instance)' failed

[Martin Robinson]

Created attachment 77573 [details]
Patch for this issue

[Xan Lopez]

Comment on attachment 77573 [details]
Patch for this issue

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

> WebKit/gtk/ChangeLog:13
> +

If this were the problem just reffing the view before emitting the signal would be enough; from our talk I understand that it must survive for a while after this function is finished, which is different. Am I wrong?

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:255
> +    gboolean isHandled = FALSE;

No need to initialize this, and perhaps you could call it 'dummy'.

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:267
>  

Aren't you checking for m_closeSoonTimer right before?

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:268
> +    m_webView->priv->corePage->setGroupName("");

And this?

> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:272
> +    // signal, but our caller my need to send more signals web view. For

'more signals web view'? Missing 'to the'?

[Martin Robinson]

Comment on attachment 77573 [details]
Patch for this issue

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

Thanks for the review!

>> WebKit/gtk/ChangeLog:13
>> +
> 
> If this were the problem just reffing the view before emitting the signal would be enough; from our talk I understand that it must survive for a while after this function is finished, which is different. Am I wrong?

I think my entry doesn't explain the problem fully. close-web-view essentially kills the view (because the client may remove the last reference), but the caller (FrameLoader) depends on the view still existing.

>> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:255
>> +    gboolean isHandled = FALSE;
> 
> No need to initialize this, and perhaps you could call it 'dummy'.

I'm fine with removing the initialization (I prefer to initialize things unconditionally), but I think isHandled better explains what it is and why I'm ignoring it.

>> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:267
>>  
> 
> Aren't you checking for m_closeSoonTimer right before?

Oh right. I separated the check out after adding it to line 263 and forgot to remove the old one. Thanks!

>> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:268
>> +    m_webView->priv->corePage->setGroupName("");
> 
> And this?

It's necessary so that JavaScript can't find the page. It originates from the other ports. I'll leave a comment.

>> WebKit/gtk/WebCoreSupport/ChromeClientGtk.cpp:272
>> +    // signal, but our caller my need to send more signals web view. For
> 
> 'more signals web view'? Missing 'to the'?

Oh yeah. Should be "more signals to the web view"

[Martin Robinson]

Created attachment 77583 [details]
Patch wit Xan's suggestions

[Martin Robinson]

Committed r74731: <http://trac.webkit.org/changeset/74731>