Bug 113616 - Crash when "willSendRequest" causes the ResourceLoader to be cancelled.
Summary: Crash when "willSendRequest" causes the ResourceLoader to be cancelled.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2013-03-29 15:47 PDT by Brady Eidson
Modified: 2013-03-29 16:00 PDT (History)
1 user (show)

See Also:


Attachments
Patch v1 (5.24 KB, patch)
2013-03-29 15:53 PDT, Brady Eidson
ap: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2013-03-29 15:47:34 PDT
Crash when "willSendRequest" causes the ResourceLoader to be cancelled.

This can happen a lot of ways.  One example is with the url http://www.rackspace.com/blog/mathematics-cannot-be-patented-case-dismissed/ where an XHR fails a redirect check and therefore the loader is cancelled.

When the WebCore loader is cancelled, the WebResourceLoader is destroyed but then it tries to access freed objects.

This was only introduced with the asynchronous ResourceHandleClient model so it's very new.

By adding a RefPtr<> protector we avoid the crash completely.  By adding a "detachFromCoreLoader" to call when the WebCore loader is aborting we can avoid unnecessary messaging back to the NetworkProcess.
Comment 1 Brady Eidson 2013-03-29 15:53:22 PDT
Created attachment 195811 [details]
Patch v1
Comment 2 Alexey Proskuryakov 2013-03-29 15:57:23 PDT
Comment on attachment 195811 [details]
Patch v1

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

> Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:161
> +    ASSERT(loader);
> +    if (!loader)
> +        return;

How strongly do you feel about this? We usually do not not add such checks without knowing that this actually happens, and waiting for reproducible cases.
Comment 3 Brady Eidson 2013-03-29 15:59:01 PDT
(In reply to comment #2)
> (From update of attachment 195811 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=195811&action=review
> 
> > Source/WebKit2/WebProcess/Network/WebResourceLoadScheduler.cpp:161
> > +    ASSERT(loader);
> > +    if (!loader)
> > +        return;
> 
> How strongly do you feel about this? We usually do not not add such checks without knowing that this actually happens, and waiting for reproducible cases.

I don't feel strongly about it, I'll remove it. (leaving the ASSERT)
Comment 4 Brady Eidson 2013-03-29 16:00:58 PDT
http://trac.webkit.org/changeset/147257