Move empty loading to DocumentLoader, simplify FrameLoader::init()

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Nate Chapin]

The logic for loading an empty main resource is spread across several functions in MainResourceLoader.  It can be done in about 5 lines in DocumentLoader::startLoadingMainResource(), without creating a MainResourceLoader.  As an added bonus, it makes it very simple to treat a Frame's initial empty document much more like a normal empty load, reducing code in FrameLoader::init().

The downside is that there will no longer be FrameLoaderClient calls for empty loads, but those would be going away in https://bugs.webkit.org/show_bug.cgi?id=49246 anyway.

[Nate Chapin]

Created attachment 172875 [details]
patch

[Adam Barth]

Comment on attachment 172875 [details]
patch

Wow, this patch is really cool.

[Adam Barth]

Comment on attachment 172875 [details]
patch

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

> LayoutTests/http/tests/loading/redirect-methods-expected.txt:-9
> -about:blank - willSendRequest <NSURLRequest URL about:blank, main document URL http://127.0.0.1:8000/loading/redirect-methods.html, http method GET> redirectResponse (null)
> -about:blank - didReceiveResponse <NSURLResponse about:blank, http status code 0>

Is it ok that we're dropping these callbacks?

[Nate Chapin]

(In reply to comment #3)
> (From update of attachment 172875 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=172875&action=review
> 
> > LayoutTests/http/tests/loading/redirect-methods-expected.txt:-9
> > -about:blank - willSendRequest <NSURLRequest URL about:blank, main document URL http://127.0.0.1:8000/loading/redirect-methods.html, http method GET> redirectResponse (null)
> > -about:blank - didReceiveResponse <NSURLResponse about:blank, http status code 0>
> 
> Is it ok that we're dropping these callbacks?

I haven't found any problems with it between this patch and my experiments with caching main resources, but I wouldn't call that definitive proof.

[Build Bot]

Comment on attachment 172875 [details]
patch

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

New failing tests:
fast/loader/javascript-url-iframe-remove-on-navigate.html

[WebKit Review Bot]

Comment on attachment 172875 [details]
patch

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

New failing tests:
http/tests/navigation/new-window-redirect-history.html
fast/frames/frame-limit.html

[Brady Eidson]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 172875 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=172875&action=review
> > 
> > > LayoutTests/http/tests/loading/redirect-methods-expected.txt:-9
> > > -about:blank - willSendRequest <NSURLRequest URL about:blank, main document URL http://127.0.0.1:8000/loading/redirect-methods.html, http method GET> redirectResponse (null)
> > > -about:blank - didReceiveResponse <NSURLResponse about:blank, http status code 0>
> > 
> > Is it ok that we're dropping these callbacks?
> 
> I haven't found any problems with it between this patch and my experiments with caching main resources, but I wouldn't call that definitive proof.

FWIW, we had *a LOT* of fallout from dropping callbacks for the empty document load in breaking both Safari and many 3rd party WebKit apps.

I'm not sure if this is okay, but I'm also not sure if it's not okay.

[Brady Eidson]

Is this groundwork for 49246?

[Nate Chapin]

(In reply to comment #8)
> Is this groundwork for 49246?

Partially. 49246 as currently written drops the empty load callbacks, so it'll trim down the diff.

This is a mostly separate patch that came out of studying this code carefully in writing 49246.

(In reply to comment #7)
> 
> FWIW, we had *a LOT* of fallout from dropping callbacks for the empty document load in breaking both Safari and many 3rd party WebKit apps.
> 
> I'm not sure if this is okay, but I'm also not sure if it's not okay.

Interesting. Do you have any suggestions where I should look for fallout from this patch, or references to the previous breakages?

[Brady Eidson]

(In reply to comment #9)
> (In reply to comment #8)
> (In reply to comment #7)
> > 
> > FWIW, we had *a LOT* of fallout from dropping callbacks for the empty document load in breaking both Safari and many 3rd party WebKit apps.
> > 
> > I'm not sure if this is okay, but I'm also not sure if it's not okay.
> 
> Interesting. Do you have any suggestions where I should look for fallout from this patch, or references to the previous breakages?

I asked Anders to take a look, and he remembers that the fallout we had was from getting *too many* callbacks on the empty document, not getting too few.  If that's the case, then this is potentially an improvement.

[Nate Chapin]

Created attachment 173142 [details]
fix tests

I'm not sure this patch fixes *all* the tests, so uploading without r? to ask EWS.

[Build Bot]

Comment on attachment 173142 [details]
fix tests

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

New failing tests:
fast/loader/javascript-url-iframe-remove-on-navigate.html

[WebKit Review Bot]

Comment on attachment 173142 [details]
fix tests

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

New failing tests:
fast/frames/frame-limit.html

[Adam Barth]

> fast/frames/frame-limit.html

I was not able to reproduce this failure.  I wonder if its timing-related somehow given that this test is marked as timing out in Debug.

[Nate Chapin]

Created attachment 173718 [details]
EWS experiment #2

[Build Bot]

Comment on attachment 173718 [details]
EWS experiment #2

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

New failing tests:
fast/loader/javascript-url-iframe-remove-on-navigate.html

[Nate Chapin]

Created attachment 174010 [details]
patch #2

I got the mac crash to repro some locally, and it appears it was related to willSendRequest() nulling out a ResourceRequest for a main resource.  Currently, willSendRequest() null the ResourceRequest functins exactly the same as an empty url getting passed in from the beginning.  I can think of three ways to resolve this:

1. Call willSendRequest() from startLoadingMainResource() for the initial load, rather than in MainResourceLoader.
2. Check whether willSendRequest() cancelled the load in MainResourceLoader, and fallback to empty loading in DocumentLoader.
3. Keep empty loading back to MainResourceLoader, scrapping the FrameLoader::init() cleanup.

This patch implements #2.  It seems ok, but it's not quite as clean as I was hoping.

[Adam Barth]

Comment on attachment 174010 [details]
patch #2

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

Overall, I think this patch is worth doing.  The improvement to FrameLoader::init is really compelling.  The more stuff we do in init, the more code has to be aware of its not-quite-consistent states.

> Source/WebCore/loader/MainResourceLoader.cpp:631
> +    willSendRequest(request, ResourceResponse());

I see.  This feels a bit sub-optimal.

[WebKit Review Bot]

Comment on attachment 174010 [details]
patch #2

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

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

[Nate Chapin]

Created attachment 174220 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 174220 [details]
Patch for landing

Clearing flags on attachment: 174220

Committed r134649: <http://trac.webkit.org/changeset/134649>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[WebKit Review Bot]

Re-opened since this is blocked by bug 102413

[Darin Adler]

Comment on attachment 174010 [details]
patch #2

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

>> Source/WebCore/loader/MainResourceLoader.cpp:631
>> +    willSendRequest(request, ResourceResponse());
> 
> I see.  This feels a bit sub-optimal.

Seems that with a bit of time and effort we can fix “clients expect it”.

> Source/WebCore/loader/MainResourceLoader.cpp:634
> +    // <rdar://problem/4801066>

This seems strange. Why is this URL here?

[Nate Chapin]

(In reply to comment #24)
> (From update of attachment 174010 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=174010&action=review
> 
> >> Source/WebCore/loader/MainResourceLoader.cpp:631
> >> +    willSendRequest(request, ResourceResponse());
> > 
> > I see.  This feels a bit sub-optimal.
> 
> Seems that with a bit of time and effort we can fix “clients expect it”.
> 
> > Source/WebCore/loader/MainResourceLoader.cpp:634
> > +    // <rdar://problem/4801066>
> 
> This seems strange. Why is this URL here?

I may have cargo-culted both of these comments, as I moved those code blocks via copy/paste. :)

Between this patch and bugs.webkit.org/show_bug.cgi?id=49246, I'm planning on doing quite a bit of work on MainResourceLoader in the near future. Please do let me know if you have requests.

[Nate Chapin]

Created attachment 174741 [details]
Set the url to about:blank for non-initial empty loads

Diffs from previous patch are all in DocumentLoader.cpp. This resolves the issues I saw in chromium's downstream browser_tests, and still appears to pass layout tests.

[Nate Chapin]

Created attachment 174779 [details]
Fix up some mac failures

All diffs from reverted version are still in DocumentLoader.cpp

[Adam Barth]

Do you have thoughts on Darin's comment about change clients to not expect this callback?  I think Nate mentioned to me that this call will get removed in his patch to cache main resources.

[Nate Chapin]

(In reply to comment #28)
> Do you have thoughts on Darin's comment about change clients to not expect this callback?  I think Nate mentioned to me that this call will get removed in his patch to cache main resources.

My main resource patch as currently written won't remove that call, but I can certainly add this to the list of things to think about.

[WebKit Review Bot]

Comment on attachment 174779 [details]
Fix up some mac failures

Clearing flags on attachment: 174779

Committed r135172: <http://trac.webkit.org/changeset/135172>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Chris Dumez]

This patch seems to have broken several API tests:

EFL:
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6080/steps/API%20tests/logs/stdio

Mac:
http://build.webkit.org/builders/Apple%20MountainLion%20Debug%20WK2%20%28Tests%29/builds/3784/steps/run-api-tests/logs/stdio

[WebKit Review Bot]

Re-opened since this is blocked by bug 102710

[Nate Chapin]

Created attachment 176503 [details]
Fix TestWebKitAPI failures

Ok, I believe I've tested this patch on API tests for mac, layout tests for mac and chromium linux, and chromium's downstream browser_tests.

[Adam Barth]

Comment on attachment 176503 [details]
Fix TestWebKitAPI failures

Sounds like we should try this one again.

[WebKit Review Bot]

Comment on attachment 176503 [details]
Fix TestWebKitAPI failures

Clearing flags on attachment: 176503

Committed r136031: <http://trac.webkit.org/changeset/136031>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Sudarsana Nagineni (babu)]

This patch broke the API test on EFL debug bot.

http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6449
http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6449/steps/API%20tests/logs/stdio

ASSERTION FAILED: m_currentItem
/home/buildslave-1/webkit-buildslave/efl-linux-64-debug-wk2/build/Source/WebCore/loader/HistoryController.cpp(112) : void WebCore::HistoryController::restoreScrollPositionAndViewState()
1   0x2b42615c05c3 WebCore::HistoryController::restoreScrollPositionAndViewState()
2   0x2b42615b465b WebCore::FrameLoader::checkLoadCompleteForThisFrame()
3   0x2b42615b5362 WebCore::FrameLoader::checkLoadComplete()
4   0x2b426158e2ca WebCore::DocumentLoader::finishedLoading()
5   0x2b4261590101 WebCore::DocumentLoader::maybeLoadEmpty()
6   0x2b42615901d5 WebCore::DocumentLoader::startLoadingMainResource()
7   0x2b42615b4972 WebCore::FrameLoader::continueLoadAfterWillSubmitForm()
8   0x2b42615b7461 WebCore::FrameLoader::continueLoadAfterNavigationPolicy(WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)
9   0x2b42615b6b95 WebCore::FrameLoader::callContinueLoadAfterNavigationPolicy(void*, WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool)
10  0x2b42615ce664 WebCore::PolicyChecker::checkNavigationPolicy(WebCore::ResourceRequest const&, WebCore::DocumentLoader*, WTF::PassRefPtr<WebCore::FormState>, void (*)(void*, WebCore::ResourceRequest const&, WTF::PassRefPtr<WebCore::FormState>, bool), void*)
11  0x2b42615b10dc WebCore::FrameLoader::loadWithDocumentLoader(WebCore::DocumentLoader*, WebCore::FrameLoadType, WTF::PassRefPtr<WebCore::FormState>)
12  0x2b42615b1739 WebCore::FrameLoader::reloadWithOverrideEncoding(WTF::String const&)
13  0x2b425df33ff5 WebKit::WebPage::setCustomTextEncodingName(WTF::String const&)

[Sudarsana Nagineni (babu)]

(In reply to comment #38)
> This patch broke the API test on EFL debug bot.
> 
> http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6449
> http://build.webkit.org/builders/EFL%20Linux%2064-bit%20Debug%20WK2/builds/6449/steps/API%20tests/logs/stdio

I filed a new bug report to track this issue - https://bugs.webkit.org/show_bug.cgi?id=103732