Bug 51686 - [GTK] Closing a window during an onload event can trigger serious GLib warnings
Summary: [GTK] Closing a window during an onload event can trigger serious GLib warnings
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks:
 
Reported: 2010-12-28 13:16 PST by Martin Robinson
Modified: 2010-12-28 17:07 PST (History)
3 users (show)

See Also:


Attachments
Patch for this issue (3.70 KB, patch)
2010-12-28 13:37 PST, Martin Robinson
no flags Details | Formatted Diff | Diff
Patch wit Xan's suggestions (4.22 KB, patch)
2010-12-28 15:13 PST, Martin Robinson
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Robinson 2010-12-28 13:16:59 PST
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
Comment 1 Martin Robinson 2010-12-28 13:37:53 PST
Created attachment 77573 [details]
Patch for this issue
Comment 2 Xan Lopez 2010-12-28 13:44:14 PST
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'?
Comment 3 Martin Robinson 2010-12-28 15:04:50 PST
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"
Comment 4 Martin Robinson 2010-12-28 15:13:08 PST
Created attachment 77583 [details]
Patch wit Xan's suggestions
Comment 5 Martin Robinson 2010-12-28 16:40:07 PST
Committed r74731: <http://trac.webkit.org/changeset/74731>