Bug 49246 – Main resource should be cached in the memory cache

• Component: Page Loading
• Status: RESOLVED FIXED

[Antti Koivisto]

Currently the document main resource is loaded using a ResourceLoader directly while the subresources are loaded through the memory cache. There are several reasons why it would be good to also load the main resource through the memory cache.

- Less reliance on the networking layer level cache. This might for example allow disabling the networking layer level memory caching, saving memory.
- Opens up new optimization possibilities (for example caching processed forms like token streams).
- Utilize the loader level HTTP revalidation mechanism for the main resource too.
- More unified loading mechanism may be less bug-prone and easier to understand.
- Would allow main resources participate in the (hypothetical) WebCore level persistent caching.

[Antti Koivisto]

Created attachment 75022 [details]
current patch

Current patch.

With this approach the MainResourceLoader is not needed at all anymore. This will make it possible to also eliminate SubresourceLoader (by merging it with the super class) and possibly eventually eliminate ResourceLoaders completely (in favor of using ResourceHandles directly).

There is still quite a bit of missing stuff especially for app cache, inspector and web archive support. 

General web browsing mostly works. Pass rate for HTTP layout tests is 86% with most failures in app cache, inspector, back-forward navigation and security tests.

[Darin Adler]

<rdar://problem/6497260>

[Nate Chapin]

Created attachment 88831 [details]
Work in progress - April 8

I took Antti's patch and have been messing around with it on and off over the last several weeks.

This patch fails about 200 tests on chromium-win (including the non-http tests). Interestingly, a bunch of the tests pass when run manually, but fail in DRT because of timing changes. I'm not sure how many of these are acceptable and how many we should consider regressions.

There's probably a separate cleanup patch that can be extracted from the SubresourceLoader/CachedResourceRequest pieces of this to centralize our multipart content handling.

[Antti Koivisto]

Perhaps we should do subframes only as the first step. That should reduce the API related complexity. It would allow preloader to init subframe loads which should be a significant win (especially since you can then preload scan the received html too).

[Tony Gentilcore]

(In reply to comment #4)
> Perhaps we should do subframes only as the first step. That should reduce the API related complexity. It would allow preloader to init subframe loads which should be a significant win (especially since you can then preload scan the received html too).

I believe simonjam has been thinking about preloading subframes as well.

[Xianzhu Wang]

Antti and Nate, are you still working on this bug?

Besides the benefits listed, fixing this bug seems it could also save memory that is used to hold the main resource data in DocumentLoader.

[Xianzhu Wang]

Created attachment 106086 [details]
patch that can be applied on ToT

FYI. Resolved conflicts and made it compile.

[WebKit Review Bot]

Attachment 106086 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/WebCore.exp.in', u'Source/W..." exit_code: 1

Source/WebCore/loader/cache/CachedMainResource.cpp:43:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/loader/cache/CachedMainResource.cpp:83:  One line control clauses should not use braces.  [whitespace/braces] [4]
Source/WebCore/loader/cache/CachedResourceLoader.cpp:417:  Should have a space between // and comment  [whitespace/comments] [4]
Source/WebCore/loader/DocumentLoader.h:273:  The parameter name "resource" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/loader/DocumentLoader.cpp:57:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/loader/DocumentLoader.cpp:58:  Alphabetical sorting problem.  [build/include_order] [4]
Source/WebCore/loader/DocumentLoader.cpp:291:  Line contains only semicolon. If this should be an empty statement, use { } instead.  [whitespace/semicolon] [5]
Source/WebCore/loader/cache/CachedResource.cpp:56:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 8 in 30 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nate Chapin]

Created attachment 120066 [details]
New attempt - see comment #9 for details

The attached patch has undergone no cleanup, is largely uncommented, and has about a dozen ugly hacks, but passes all but ~10 layout tests (which should mostly be tests where callbacks were reordered in benign ways).

Some notes:
* This patch routes main resource loads through CachedRawResource, rather than a new CachedMainResource type.  I'm not sure whether this is sane or whether we should copy CachedRawResource over and use it as a starting point for a CachedMainResource, but the logic is close enough that I'd lean toward just using CachedRawResource.
* MainResourceLoader is now a CachedResourceClient and should be renamed.  MainResourceLoader is the main part of this patch that needs a lot of cleanup, as well as a thorough inspection for now-dead code.
* It's possible DocumentLoader and MainResourceLoader could be merged, as in previous works-in-progress.
* This patch does NOT use a ENABLE(CACHED_MAIN_RESOURCE) flag.
* I haven't spent any time actually browsing with a debug build including this patch, so no guarantees that it won't assert all over the place.

[Nate Chapin]

Created attachment 142571 [details]
WIP - May 17, 2012

Attached is my current work in progress.

I think I've got something that is relatively sane.  I'm going to try to start breaking this into pieces and get some patches landed on trunk over the coming weeks.

[WebKit Review Bot]

Attachment 142571 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/fast/loader/subframe-removes-i..." exit_code: 1
Source/WebCore/loader/MainResourceLoader.cpp:640:  Use 0 or null instead of NULL (even in *comments*).  [readability/null] [4]
Source/WebCore/loader/cache/CachedResource.cpp:58:  A case label should not be indented, but line up with its switch statement.  [whitespace/indent] [4]
Total errors found: 2 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[WebKit Review Bot]

Comment on attachment 142571 [details]
WIP - May 17, 2012

Attachment 142571 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12727226

New failing tests:
svg/dynamic-updates/SVGUseElement-dom-href1-attr.html

[WebKit Review Bot]

Created attachment 142622 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Nate Chapin]

Created attachment 169636 [details]
WIP - Oct 19

[Nate Chapin]

Created attachment 171043 [details]
Reviewable Attempt #1

[WebKit Review Bot]

Comment on attachment 171043 [details]
Reviewable Attempt #1

Attachment 171043 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14609746

