Bug 137936 – Remove USE(CFNETWORK)+PLATFORM(COCOA) code path

• Status: NEW
• Component: Page Loading

[Antti Koivisto]

This C API path is currently used on iOS. It is adding tons of complexity and bug surface for questionable benefits. We should remove it and switch to the CFNetwork ObjC API which we already use on OS X.

[Antti Koivisto]

Created attachment 240227 [details]
WIP patch

[Antti Koivisto]

Created attachment 240231 [details]
another

[Antti Koivisto]

Created attachment 240232 [details]
another

[WebKit Commit Bot]

Attachment 240232 [details] did not pass style-queue:


ERROR: Source/WebKit2/Shared/Downloads/ios/DownloadIOS.mm:1:  Could not find a newline character at the end of the file.  [whitespace/ending_newline] [5]
Total errors found: 1 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Antti Koivisto]

Created attachment 246813 [details]
rebased

[Antti Koivisto]

Created attachment 246814 [details]
rebased

[Alexey Proskuryakov]

I strongly oppose doing this at this time.

While using the same code path on both platforms is so obviously the way to go, there couldn't be a worse time to do it.

[Antti Koivisto]

The iOS build failure on EWS is due to NSURLDownload being a private header. Internal build seems to run fine.

[David Kilzer (:ddkilzer)]

Created attachment 267462 [details]
Rebased onto r194115

[David Kilzer (:ddkilzer)]

(In reply to comment #9)
> Created attachment 267462 [details]
> Rebased onto r194115

This patch still has a scheduling bug where NSURLConnection callbacks are happening on the main thread (without the WebThread lock), but I have a fix for that that I'm testing locally.

This patch is looking very promising now.

[Darin Adler]

At the time we switched to the C API it was a significant speedup. Has this changed since?

[David Kilzer (:ddkilzer)]

(In reply to comment #11)
> At the time we switched to the C API it was a significant speedup. Has this
> changed since?

Performance of this change has not been tested yet.  However, I think the switch to CFURLConnection occurred prior to many CF types being [re-]implemented in terms of NS types.  I plan to engage the perf team to test this change before landing it.

[David Kilzer (:ddkilzer)]

(In reply to comment #10)
> (In reply to comment #9)
> > Created attachment 267462 [details]
> > Rebased onto r194115
> 
> This patch still has a scheduling bug where NSURLConnection callbacks are
> happening on the main thread (without the WebThread lock), but I have a fix
> for that that I'm testing locally.

Fixed scheduling issue in Page::platformInitialize() in PageMac.mm.  Also fixed a crash that happened on all quicklook tests due to the -clearHandle method being called on a class that didn't implement it.

With these fixes, the NSURLConnection loader is experiencing roughly the same crashes as the exiting CFURLConnection code path (less than 10 total crashes on a full test run).

[David Kilzer (:ddkilzer)]

Created attachment 267532 [details]
Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed)

[David Kilzer (:ddkilzer)]

Created attachment 267533 [details]
Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed) v2

[WebKit Commit Bot]

Attachment 267533 [details] did not pass style-queue:


ERROR: Source/WebCore/platform/network/ios/QuickLook.mm:41:  *SoftLink.h header should be included after all other headers.  [build/include_order] [4]
Total errors found: 1 in 34 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[David Kilzer (:ddkilzer)]

Created attachment 267553 [details]
Patch to find build errors (fixes for iOS and Windows)

[David Kilzer (:ddkilzer)]

Created attachment 267554 [details]
Patch to find build errors (fixes for iOS and Windows) v2

[David Kilzer (:ddkilzer)]

Created attachment 267557 [details]
Patch to find build errors (fix NSURLDownload on iOS)

[David Kilzer (:ddkilzer)]

Created attachment 267564 [details]
Patch to find build errors (fix WebDownload on iOS)

[David Kilzer (:ddkilzer)]

Created attachment 267567 [details]
Patch to find build errors (fix WebDownload on iOS) v2

[David Kilzer (:ddkilzer)]

Created attachment 267569 [details]
Patch to find build errors (fix DownloadMac.mm on iOS)

[Antti Koivisto]

One option would be to land https://bugs.webkit.org/show_bug.cgi?id=142540 first. It just switches the code paths (and makes them work) without deleting anything. That might be good for checking regressions.

[David Kilzer (:ddkilzer)]

Created attachment 267925 [details]
Rebaseline onto r194421 (build is likely broken again)

[David Kilzer (:ddkilzer)]

(In reply to comment #23)
> One option would be to land https://bugs.webkit.org/show_bug.cgi?id=142540
> first. It just switches the code paths (and makes them work) without
> deleting anything. That might be good for checking regressions.

Okay.  I need to port some fixes from this patch to that one.  Probably a better idea to land them separately anyway.  :)

[David Kilzer (:ddkilzer)]

(In reply to comment #25)
> (In reply to comment #23)
> > One option would be to land https://bugs.webkit.org/show_bug.cgi?id=142540
> > first. It just switches the code paths (and makes them work) without
> > deleting anything. That might be good for checking regressions.
> 
> Okay.  I need to port some fixes from this patch to that one.  Probably a
> better idea to land them separately anyway.  :)

There is now a patch up on Bug 142540 for review.