Summary: | Freeze when loading a particular page on washingtonpost.com with NetworkProcess enabled | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alexey Proskuryakov <ap> | ||||||||
Component: | WebKit2 | Assignee: | Alexey Proskuryakov <ap> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | buildbot, commit-queue, darin, eflews.bot, gyuyoung.kim, rniwa, webkit-ews | ||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
URL: | http://www.washingtonpost.com/wp-srv/special/sports/grade-robert-griffin-iii | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=157153 | ||||||||||
Bug Depends on: | 117036 | ||||||||||
Bug Blocks: | |||||||||||
Attachments: |
|
Description
Alexey Proskuryakov
2013-05-28 14:27:15 PDT
Created attachment 203093 [details]
proposed fix
Attachment 203093 [details] did not pass style-queue:
Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http/tests/uri/curly-braces-escaping-expected.txt', u'LayoutTests/http/tests/uri/curly-braces-escaping.html', u'LayoutTests/http/tests/uri/resources/echo-uri.php', u'Source/WebCore/ChangeLog', u'Source/WebCore/WebCore.exp.in', u'Source/WebCore/WebCore.vcproj/WebCore.vcproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj', u'Source/WebCore/WebCore.vcxproj/WebCore.vcxproj.filters', u'Source/WebCore/WebCore.xcodeproj/project.pbxproj', u'Source/WebCore/platform/KURL.cpp', u'Source/WebCore/platform/KURL.h', u'Source/WebCore/platform/cf/CFURLExtras.cpp', u'Source/WebCore/platform/cf/CFURLExtras.h', u'Source/WebCore/platform/cf/KURLCFNet.cpp', u'Source/WebCore/platform/mac/KURLMac.mm', u'Source/WebCore/platform/network/cf/ResourceErrorCF.cpp', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/Shared/API/c/cf/WKURLCF.cpp', u'Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp']" exit_code: 1
Source/WebCore/platform/KURL.h:200: The parameter name "buffer" adds no information, so it should be removed. [readability/parameter_name] [5]
Source/WebCore/platform/cf/KURLCFNet.cpp:30: Alphabetical sorting problem. [build/include_order] [4]
Total errors found: 2 in 20 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 203093 [details] proposed fix Attachment 203093 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/710321 Comment on attachment 203093 [details] proposed fix Attachment 203093 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/700103 Comment on attachment 203093 [details] proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=203093&action=review r=me, but the patch as-is breaks builds on all non-CF platforms so please do fix that > Source/WebCore/platform/KURL.h:29 > +#include "CFURLExtras.h" This is a platform-specific header file so it should not be included unconditionally. I think it’s hard to know where to put the URLCharBuffer typedef. Maybe we can repeat it in both CFURLExtras.h and in KURL.h. >> Source/WebCore/platform/KURL.h:200 >> + void copyToBuffer(URLCharBuffer& buffer) const; > > The parameter name "buffer" adds no information, so it should be removed. [readability/parameter_name] [5] I agree with the style bot on this. > Source/WebCore/platform/cf/CFURLExtras.h:36 > +typedef Vector<char, 512> URLCharBuffer; This is not CF-specific and needs to be compiled on platform that can’t compile CFURLRef. > Source/WebCore/platform/cf/KURLCFNet.cpp:44 > + // FIXME: Why is it OK to ignore base URL here? The normal way to deal with this would be to call CFURLCopyAbsoluteURL. I think the comment should say something more like: // FIXME: Should we call CFURLCopyAbsoluteURL here? > Source/WebCore/platform/mac/KURLMac.mm:62 > + // We use the toll-free bridge between NSURL and CFURL to > + // create a CFURLRef supporting both empty and null values. What does “a CFURLRef supporting both empty and null values” mean? > Source/WebKit2/Shared/cf/ArgumentCodersCF.cpp:532 > + // FIXME: This loses base URL, which seems incorrect. I would say “discards” rather than “loses”. Comment on attachment 203093 [details] proposed fix Attachment 203093 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/691132 Created attachment 203097 [details]
with build fixes
Comment on attachment 203097 [details] with build fixes Attachment 203097 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/741078 (In reply to comment #5) > > Source/WebCore/platform/cf/KURLCFNet.cpp:44 > > + // FIXME: Why is it OK to ignore base URL here? > > The normal way to deal with this would be to call CFURLCopyAbsoluteURL. I think the comment should say something more like: I don't think that we can do this. As I found when working on this patch, the main thing to know about CFURL is to not use any accessors other than CFURLGetBytes, as everything else works on a normalized string. I'll change the FIXME to suggest that we may want to recursively apply base URL. > > Source/WebCore/platform/mac/KURLMac.mm:62 > > + // We use the toll-free bridge between NSURL and CFURL to > > + // create a CFURLRef supporting both empty and null values. > > What does “a CFURLRef supporting both empty and null values” mean? I don't understand it in enough detail to change (it's a pre-existing one that someone else added). It certainly has something to do with a comment we have in WebKit2 code, "CFURL can't hold an empty URL, unlike NSURL", but that is also dubious. Created attachment 203100 [details]
with more build fixes
Comment on attachment 203100 [details] with more build fixes Attachment 203100 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/659058 Comment on attachment 203100 [details] with more build fixes Attachment 203100 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/724194 Committed <http://trac.webkit.org/r150853>. |