Bug 116887

Summary: Freeze when loading a particular page on washingtonpost.com with NetworkProcess enabled
Product: WebKit Reporter: Alexey Proskuryakov <ap>
Component: WebKit2Assignee: 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 Flags
proposed fix
darin: review+, webkit-ews: commit-queue-
with build fixes
darin: review+, buildbot: commit-queue-
with more build fixes buildbot: commit-queue-

Description Alexey Proskuryakov 2013-05-28 14:27:15 PDT
WebProcess freezes when opening <http://www.washingtonpost.com/wp-srv/special/sports/grade-robert-griffin-iii>

This happens because we load a certain resource using a slightly different URL, and get dramatically different responses:

http://data.washingtonpost.com/features/ratings/?q={%22grade_rg3_game_id%22:%22%22,%22username%22:%22d1367869948322%22}

vs.

http://data.washingtonpost.com/features/ratings/?q=%7B%22grade_rg3_game_id%22:%22%22,%22username%22:%22d1367869948322%22%7D

The two NSURLs are almost indistinguishable via API. The only way to get to original unencoded string is through CFURLGetBytes. So, this is what WebKit2 IPC should use, not CFURLGetString.

<rdar://problem/12965959>
Comment 1 Alexey Proskuryakov 2013-05-28 14:51:36 PDT
Created attachment 203093 [details]
proposed fix
Comment 2 WebKit Commit Bot 2013-05-28 14:54:37 PDT
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 3 Early Warning System Bot 2013-05-28 15:03:05 PDT
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 4 Early Warning System Bot 2013-05-28 15:04:16 PDT
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 5 Darin Adler 2013-05-28 15:06:04 PDT
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 6 EFL EWS Bot 2013-05-28 15:08:30 PDT
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
Comment 7 Alexey Proskuryakov 2013-05-28 15:09:07 PDT
Created attachment 203097 [details]
with build fixes
Comment 8 Build Bot 2013-05-28 15:12:24 PDT
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
Comment 9 Alexey Proskuryakov 2013-05-28 15:17:12 PDT
(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.
Comment 10 Alexey Proskuryakov 2013-05-28 15:29:10 PDT
Created attachment 203100 [details]
with more build fixes
Comment 11 Build Bot 2013-05-28 15:41:05 PDT
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 12 Build Bot 2013-05-28 15:41:20 PDT
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
Comment 13 Alexey Proskuryakov 2013-05-28 16:26:51 PDT
Committed <http://trac.webkit.org/r150853>.