WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
101512
Move empty loading to DocumentLoader, simplify FrameLoader::init()
https://bugs.webkit.org/show_bug.cgi?id=101512
Summary
Move empty loading to DocumentLoader, simplify FrameLoader::init()
Nate Chapin
Reported
2012-11-07 14:37:20 PST
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.
Attachments
patch
(22.55 KB, patch)
2012-11-07 14:44 PST
,
Nate Chapin
buildbot
: commit-queue-
Details
Formatted Diff
Diff
fix tests
(28.74 KB, patch)
2012-11-08 16:17 PST
,
Nate Chapin
buildbot
: commit-queue-
Details
Formatted Diff
Diff
EWS experiment #2
(27.43 KB, patch)
2012-11-12 13:57 PST
,
Nate Chapin
buildbot
: commit-queue-
Details
Formatted Diff
Diff
patch #2
(28.73 KB, patch)
2012-11-13 15:40 PST
,
Nate Chapin
no flags
Details
Formatted Diff
Diff
Patch for landing
(30.01 KB, patch)
2012-11-14 12:12 PST
,
Nate Chapin
no flags
Details
Formatted Diff
Diff
Set the url to about:blank for non-initial empty loads
(29.38 KB, patch)
2012-11-16 12:28 PST
,
Nate Chapin
no flags
Details
Formatted Diff
Diff
Fix up some mac failures
(29.56 KB, patch)
2012-11-16 16:28 PST
,
Nate Chapin
no flags
Details
Formatted Diff
Diff
Fix TestWebKitAPI failures
(32.74 KB, patch)
2012-11-28 09:58 PST
,
Nate Chapin
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
Nate Chapin
Comment 1
2012-11-07 14:44:05 PST
Created
attachment 172875
[details]
patch
Adam Barth
Comment 2
2012-11-07 15:26:32 PST
Comment on
attachment 172875
[details]
patch Wow, this patch is really cool.
Adam Barth
Comment 3
2012-11-07 15:29:29 PST
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
Comment 4
2012-11-07 16:09:05 PST
(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 5
2012-11-07 19:09:53 PST
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 6
2012-11-08 12:54:07 PST
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
Comment 7
2012-11-08 14:36:13 PST
(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
Comment 8
2012-11-08 14:36:55 PST
Is this groundwork for 49246?
Nate Chapin
Comment 9
2012-11-08 15:47:21 PST
(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
Comment 10
2012-11-08 15:53:06 PST
(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
Comment 11
2012-11-08 16:17:25 PST
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 12
2012-11-09 04:28:51 PST
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 13
2012-11-09 09:16:22 PST
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
Comment 14
2012-11-12 13:30:13 PST
> 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
Comment 15
2012-11-12 13:57:10 PST
Created
attachment 173718
[details]
EWS experiment #2
Build Bot
Comment 16
2012-11-13 10:01:07 PST
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
Comment 17
2012-11-13 15:40:41 PST
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 18
2012-11-13 22:49:28 PST
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 19
2012-11-14 00:37:03 PST
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
Comment 20
2012-11-14 12:12:27 PST
Created
attachment 174220
[details]
Patch for landing
WebKit Review Bot
Comment 21
2012-11-14 12:35:54 PST
Comment on
attachment 174220
[details]
Patch for landing Clearing flags on attachment: 174220 Committed
r134649
: <
http://trac.webkit.org/changeset/134649
>
WebKit Review Bot
Comment 22
2012-11-14 12:35:58 PST
All reviewed patches have been landed. Closing bug.
WebKit Review Bot
Comment 23
2012-11-15 10:40:43 PST
Re-opened since this is blocked by
bug 102413
Darin Adler
Comment 24
2012-11-15 21:25:04 PST
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
Comment 25
2012-11-16 10:00:33 PST
(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
Comment 26
2012-11-16 12:28:10 PST
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
Comment 27
2012-11-16 16:28:51 PST
Created
attachment 174779
[details]
Fix up some mac failures All diffs from reverted version are still in DocumentLoader.cpp
Adam Barth
Comment 28
2012-11-17 10:02:34 PST
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
Comment 29
2012-11-19 09:47:47 PST
(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 30
2012-11-19 10:01:34 PST
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
Comment 31
2012-11-19 10:01:40 PST
All reviewed patches have been landed. Closing bug.
Chris Dumez
Comment 32
2012-11-19 12:07:06 PST
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
Comment 33
2012-11-19 13:08:53 PST
Re-opened since this is blocked by
bug 102710
Nate Chapin
Comment 34
2012-11-28 09:58:42 PST
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 35
2012-11-28 11:12:38 PST
Comment on
attachment 176503
[details]
Fix TestWebKitAPI failures Sounds like we should try this one again.
WebKit Review Bot
Comment 36
2012-11-28 11:31:38 PST
Comment on
attachment 176503
[details]
Fix TestWebKitAPI failures Clearing flags on attachment: 176503 Committed
r136031
: <
http://trac.webkit.org/changeset/136031
>
WebKit Review Bot
Comment 37
2012-11-28 11:31:44 PST
All reviewed patches have been landed. Closing bug.
Sudarsana Nagineni (babu)
Comment 38
2012-11-30 03:53:14 PST
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)
Comment 39
2012-11-30 07:30:52 PST
(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
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug