Bug 112334 - Reset all clients on WebPage close
Summary: Reset all clients on WebPage close
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 115509 (view as bug list)
Depends on:
Blocks:
 
Reported: 2013-03-14 02:30 PDT by Xan Lopez
Modified: 2013-05-27 08:42 PDT (History)
8 users (show)

See Also:


Attachments
0001-Reset-all-clients-on-WebPage-close.patch (2.44 KB, patch)
2013-03-14 02:32 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Xan Lopez 2013-03-14 02:30:02 PDT
There's a small chance that someone will call one of the clients WebPage holds when the page has already been destructed, since the pointers are valid after ::close(). This won't work and will (at least in the GTK+ port) cause a bunch of warnings. To avoid this situation just reset all clients to null, like the WebPageProxy class does.
Comment 1 Xan Lopez 2013-03-14 02:32:08 PDT
Created attachment 193094 [details]
0001-Reset-all-clients-on-WebPage-close.patch
Comment 2 Xan Lopez 2013-04-19 02:35:29 PDT
Ping about this?
Comment 3 Sergio Villar Senin 2013-05-06 01:09:44 PDT
*** Bug 115509 has been marked as a duplicate of this bug. ***
Comment 4 Sergio Villar Senin 2013-05-23 03:55:25 PDT
(In reply to comment #0)
> There's a small chance that someone will call one of the clients WebPage holds when the page has already been destructed, since the pointers are valid after ::close(). This won't work and will (at least in the GTK+ port) cause a bunch of warnings. To avoid this situation just reset all clients to null, like the WebPageProxy class does.

Actually there is not only a small chance but I found an scenario where not reseting the clients causes a crash 100% of the times.

So imagine that a webpage has some javascript that executes a XmlHttpRequest in the onunload() event handler. The ::close() method of the WebPage calls injectedBundle->willDestroyPage(). That will likely be used by the WKBundlePageResourceLoadClients to free their resources, including the clientInfo they have passed to the WKBundlePageSetResourceLoadClient.

In the same ::close() method, we find a call to  m_mainFrame->coreFrame()->loader()->detachFromParent() which will trigger the execution of the XMLHttpRequest, which will issue a willSendRequest among other notifications. As the client is still set, the method will be called but it will likely try to use client's already freed resources leading to crashes.

So yeah I big +1 for this change :)
Comment 5 Xan Lopez 2013-05-23 04:52:28 PDT
If it's possible to make a layout test for this crasher I bet this patch would land a lot faster ;)
Comment 6 Alejandro G. Castro 2013-05-27 00:54:03 PDT
Fixes the issue for me, it was easy to reproduce just closing a tab while loading. We should add this to the release today.
Comment 7 Xan Lopez 2013-05-27 08:20:07 PDT
Comment on attachment 193094 [details]
0001-Reset-all-clients-on-WebPage-close.patch

Thanks. Let's see if cq can still land it.
Comment 8 WebKit Commit Bot 2013-05-27 08:42:45 PDT
Comment on attachment 193094 [details]
0001-Reset-all-clients-on-WebPage-close.patch

Clearing flags on attachment: 193094

Committed r150758: <http://trac.webkit.org/changeset/150758>
Comment 9 WebKit Commit Bot 2013-05-27 08:42:48 PDT
All reviewed patches have been landed.  Closing bug.