Bug 116887 – Freeze when loading a particular page on washingtonpost.com with NetworkProcess enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Alexey Proskuryakov]

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>

[Alexey Proskuryakov]

Created attachment 203093 [details]
proposed fix

[WebKit Commit Bot]

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.

[Early Warning System Bot]

Comment on attachment 203093 [details]
proposed fix

Attachment 203093 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/710321

[Early Warning System Bot]

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

[Darin Adler]

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

[EFL EWS Bot]

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

[Alexey Proskuryakov]

Created attachment 203097 [details]
with build fixes

[Build Bot]

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

[Alexey Proskuryakov]

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

[Alexey Proskuryakov]

Created attachment 203100 [details]
with more build fixes

[Build Bot]

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

[Build Bot]

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

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/r150853>.