Bug 112593 - Merge MainResourceLoader::responseReceived into DocumentLoader
Summary: Merge MainResourceLoader::responseReceived into DocumentLoader
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nate Chapin
URL:
Keywords:
Depends on:
Blocks: 104969 112745 112765
  Show dependency treegraph
 
Reported: 2013-03-18 10:51 PDT by Nate Chapin
Modified: 2013-03-19 18:17 PDT (History)
5 users (show)

See Also:


Attachments
patch (26.69 KB, patch)
2013-03-18 10:59 PDT, Nate Chapin
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
attempted fix to test failure (26.53 KB, patch)
2013-03-18 12:56 PDT, Nate Chapin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Nate Chapin 2013-03-18 10:51:57 PDT
...as well as content policy handling, which is started within responseReceived().
Comment 1 Nate Chapin 2013-03-18 10:59:03 PDT
Created attachment 193605 [details]
patch
Comment 2 WebKit Review Bot 2013-03-18 12:40:39 PDT
Comment on attachment 193605 [details]
patch

Attachment 193605 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://webkit-commit-queue.appspot.com/results/17205560

New failing tests:
platform/chromium/http/tests/security/mixedContent/insecure-iframe-in-main-frame-blocked.html
http/tests/security/contentSecurityPolicy/1.1/form-action-src-blocked.html
http/tests/security/contentSecurityPolicy/1.1/form-action-src-get-blocked.html
Comment 3 Nate Chapin 2013-03-18 12:56:09 PDT
Created attachment 193634 [details]
attempted fix to test failure
Comment 4 Adam Barth 2013-03-18 14:40:47 PDT
Comment on attachment 193634 [details]
attempted fix to test failure

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

> Source/WebCore/loader/MainResourceLoader.cpp:-327
> -    ref(); // balanced by deref in continueAfterContentPolicy and cancel

Why don't you need these manual ref counts anymore?
Comment 5 Nate Chapin 2013-03-18 14:48:19 PDT
(In reply to comment #4)
> (From update of attachment 193634 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=193634&action=review
> 
> > Source/WebCore/loader/MainResourceLoader.cpp:-327
> > -    ref(); // balanced by deref in continueAfterContentPolicy and cancel
> 
> Why don't you need these manual ref counts anymore?

I don't fully understand why they were there in the first place (they date back to the initial "make the loader platform-independent" effort). I can't find any other place we have raw ref/deref calls to protect an object during policy callbacks. Also, as far as I can tell, DocumentLoader is a little tougher to assassinate than MainResourceLoader.

My best guess is that these are already vestigial, but it's hard to prove.
Comment 6 Adam Barth 2013-03-18 17:44:25 PDT
Comment on attachment 193634 [details]
attempted fix to test failure

Ok.  That's the part of this change that seems the scariest, but maybe I've got my rose colored glasses on today.  ;)
Comment 7 WebKit Review Bot 2013-03-19 10:10:38 PDT
Comment on attachment 193634 [details]
attempted fix to test failure

Clearing flags on attachment: 193634

Committed r146216: <http://trac.webkit.org/changeset/146216>
Comment 8 WebKit Review Bot 2013-03-19 10:10:42 PDT
All reviewed patches have been landed.  Closing bug.