Bug 101512

Created attachment 172875 [details] patch

Comment on attachment 172875 [details] patch Wow, this patch is really cool.

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?

(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.

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

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

(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.

Is this groundwork for 49246?

(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?

(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.

Created attachment 173142 [details] fix tests I'm not sure this patch fixes *all* the tests, so uploading without r? to ask EWS.

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

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

> 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.

Created attachment 173718 [details] EWS experiment #2

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

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.

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.

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

Created attachment 174220 [details] Patch for landing

Comment on attachment 174220 [details] Patch for landing Clearing flags on attachment: 174220 Committed r134649: <http://trac.webkit.org/changeset/134649>

All reviewed patches have been landed. Closing bug.

Re-opened since this is blocked by bug 102413

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?

(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.

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.

Created attachment 174779 [details] Fix up some mac failures All diffs from reverted version are still in DocumentLoader.cpp

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.

(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.

Comment on attachment 174779 [details] Fix up some mac failures Clearing flags on attachment: 174779 Committed r135172: <http://trac.webkit.org/changeset/135172>

All reviewed patches have been landed. Closing bug.

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

Re-opened since this is blocked by bug 102710

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.

Comment on attachment 176503 [details] Fix TestWebKitAPI failures Sounds like we should try this one again.

Comment on attachment 176503 [details] Fix TestWebKitAPI failures Clearing flags on attachment: 176503 Committed r136031: <http://trac.webkit.org/changeset/136031>

All reviewed patches have been landed. Closing bug.

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&)

(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