Bug 50173 - corrupted pointer in webkit_web_view_get_resource
Summary: corrupted pointer in webkit_web_view_get_resource
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-29 12:59 PST by Kevin Fenzi
Modified: 2015-05-07 17:18 PDT (History)
3 users (show)

See Also:


Attachments
proposed fix (760 bytes, patch)
2011-06-12 04:10 PDT, tuxator
xan.lopez: review-
Details | Formatted Diff | Diff
Incorporating review comments (1.65 KB, patch)
2011-06-13 04:35 PDT, Naiem
no flags Details | Formatted Diff | Diff
update ref counts (2.10 KB, patch)
2011-06-13 08:04 PDT, tuxator
no flags Details | Formatted Diff | Diff
correct version of previous patch (2.18 KB, patch)
2011-06-13 09:12 PDT, tuxator
mrobinson: review-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kevin Fenzi 2010-11-29 12:59:23 PST
We are seeing a crash here in webkit_web_view_get resource. 
This is webkitgtk-1.3.6. 

The downstream report is: 

https://bugzilla.redhat.com/show_bug.cgi?id=657683

The stack trace is available at: 

https://bugzilla.redhat.com/attachment.cgi?id=463209
Comment 1 tuxator 2011-06-12 04:10:38 PDT
Created attachment 96876 [details]
proposed fix
Comment 2 Xan Lopez 2011-06-12 07:07:55 PDT
Comment on attachment 96876 [details]
proposed fix

When we remove the main resource we set its identifier to the empty string "", so unless there's a good reason not to I'd rather set it to "" initially too. That would make this method just fail gracefully and return NULL without the need for a g_return_val_if_fail.
Comment 3 Martin Robinson 2011-06-12 15:48:52 PDT
Comment on attachment 96876 [details]
proposed fix

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

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4882
> +    g_return_val_if_fail(priv->mainResourceIdentifier.data(), 0);
> +

Won't this also print a critical warning?
Comment 4 Naiem 2011-06-13 04:35:45 PDT
Created attachment 96944 [details]
Incorporating review comments
Comment 5 tuxator 2011-06-13 08:04:26 PDT
Created attachment 96955 [details]
update ref counts

Patch as suggested by Martin Robinson on irc.
Works nice. Does not crash anymore.
Comment 6 tuxator 2011-06-13 09:12:20 PDT
Created attachment 96960 [details]
correct version of previous patch

Sorry. I edited previous patch wrongly by mistake. This one is a proper version.
Comment 7 Martin Robinson 2011-06-13 09:56:03 PDT
Comment on attachment 96960 [details]
correct version of previous patch

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

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4837
> +    g_object_ref(webView);

Please leave a comment here explaining that we want to keep the WebKitWebView alive until we have finished loading the main resource. You should also mention that the same approach is taken in the Mac port.

> Source/WebKit/gtk/webkit/webkitwebview.cpp:4855
>          priv->mainResource = 0;
>      } else
>        g_hash_table_remove(priv->subResources.get(), identifier);
> +
> +    g_object_unref(webView);

Here you should only unref the WebView if the main resource is done loading.
Comment 8 Martin Robinson 2015-05-07 17:18:51 PDT
WebKit1 is no longer around, so I'm going to close this one. Sorry that we weren't able to incorporate it.