New failing tests:
http/tests/misc/will-send-request-returns-null-on-redirect.html
svg/dynamic-updates/SVGUseElement-dom-href1-attr.html
http/tests/navigation/new-window-redirect-history.html
svg/text/textpath-reference-crash.html
fast/replaced/border-radius-clip-content-edge.html
fast/forms/access-key.html

[Brady Eidson]

Comment on attachment 171043 [details]
Reviewable Attempt #1

Do you still want review on this, even though there's failing layout tests?

[Nate Chapin]

Comment on attachment 171043 [details]
Reviewable Attempt #1

No, sorry, I'm working on fixing this and will upload a new patch sometime this week hopefully.

[Brady Eidson]

Any update?

[Nate Chapin]

(In reply to comment #19)
> Any update?

I've been focusing on https://bugs.webkit.org/show_bug.cgi?id=101512, once I get that re-landed, I'll be back to working on this.

[Maciej Stachowiak]

I asked Antti to look into this too if he has time - he might be able to help finish it.

[Brady Eidson]

I'm taking a look at this as of this morning.

[Brady Eidson]

There's three classes of changes in the layouttest results:

1 - WebInspector resource counting changes.  

These seem noncontroversial.

2 - A reordering of didReceiveServerRedirectForProvisionalLoadForFrame and willSendRequest.  

This is due to subtle interaction between the loading layer and DocumentLoader and happens because DocumentLoader is - bizarrely - in charge of calling dispatchDidReceiveServerRedirectForProvisionalLoadForFrame.

The reordering is most definitely *NOT* desirable, and fixing up who actually does the dispatchDidReceiveServerRedirectForProvisionalLoadForFrame seems noncontroversal.'

3 - about:blank callbacks not getting sent out.

In the X-Frame-Options cases the failed iframe navigation has the fallback behavior of loading about:blank into the frame.  This load is scheduled.  The code that schedules it *is* getting hit with the patch but the fact that we don't see the output implies that LayoutTestController is not waiting around for the navigation to happen.  

This is probably due to subtle change in checkLoadComplete() at some level.

I haven't had a chance to fully vet the http/tests/loading/redirect-methods case yet.

[Vsevolod Vlasov]

(In reply to comment #23)
> There's three classes of changes in the layouttest results:
> 
> 1 - WebInspector resource counting changes.  
> 
> These seem noncontroversial.
I'm not sure these changes are correct, to me this looks like network panel is missing main resource request now. Nate, could you please check that?

[Brady Eidson]

(In reply to comment #24)
> (In reply to comment #23)
> > There's three classes of changes in the layouttest results:
> > 
> > 1 - WebInspector resource counting changes.  
> > 
> > These seem noncontroversial.
> I'm not sure these changes are correct, to me this looks like network panel is missing main resource request now. Nate, could you please check that?

I was operating on the premise Nate had actually tested the patch in browser and this was just a regression test change.  I'll take a look.

[Antti Koivisto]

(In reply to comment #23)
> 2 - A reordering of didReceiveServerRedirectForProvisionalLoadForFrame and willSendRequest.  
> 3 - about:blank callbacks not getting sent out.

I wonder if we should just disable the main resource caching with WebKit1 clients on Mac? So much of the internal behavior is exposed to the clients there. Having very different behavior with WebKit1 and WebKit2 will be annoying too though.

[Maciej Stachowiak]

(In reply to comment #26)
> (In reply to comment #23)
> > 2 - A reordering of didReceiveServerRedirectForProvisionalLoadForFrame and willSendRequest.  
> > 3 - about:blank callbacks not getting sent out.
> 
> I wonder if we should just disable the main resource caching with WebKit1 clients on Mac? So much of the internal behavior is exposed to the clients there. Having very different behavior with WebKit1 and WebKit2 will be annoying too though.

WK2 has callbacks equivalent to (2) above, and it would probably be bad to reverse their order.

[Antti Koivisto]

(In reply to comment #27)
> WK2 has callbacks equivalent to (2) above, and it would probably be bad to reverse their order.

With WK2 it would be easier to fix all clients to not depend on details like this (I don't know if the new order is actually wrong or not).

[Brady Eidson]

(In reply to comment #26)
> (In reply to comment #23)
> > 2 - A reordering of didReceiveServerRedirectForProvisionalLoadForFrame and willSendRequest.  
> > 3 - about:blank callbacks not getting sent out.
> 
> I wonder if we should just disable the main resource caching with WebKit1 clients on Mac? So much of the internal behavior is exposed to the clients there. Having very different behavior with WebKit1 and WebKit2 will be annoying too though.

I didn't raise these to proclaim there's humongous problems with the patch.  I actually have a precise understanding of what is causing each of these and both should be fairly simple to resolve.

I'm disposed with unpredicted personal business for the next few days but hope to fix them soon.

[Nate Chapin]

(In reply to comment #23)
> There's three classes of changes in the layouttest results:
> 
> 1 - WebInspector resource counting changes.

I'm still looking into this, will report back soon.

> 
> 2 - A reordering of didReceiveServerRedirectForProvisionalLoadForFrame and willSendRequest.  

I agree that this is trivial. The only potentially controversial part is that, given the architecture in this patch, the simplest place to put the call to didReceiveServerRedirectForProvisionalLoadForFrame is in SubresourceLoader::willSendRequest(). I personally don't think this is a problem, but it's a little weird.

I guess the alternative is to add a new CachedRawResourceClient callback and make the call in MainResourceLoader.

> 
> 3 - about:blank callbacks not getting sent out.
> 
> In the X-Frame-Options cases the failed iframe navigation has the fallback behavior of loading about:blank into the frame.  This load is scheduled.  The code that schedules it *is* getting hit with the patch but the fact that we don't see the output implies that LayoutTestController is not waiting around for the navigation to happen.  

All of the about:blank callbacks disappearing are the result of how MainResourceLoader handled empty loads in https://bugs.webkit.org/attachment.cgi?id=171043. Because a ResourceLoader isn't being created for these loads, ResourceLoadNotifier callbacks aren't being sent. I ended up making those changes to empty loads as part of https://bugs.webkit.org/show_bug.cgi?id=101512, so these diffs shouldn't be part of a new version of this patch.

In the case of the X-Frame-Options loads, I believe the tests would be timing out if we were not performing the about:blank fallback load. Each test uses the following function as its load event for the iframe:

function checkIfDone()
{
    if (document.getElementsByTagName("iframe")[0].contentWindow.location == "about:blank")
        testRunner.notifyDone();
}

This shouldn't return true until the empty document is loaded and has replaced the blocked document.


The changes http/tests/loading/redirect-methods.html are a combination of (2) and (3). The portion caused by (3) are landed in https://bugs.webkit.org/show_bug.cgi?id=101512, and the portion caused by (2) will be unnecessary with the changes to didReceiveServerRedirectForProvisionalLoadForFrame timing.

[Brady Eidson]

(In reply to comment #30)
> (In reply to comment #23)

> > 
> > 3 - about:blank callbacks not getting sent out.
> > 
> > In the X-Frame-Options cases the failed iframe navigation has the fallback behavior of loading about:blank into the frame.  This load is scheduled.  The code that schedules it *is* getting hit with the patch but the fact that we don't see the output implies that LayoutTestController is not waiting around for the navigation to happen.  
> 
> All of the about:blank callbacks disappearing are the result of how MainResourceLoader handled empty loads in https://bugs.webkit.org/attachment.cgi?id=171043. Because a ResourceLoader isn't being created for these loads, ResourceLoadNotifier callbacks aren't being sent. I ended up making those changes to empty loads as part of https://bugs.webkit.org/show_bug.cgi?id=101512, so these diffs shouldn't be part of a new version of this patch.

A little fuzzy now on no sleep.  I think what you're saying is that if 101512 lands (and sticks) then these callbacks start coming through?

> In the case of the X-Frame-Options loads, I believe the tests would be timing out if we were not performing the about:blank fallback load. Each test uses the following function as its load event for the iframe:
> 
> function checkIfDone()
> {
>     if (document.getElementsByTagName("iframe")[0].contentWindow.location == "about:blank")
>         testRunner.notifyDone();
> }
> 
> This shouldn't return true until the empty document is loaded and has replaced the blocked document.
> 
> 
> The changes http/tests/loading/redirect-methods.html are a combination of (2) and (3). The portion caused by (3) are landed in https://bugs.webkit.org/show_bug.cgi?id=101512, and the portion caused by (2) will be unnecessary with the changes to didReceiveServerRedirectForProvisionalLoadForFrame timing.

This further supports my "what I think you're saying is..." above.

[Nate Chapin]

(In reply to comment #31)
> (In reply to comment #30)
> > (In reply to comment #23)
> 
> > > 
> > > 3 - about:blank callbacks not getting sent out.
> > > 
> > > In the X-Frame-Options cases the failed iframe navigation has the fallback behavior of loading about:blank into the frame.  This load is scheduled.  The code that schedules it *is* getting hit with the patch but the fact that we don't see the output implies that LayoutTestController is not waiting around for the navigation to happen.  
> > 
> > All of the about:blank callbacks disappearing are the result of how MainResourceLoader handled empty loads in https://bugs.webkit.org/attachment.cgi?id=171043. Because a ResourceLoader isn't being created for these loads, ResourceLoadNotifier callbacks aren't being sent. I ended up making those changes to empty loads as part of https://bugs.webkit.org/show_bug.cgi?id=101512, so these diffs shouldn't be part of a new version of this patch.
> 
> A little fuzzy now on no sleep.  I think what you're saying is that if 101512 lands (and sticks) then these callbacks start coming through?
> 
> > In the case of the X-Frame-Options loads, I believe the tests would be timing out if we were not performing the about:blank fallback load. Each test uses the following function as its load event for the iframe:
> > 
> > function checkIfDone()
> > {
> >     if (document.getElementsByTagName("iframe")[0].contentWindow.location == "about:blank")
> >         testRunner.notifyDone();
> > }
> > 
> > This shouldn't return true until the empty document is loaded and has replaced the blocked document.
> > 
> > 
> > The changes http/tests/loading/redirect-methods.html are a combination of (2) and (3). The portion caused by (3) are landed in https://bugs.webkit.org/show_bug.cgi?id=101512, and the portion caused by (2) will be unnecessary with the changes to didReceiveServerRedirectForProvisionalLoadForFrame timing.
> 
> This further supports my "what I think you're saying is..." above.

https://bugs.webkit.org/show_bug.cgi?id=101512 landed, we'll see whether it sticks...

I believe that, with https://bugs.webkit.org/show_bug.cgi?id=101512 on trunk, the callbacks are gone and won't be coming back. My point was that the layout test result changes occurred in that patch, and they shouldn't be necessary in this bug.

Does that make sense?

[Nate Chapin]

(In reply to comment #25)
> (In reply to comment #24)
> > (In reply to comment #23)
> > > There's three classes of changes in the layouttest results:
> > > 
> > > 1 - WebInspector resource counting changes.  
> > > 
> > > These seem noncontroversial.
> > I'm not sure these changes are correct, to me this looks like network panel is missing main resource request now. Nate, could you please check that?
> 
> I was operating on the premise Nate had actually tested the patch in browser and this was just a regression test change.  I'll take a look.

I just tested it with my patch, and see the main resource on both the Resources and Network tabs. Is there anywhere else I need to check?

[Nate Chapin]

Created attachment 176599 [details]
Merged to trunk, addressed server redirect concerns, etc

I'm still running a broader group of tests, so not marking for review yet.

[WebKit Review Bot]

Comment on attachment 176599 [details]
Merged to trunk, addressed server redirect concerns, etc

Attachment 176599 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15015665

New failing tests:
fast/loader/onload-willSendRequest-null-for-frame.html
plugins/plugin-document-willSendRequest-null.html
fast/replaced/border-radius-clip-content-edge.html
fast/loader/javascript-url-iframe-remove-on-navigate.html

[Build Bot]

Comment on attachment 176599 [details]
Merged to trunk, addressed server redirect concerns, etc

Attachment 176599 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15013693

New failing tests:
svg/foreignObject/viewport-foreignobject-crash.html

[Nate Chapin]

Created attachment 176829 [details]
Fix EWS failures

[Darin Adler]

Comment on attachment 176829 [details]
Fix EWS failures

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

> Source/WebCore/loader/MainResourceLoader.h:33
> +#include "Frame.h"

Is this include unavoidable? Is it just so we can inline the frameLoader() function?

> Source/WebCore/loader/MainResourceLoader.h:75
> +    unsigned long identifier() const;

Strange choice of type here. WebKit is used on platforms where unsigned and unsigned long are the same size, so we almost never use unsigned long. Is there some reason to use it here?

> Source/WebCore/loader/MainResourceLoader.h:86
> +    virtual void redirectReceived(CachedResource*, ResourceRequest&, const ResourceResponse&);
> +    virtual void responseReceived(CachedResource*, const ResourceResponse&);
> +    virtual void dataReceived(CachedResource*, const char* data, int dataLength);
> +    virtual void notifyFinished(CachedResource*);

Why no OVERRIDE on these?

> Source/WebCore/loader/MainResourceLoader.h:125
> +    RefPtr<Frame> m_frame;
> +    RefPtr<DocumentLoader> m_documentLoader;

Adding these seems really unfortunate. Is this really needed?

> Source/WebCore/loader/MainResourceLoader.cpp:-381
> -    // There is a bug in CFNetwork where callbacks can be dispatched even when loads are deferred.
> -    // See <rdar://problem/6304600> for more details.
> -#if !USE(CF)
> -    ASSERT(!defersLoading());
> -#endif

Where did this assertion go?

> Source/WebCore/loader/MainResourceLoader.cpp:-496
> -    // There is a bug in CFNetwork where callbacks can be dispatched even when loads are deferred.
> -    // See <rdar://problem/6304600> for more details.
> -#if !USE(CF)
> -    ASSERT(!defersLoading() || InspectorInstrumentation::isDebuggerPaused(m_frame.get()));
> -#endif

Where did this assertion go?

> Source/WebCore/loader/MainResourceLoader.cpp:-542
> -    // There is a bug in CFNetwork where callbacks can be dispatched even when loads are deferred.
> -    // See <rdar://problem/6304600> for more details.
> -#if !USE(CF)
> -    ASSERT(!defersLoading());
> -#endif

Where did this assertion go?

> Source/WebCore/loader/archive/Archive.h:57
> +    void addSubresource(PassRefPtr<ArchiveResource> subResource);

Should remove the (miscapitalized) argument name.

> Source/WebCore/loader/archive/Archive.cpp:38
> +void Archive::addSubresource(PassRefPtr<ArchiveResource> subResource)

The word “subresource” should not have a capital letter in it.

[Darin Adler]

Comment on attachment 176829 [details]
Fix EWS failures

Can this be broken into smaller patches? I don’t think all these changes necessarily go together.

[Darin Adler]

Comment on attachment 176829 [details]
Fix EWS failures

Thanks very much for taking this on; there are things here that generally look good. I am a bit puzzled by the overall direction, though.

[WebKit Review Bot]

Comment on attachment 176829 [details]
Fix EWS failures

Attachment 176829 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15056150

New failing tests:
fast/replaced/border-radius-clip-content-edge.html

[Brady Eidson]

(In reply to comment #32)
> (In reply to comment #31)
> 
> https://bugs.webkit.org/show_bug.cgi?id=101512 landed, we'll see whether it sticks...
> 
> I believe that, with https://bugs.webkit.org/show_bug.cgi?id=101512 on trunk, the callbacks are gone and won't be coming back. My point was that the layout test result changes occurred in that patch, and they shouldn't be necessary in this bug.
> 
> Does that make sense?

I see.  I wasn't following 101512 as closely as I could have/should have.  Removing the about:blank callbacks might be problematic for WebKit apps.  Was any sort of archeological effort made through their history to determine that the about:blank behavior was not actually important?

If we didn't need them, then we didn't need them.  But in the past even the most subtle changes to loading delegate callbacks have rendered high visibility WebKit applications broken.

[Brady Eidson]

Comment on attachment 176829 [details]
Fix EWS failures

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

>> Source/WebCore/loader/MainResourceLoader.h:75
>> +    unsigned long identifier() const;
> 
> Strange choice of type here. WebKit is used on platforms where unsigned and unsigned long are the same size, so we almost never use unsigned long. Is there some reason to use it here?

Currently all "loading identifiers" in WebCore are "unsigned long".  The includes the ResourceLoader and WebInspector machineries in addition to FrameLoaderClient callbacks.

This actually drives me crazy and has been a thorn in my side recently.  I'd strongly prefer to use uint64_t as at least some users strongly expect greater than 32-bit fidelity.

I started to work on a patch to replace it all, but it turned in to a *much* larger task than I had time for.

My point here is that Nate is simply following a *large* amount of momentum with this choice and I'm perfectly okay with it.

[Build Bot]

Comment on attachment 176829 [details]
Fix EWS failures

Attachment 176829 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15036854

New failing tests:
svg/foreignObject/viewport-foreignobject-crash.html

[Nate Chapin]

(In reply to comment #42)
> (In reply to comment #32)
> > (In reply to comment #31)
> > 
> > https://bugs.webkit.org/show_bug.cgi?id=101512 landed, we'll see whether it sticks...
> > 
> > I believe that, with https://bugs.webkit.org/show_bug.cgi?id=101512 on trunk, the callbacks are gone and won't be coming back. My point was that the layout test result changes occurred in that patch, and they shouldn't be necessary in this bug.
> > 
> > Does that make sense?
> 
> I see.  I wasn't following 101512 as closely as I could have/should have.  Removing the about:blank callbacks might be problematic for WebKit apps.  Was any sort of archeological effort made through their history to determine that the about:blank behavior was not actually important?
> 
> If we didn't need them, then we didn't need them.  But in the past even the most subtle changes to loading delegate callbacks have rendered high visibility WebKit applications broken.

I didn't do a ton of archaeology, based on https://bugs.webkit.org/show_bug.cgi?id=101512#c10. I'd be happy to do so now if you have pointers as to where I should be looking.

[Nate Chapin]

(In reply to comment #39)
> (From update of attachment 176829 [details])
> Can this be broken into smaller patches? I don’t think all these changes necessarily go together.

I've already broken off several pieces (see the blocking bugs), but I can certainly try to find other pieces to break off.

(In reply to comment #38)
> (From update of attachment 176829 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=176829&action=review
> 
> > Source/WebCore/loader/MainResourceLoader.h:33
> > +#include "Frame.h"
> 
> Is this include unavoidable? Is it just so we can inline the frameLoader() function?

I think it is just for the inline, will fix.

> 
> > Source/WebCore/loader/MainResourceLoader.h:125
> > +    RefPtr<Frame> m_frame;
> > +    RefPtr<DocumentLoader> m_documentLoader;
> 
> Adding these seems really unfortunate. Is this really needed?

It's because MainResourceLoader no longer inherits from ResourceLoader. Previously, these were members of ResourceLoader. We really need at least m_documentLoader, but we could probably do without m_frame and just use m_documentLoader->frame() if you think that's cleaner.

> 
> > Source/WebCore/loader/MainResourceLoader.cpp:-381
> > -    // There is a bug in CFNetwork where callbacks can be dispatched even when loads are deferred.
> > -    // See <rdar://problem/6304600> for more details.
> > -#if !USE(CF)
> > -    ASSERT(!defersLoading());
> > -#endif
> 
> Where did this assertion go?

For these, I was working on the premise that there is no longer a defersLoading() on this class, it was by definition always false. Perhaps I should add back in defersLoading(), and have it reach into the underlying ResourceLoader to approximate the current behavior.

[Nate Chapin]

Created attachment 177046 [details]
Address darin's comments

I've got a split-out subset that I'm still testing, will post it separately when it's ready.

[WebKit Review Bot]

Comment on attachment 177046 [details]
Address darin's comments

Attachment 177046 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15086592

New failing tests:
fast/replaced/border-radius-clip-content-edge.html

[Darin Adler]

(In reply to comment #46)
> It's because MainResourceLoader no longer inherits from ResourceLoader. Previously, these were members of ResourceLoader. We really need at least m_documentLoader, but we could probably do without m_frame and just use m_documentLoader->frame() if you think that's cleaner.

I do.

> For these, I was working on the premise that there is no longer a defersLoading() on this class, it was by definition always false. Perhaps I should add back in defersLoading(), and have it reach into the underlying ResourceLoader to approximate the current behavior.

Here’s my take: The assertions about loading deferral are not about the internal contract of just this single class. I believe we’d want those assertions even if they had to grope through other objects to find out what the deferral status is.

[Nate Chapin]

Created attachment 177317 [details]
Merged in r136412, rebase the failing cr-linux test

The cr-linux ews results change appears to be a symptom of an unrelated off-by-one error.

[Build Bot]

Comment on attachment 177317 [details]
Merged in r136412, rebase the failing cr-linux test

Attachment 177317 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15101893

New failing tests:
svg/foreignObject/viewport-foreignobject-crash.html

[Nate Chapin]

Created attachment 177515 [details]
Attempt of the day, Dec 4.

[Adam Barth]

Comment on attachment 177515 [details]
Attempt of the day, Dec 4.

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

> Source/WebCore/loader/DocumentLoader.cpp:815
>      m_subresourceLoaders.add(loader);

Should we ASSERT that loader isn't in m_subresourceLoaders?

We might also want to ASSERT that m_mainResourceLoader->loader() != loader given that we're trying to filter that one out.

> Source/WebCore/loader/DocumentLoader.cpp:821
> +    if (!m_subresourceLoaders.contains(loader))
> +        return;

Should we ASSERT that loader == m_mainResourceLoader->loader() in this branch?

> Source/WebCore/loader/MainResourceLoader.cpp:641
> +        CachedResourceRequest cachedResourceRequest(request, ResourceLoaderOptions(SendCallbacks, SniffContent, BufferData,
> +            AllowStoredCredentials, AskClientForCrossOriginCredentials, SkipSecurityCheck));

nit: I might have made ResourceLoaderOptions a named local variable since it's constructor is long.

> Source/WebCore/loader/MainResourceLoader.cpp:643
> +        if (!m_resource) {

When does this occur?  I guess if the CachedResourceLoader blocks the request for some sort of policy reason?

> Source/WebCore/loader/MainResourceLoader.cpp:655
> +        if (url != request.url())
> +            request.setURL(url);

Is URL the only think that can change?  It looks like url is a const reference to r.url() and request is a copy of r.  I guess request can be mutated somewhere in here?

Is it essential to use url instead of r.url()?  This appears to be the only use of |url|, so I assume you had a reason for using it.  Is it important that url is a reference rather than a copy of r.url()?  I guess what I'm saying is that I find these two lines of code confusing.

> Source/WebCore/loader/MainResourceLoader.h:54
> -class MainResourceLoader : public ResourceLoader {
> +class MainResourceLoader : public RefCounted<MainResourceLoader>, public CachedRawResourceClient {

Crazy!  I wonder if we'll want to rename MainResourceLoader in a future patch now that its not a subclass of ResourceLoader.

> Source/WebCore/loader/archive/Archive.cpp:40
> +    if (m_mainResource->url() != subresource->url())

Is it a valid assumption for archives that their sub resources never have the same URL as their main resources?  That's not a valid assumption for web sites, but I'm not sure about archives.

[Adam Barth]

Looks almost sane, which is saying something.  :)

[Nate Chapin]

(In reply to comment #53)
> (From update of attachment 177515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177515&action=review

> 
> > Source/WebCore/loader/MainResourceLoader.cpp:643
> > +        if (!m_resource) {
> 
> When does this occur?  I guess if the CachedResourceLoader blocks the request for some sort of policy reason?

Yeah, most commonly it will be because the load failed synchronously, perhaps because it was cancelled via an embedder's willSendRequest() call.

> 
> > Source/WebCore/loader/MainResourceLoader.cpp:655
> > +        if (url != request.url())
> > +            request.setURL(url);
> 
> Is URL the only think that can change?  It looks like url is a const reference to r.url() and request is a copy of r.  I guess request can be mutated somewhere in here?

The request will be modified within requestMainResource() by the embedder's willSendRequest(), by headers added in CachedResource::load(), etc. We want all those headers, but we still want the fragment identifier. Is there anything I can do to make the comment just above your snip more clear?

> 
> Is it essential to use url instead of r.url()?  This appears to be the only use of |url|, so I assume you had a reason for using it.  Is it important that url is a reference rather than a copy of r.url()?  I guess what I'm saying is that I find these two lines of code confusing.

I don't think url is necessary, I think I copy/pasted it from the old way. r should probably be renamed, too.

> 
> > Source/WebCore/loader/MainResourceLoader.h:54
> > -class MainResourceLoader : public ResourceLoader {
> > +class MainResourceLoader : public RefCounted<MainResourceLoader>, public CachedRawResourceClient {
> 
> Crazy!  I wonder if we'll want to rename MainResourceLoader in a future patch now that its not a subclass of ResourceLoader.

Possibly. I've been trying to come up with something better, but I haven't found it yet. I suppose there's also the more ambitious idea of merging it with DocumentLoader.

> 
> > Source/WebCore/loader/archive/Archive.cpp:40
> > +    if (m_mainResource->url() != subresource->url())
> 
> Is it a valid assumption for archives that their sub resources never have the same URL as their main resources?  That's not a valid assumption for web sites, but I'm not sure about archives.

I don't know for sure. If subresources can have the same url as the main resource, we don't have test coverage for it.

[Nate Chapin]

(In reply to comment #53)
> (From update of attachment 177515 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177515&action=review
> > Source/WebCore/loader/DocumentLoader.cpp:821
> > +    if (!m_subresourceLoaders.contains(loader))
> > +        return;
> 
> Should we ASSERT that loader == m_mainResourceLoader->loader() in this branch?

Turns out this doesn't work. m_mainResourceLoader is sometimes (maybe always) cleared by the time removeSubresourceLoader() is called.

(In reply to comment #55)
> (In reply to comment #53)
> > (From update of attachment 177515 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=177515&action=review
> 
> > 
> > > Source/WebCore/loader/archive/Archive.cpp:40
> > > +    if (m_mainResource->url() != subresource->url())
> > 
> > Is it a valid assumption for archives that their sub resources never have the same URL as their main resources?  That's not a valid assumption for web sites, but I'm not sure about archives.
> 
> I don't know for sure. If subresources can have the same url as the main resource, we don't have test coverage for it.

It appears that a cleaner, simpler way of preventing duplication of the main resource is to skip main resources by their CachedResource::Type in DocumentLoader::subresource(). Will do that instead.

[Nate Chapin]

Created attachment 177839 [details]
Address abarth's comments

[Adam Barth]

Comment on attachment 177839 [details]
Address abarth's comments

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

> Source/WebCore/loader/MainResourceLoader.cpp:653
> +        // We need to wait until after requestMainResource() is called to setRequest(), because there are a bunch of headers set when
> +        // the underlying ResourceLoader is created, and DocumentLoader::m_request needs to include those. However, the cache will
> +        // strip the fragment identifier (which DocumentLoader::m_request should also include), so add that back in.
> +        if (loader())
> +            request = loader()->originalRequest();
> +        if (initialRequest.url() != request.url())
> +            request.setURL(initialRequest.url());

Maybe we should ASSERT that the URLs differ only in their fragment identifier?

[Adam Barth]

I'm happy with this patch.  I don't fully understand all of its consequences, but I think we should try landing it.

[Adam Barth]

Comment on attachment 177839 [details]
Address abarth's comments

I'm not sure if other folks have feedback for you, but I'm going to mark this patch r+.  Please use you judgement as to whether you'd like other folks to look at the patch as well.

[Antti Koivisto]

Comment on attachment 177839 [details]
Address abarth's comments

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

The patch is hard to follow partly due to general messiness of the loader code and also because it is not clear what the end state looks like (since it doesn't enable caching). It is sad that we haven't made much progress in refactoring the code before attempting this. At minimum it seems to me that we at least should get the layering right.

> Source/WebCore/loader/MainResourceLoader.cpp:372
> -void MainResourceLoader::didReceiveResponse(const ResourceResponse& r)
> +void MainResourceLoader::responseReceived(CachedResource* resource, const ResourceResponse& r)

CachedResource is a higher level concept than ResourceLoader. Making the latter depend on the former seems architecturally wrong.

[Adam Barth]

Comment on attachment 177839 [details]
Address abarth's comments

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

>> Source/WebCore/loader/MainResourceLoader.cpp:372
>> +void MainResourceLoader::responseReceived(CachedResource* resource, const ResourceResponse& r)
> 
> CachedResource is a higher level concept than ResourceLoader. Making the latter depend on the former seems architecturally wrong.

I don't really understand this comment.  MainResourceLoader is no longer a subclass of ResourceLoader, so I don't think there is any dependency being added from ResourceLoader to CachedResource.

[Nate Chapin]

(In reply to comment #61)
> (From update of attachment 177839 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=177839&action=review
> 
> The patch is hard to follow partly due to general messiness of the loader code and also because it is not clear what the end state looks like (since it doesn't enable caching). It is sad that we haven't made much progress in refactoring the code before attempting this. At minimum it seems to me that we at least should get the layering right.

I haven't written the patch to turn caching on yet, but in theory it should be as simple as removing the if statement at http://trac.webkit.org/browser/trunk/Source/WebCore/loader/cache/CachedResourceLoader.cpp?rev=136861#L545 .

In practice, there may be subtle issues to work out, but given that main resources will reuse the plumbing for cached XHRs, I'm optimistic that the issues will be minimal.

> 
> > Source/WebCore/loader/MainResourceLoader.cpp:372
> > -void MainResourceLoader::didReceiveResponse(const ResourceResponse& r)
> > +void MainResourceLoader::responseReceived(CachedResource* resource, const ResourceResponse& r)
> 
> CachedResource is a higher level concept than ResourceLoader. Making the latter depend on the former seems architecturally wrong.

As abarth mentioned, in this patch, MainResourceLoader is no longer a ResourceLoader. It is a subsidiary of DocumentLoader, sitting above the cache. It has been suggested in multiple places (namely comments #9 and #53) that MainResourceLoader should probably be renamed, but I haven't come up with anything suitable yet.

Have I misunderstood your concern?

[Antti Koivisto]

(In reply to comment #62)
> I don't really understand this comment.  MainResourceLoader is no longer a subclass of ResourceLoader, 

Ok. Do you know where the confusion might originate from?

[Antti Koivisto]

(In reply to comment #63)
> As abarth mentioned, in this patch, MainResourceLoader is no longer a ResourceLoader. It is a subsidiary of DocumentLoader, sitting above the cache. It has been suggested in multiple places (namely comments #9 and #53) that MainResourceLoader should probably be renamed, but I haven't come up with anything suitable yet.
> 
> Have I misunderstood your concern?

Probably not. It sounds the class really needs a new name though.

[Antti Koivisto]

Comment on attachment 177839 [details]
Address abarth's comments

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

r+'d back since this is mainly a naming concern. The current MainResourceLoader still seems like an odd type (besides the name) with ill-defined purpose so I think this is going to need more work.

> Source/WebCore/loader/MainResourceLoader.cpp:637
> +    if (m_substituteData.isValid())
> +        handleSubstituteDataLoadSoon(request);
> +    else {
> +        DEFINE_STATIC_LOCAL(ResourceLoaderOptions, mainResourceLoadOptions,

Looks like this could do early return.

[Nate Chapin]

Created attachment 178623 [details]
Patch for landing

[Nate Chapin]

(In reply to comment #67)
> Created an attachment (id=178623) [details]
> Patch for landing

The commit queue failed because of incompatibility with http://trac.webkit.org/changeset/136439. This is a simple fix, temporarily leaving resource timing off for main resources. I'll coordinate with simonjam to get it turned on once this patch has stuck.

Also, chromium asan indicates that CachedRawResource::willSendRequest() needs to protect(this), as main resources may cancel their loads re-entrantly.

[Nate Chapin]

Created attachment 178826 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 178826 [details]
Patch for landing

Clearing flags on attachment: 178826

Committed r137333: <http://trac.webkit.org/changeset/137333>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 104771

[Adam Klein]

The failure that broke the camel's back (and caused the WebKit roll to get reverted) was this one:

ExtensionResourceRequestPolicyTest.Iframe: 
ASSERTION FAILED: equalIgnoringFragmentIdentifier(initialRequest.url(), request.url())
../../third_party/WebKit/Source/WebCore/loader/MainResourceLoader.cpp(656) : void WebCore::MainResourceLoader::load(const WebCore::ResourceRequest &, const WebCore::SubstituteData &)
[3510:-1256083456:1211/172350:916009739873:WARNING:extension_protocols.cc(344)] Failed to GetPathForExtension: invalid
[3510:-1256083456:1211/172350:916009829652:WARNING:url_request_job_manager.cc(131)] Failed to map: chrome-extension://invalid/
1 0x12883e9e WebCore::MainResourceLoader::load(WebCore::ResourceRequest const&, WebCore::SubstituteData const&)
[3510:-1408298304:1211/172350:916021817738:INFO:CONSOLE(0)] "Denying load of chrome-extension://pbkkcbgdkliohhfaeefcijaghglkahja/inaccessible-iframe-contents.html. Resources must be listed in the web_accessible_resources manifest key in order to be loaded by pages outside the extension.", source: about:blank (0)
2 0x1281e972 WebCore::DocumentLoader::startLoadingMainResource()
3 0x1286293c WebCore::FrameLoader::continueLoadAfterWillSubmitForm()

See rollout codereview for more links: https://codereview.chromium.org/11548010/

[Nate Chapin]

Created attachment 179139 [details]
Re-land after revert

Differences from r137333:

* clearResource() is called in MainResourceLoader's destructor, so we don't leak m_resource
* No modifications to http/tests/inspector/resource-parameters.html
* Fragment identifier reconstruction is no longer necessary at the end of MainResourceLoader::load().

Note that until bug 104721 lands, http/tests/inspector/resource-parameters.html will fail, so EWS for this patch is not excepted to be 100% green.

[Alexey Proskuryakov]

Comment on attachment 179139 [details]
Re-land after revert

Changes look reasonable.

[WebKit Review Bot]

Comment on attachment 179139 [details]
Re-land after revert

Attachment 179139 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15314039

New failing tests:
http/tests/navigation/anchor-basic.html
http/tests/navigation/anchor-frames-same-origin.html
http/tests/history/redirect-200-refresh-2-seconds.pl
http/tests/history/redirect-js-location-assign-before-load.html
http/tests/history/redirect-js-location-2-seconds.html
http/tests/history/redirect-js-location-0-seconds.html
http/tests/history/redirect-js-location-href-2-seconds.html
http/tests/history/redirect-js-location-replace-before-load.html
http/tests/history/redirect-js-document-location-before-load.html
http/tests/history/redirect-js-location-assign-0-seconds.html
http/tests/navigation/anchor-frames.html
http/tests/history/redirect-js-document-location-2-seconds.html
http/tests/misc/will-send-request-returns-null-on-redirect.html
http/tests/history/redirect-js-form-submit-before-load.html
http/tests/history/redirect-meta-refresh-0-seconds.html
http/tests/history/redirect-js-location-href-0-seconds.html
http/tests/history/redirect-js-form-submit-2-seconds.html
http/tests/history/redirect-js-document-location-0-seconds.html
http/tests/history/redirect-js-location-href-before-load.html
http/tests/history/redirect-200-refresh-0-seconds.pl
http/tests/history/redirect-js-form-submit-0-seconds.html
http/tests/history/redirect-js-location-before-load.html
http/tests/history/redirect-meta-refresh-2-seconds.html
http/tests/css/cross-fade-reload.html
http/tests/history/redirect-js-location-replace-0-seconds.html
http/tests/navigation/anchor-goback.html
http/tests/history/redirect-js-location-replace-2-seconds.html
http/tests/inspector/resource-parameters.html
http/tests/history/redirect-js-location-assign-2-seconds.html

[Build Bot]

Comment on attachment 179139 [details]
Re-land after revert

Attachment 179139 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15317252

New failing tests:
http/tests/history/redirect-js-location-2-seconds.html
http/tests/history/redirect-200-refresh-2-seconds.pl
http/tests/history/redirect-js-location-assign-before-load.html
http/tests/history/redirect-js-location-0-seconds.html
http/tests/history/redirect-js-location-href-2-seconds.html
http/tests/history/redirect-js-location-replace-before-load.html
http/tests/history/redirect-js-document-location-before-load.html
http/tests/history/redirect-js-location-assign-0-seconds.html
http/tests/history/redirect-js-document-location-2-seconds.html
http/tests/history/redirect-js-form-submit-before-load.html
http/tests/history/redirect-meta-refresh-0-seconds.html
http/tests/history/redirect-js-location-href-0-seconds.html
http/tests/history/redirect-js-form-submit-2-seconds.html
http/tests/history/redirect-js-document-location-0-seconds.html
http/tests/history/redirect-js-location-href-before-load.html
http/tests/history/redirect-200-refresh-0-seconds.pl
http/tests/history/redirect-js-form-submit-0-seconds.html
fast/css/font-face-data-uri.html
http/tests/history/redirect-meta-refresh-2-seconds.html
http/tests/css/cross-fade-reload.html
http/tests/history/redirect-js-location-replace-0-seconds.html
http/tests/history/redirect-js-location-replace-2-seconds.html
fast/loader/javascript-url-in-object.html
http/tests/history/redirect-js-location-before-load.html
http/tests/history/redirect-js-location-assign-2-seconds.html

[Nate Chapin]

Created attachment 179293 [details]
Patch for landing

[Nate Chapin]

Created attachment 179294 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 179294 [details]
Patch for landing

Clearing flags on attachment: 179294

Committed r137607: <http://trac.webkit.org/changeset/137607>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

I'm not sure why yet but r137607 seems to have broken EFL's test_ewk2_download API test (basically, just testing a PDF file download).

[Thiago Marcos P. Santos]

(In reply to comment #82)
> I'm not sure why yet but r137607 seems to have broken EFL's test_ewk2_download API test (basically, just testing a PDF file download).

After this patch, we are getting ::cancel() on the ResourceHandleSoup. Probably GTK is broken as well. A fix was provided only for Mac at bug 105044.

[Carlos Garcia Campos]

(In reply to comment #83)
> (In reply to comment #82)
> > I'm not sure why yet but r137607 seems to have broken EFL's test_ewk2_download API test (basically, just testing a PDF file download).
> 
> After this patch, we are getting ::cancel() on the ResourceHandleSoup. Probably GTK is broken as well. A fix was provided only for Mac at bug 105044.

Yes, GTK is broken too, it seems that after this patch callbacks didCommitLoadForFrame and didFinishLoadForFrame are never called for the main resource when loaded using loadHTML(). I have to look at it in more detail, but this causes that most of our unit tests timeout because they wait forever until load finishes. We didn't notice it, because of a bug in our unit tests script, that doesn't consider timeout tests as failures, so bots are green :-(

[Carlos Garcia Campos]

(In reply to comment #84)
> (In reply to comment #83)
> > (In reply to comment #82)
> > > I'm not sure why yet but r137607 seems to have broken EFL's test_ewk2_download API test (basically, just testing a PDF file download).
> > 
> > After this patch, we are getting ::cancel() on the ResourceHandleSoup. Probably GTK is broken as well. A fix was provided only for Mac at bug 105044.
> 
> Yes, GTK is broken too, it seems that after this patch callbacks didCommitLoadForFrame and didFinishLoadForFrame are never called for the main resource when loaded using loadHTML(). I have to look at it in more detail, but this causes that most of our unit tests timeout because they wait forever until load finishes. We didn't notice it, because of a bug in our unit tests script, that doesn't consider timeout tests as failures, so bots are green :-(

After looking at it in more detail, frame loader callbacks are indeed called, the ones that are no longer called for the main resource when using substitute data are the resource loader client ones, WKPageResourceLoadClient in WebKit2  API. In WebKit2 GTK+ API we always wait for the main resource, and that's why unit tests timeout.

[Thiago Marcos P. Santos]

We have a fix candidate at bug 105044.

[Carlos Garcia Campos]

(In reply to comment #86)
> We have a fix candidate at bug 105044.

That's a bug with downloads, the bug I'm talking about is that resource load client callbacks are not called anymore for the main resource when loading with substitute data.

[Marshall Greenblatt]

Bug 107175 ([Chromium] Incorrect |isRedirect| value passed to decidePolicyForNavigation) is also related to this issue.