Bug 137936 - Remove USE(CFNETWORK)+PLATFORM(COCOA) code path
Summary: Remove USE(CFNETWORK)+PLATFORM(COCOA) code path
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Kilzer (:ddkilzer)
URL:
Keywords:
Depends on: 142540
Blocks:
  Show dependency treegraph
 
Reported: 2014-10-21 15:49 PDT by Antti Koivisto
Modified: 2015-12-31 01:22 PST (History)
18 users (show)

See Also:


Attachments
WIP patch (74.14 KB, patch)
2014-10-21 15:50 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
another (80.34 KB, patch)
2014-10-21 17:29 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
another (80.50 KB, patch)
2014-10-21 17:33 PDT, Antti Koivisto
no flags Details | Formatted Diff | Diff
rebased (89.64 KB, patch)
2015-02-18 06:02 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
rebased (91.69 KB, patch)
2015-02-18 07:09 PST, Antti Koivisto
no flags Details | Formatted Diff | Diff
Rebased onto r194115 (107.55 KB, patch)
2015-12-16 09:49 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed) (123.72 KB, patch)
2015-12-16 21:33 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed) v2 (123.43 KB, patch)
2015-12-16 21:40 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch to find build errors (fixes for iOS and Windows) (125.36 KB, patch)
2015-12-17 03:14 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch to find build errors (fixes for iOS and Windows) v2 (125.32 KB, patch)
2015-12-17 03:20 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch to find build errors (fix NSURLDownload on iOS) (127.45 KB, patch)
2015-12-17 04:45 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch to find build errors (fix WebDownload on iOS) (128.10 KB, patch)
2015-12-17 09:00 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch to find build errors (fix WebDownload on iOS) v2 (128.09 KB, patch)
2015-12-17 09:49 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Patch to find build errors (fix DownloadMac.mm on iOS) (128.16 KB, patch)
2015-12-17 10:47 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff
Rebaseline onto r194421 (build is likely broken again) (123.36 KB, patch)
2015-12-26 04:21 PST, David Kilzer (:ddkilzer)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antti Koivisto 2014-10-21 15:49:22 PDT
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.
Comment 1 Antti Koivisto 2014-10-21 15:50:37 PDT
Created attachment 240227 [details]
WIP patch
Comment 2 Antti Koivisto 2014-10-21 17:29:45 PDT
Created attachment 240231 [details]
another
Comment 3 Antti Koivisto 2014-10-21 17:33:16 PDT
Created attachment 240232 [details]
another
Comment 4 WebKit Commit Bot 2014-10-21 17:35:59 PDT
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.
Comment 5 Antti Koivisto 2015-02-18 06:02:55 PST
Created attachment 246813 [details]
rebased
Comment 6 Antti Koivisto 2015-02-18 07:09:12 PST
Created attachment 246814 [details]
rebased
Comment 7 Alexey Proskuryakov 2015-02-18 12:03:23 PST
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.
Comment 8 Antti Koivisto 2015-02-18 13:15:36 PST
The iOS build failure on EWS is due to NSURLDownload being a private header. Internal build seems to run fine.
Comment 9 David Kilzer (:ddkilzer) 2015-12-16 09:49:47 PST
Created attachment 267462 [details]
Rebased onto r194115
Comment 10 David Kilzer (:ddkilzer) 2015-12-16 11:10:51 PST
(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.
Comment 11 Darin Adler 2015-12-16 19:32:42 PST
At the time we switched to the C API it was a significant speedup. Has this changed since?
Comment 12 David Kilzer (:ddkilzer) 2015-12-16 21:22:31 PST
(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.
Comment 13 David Kilzer (:ddkilzer) 2015-12-16 21:26:27 PST
(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).
Comment 14 David Kilzer (:ddkilzer) 2015-12-16 21:33:22 PST
Created attachment 267532 [details]
Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed)
Comment 15 David Kilzer (:ddkilzer) 2015-12-16 21:40:11 PST
Created attachment 267533 [details]
Patch to find build errors (iOS WK1 scheduling fixed; quickook tests fixed) v2
Comment 16 WebKit Commit Bot 2015-12-16 21:41:34 PST
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.
Comment 17 David Kilzer (:ddkilzer) 2015-12-17 03:14:38 PST
Created attachment 267553 [details]
Patch to find build errors (fixes for iOS and Windows)
Comment 18 David Kilzer (:ddkilzer) 2015-12-17 03:20:10 PST
Created attachment 267554 [details]
Patch to find build errors (fixes for iOS and Windows) v2
Comment 19 David Kilzer (:ddkilzer) 2015-12-17 04:45:51 PST
Created attachment 267557 [details]
Patch to find build errors (fix NSURLDownload on iOS)
Comment 20 David Kilzer (:ddkilzer) 2015-12-17 09:00:51 PST
Created attachment 267564 [details]
Patch to find build errors (fix WebDownload on iOS)
Comment 21 David Kilzer (:ddkilzer) 2015-12-17 09:49:59 PST
Created attachment 267567 [details]
Patch to find build errors (fix WebDownload on iOS) v2
Comment 22 David Kilzer (:ddkilzer) 2015-12-17 10:47:42 PST
Created attachment 267569 [details]
Patch to find build errors (fix DownloadMac.mm on iOS)
Comment 23 Antti Koivisto 2015-12-18 11:13:45 PST
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.
Comment 24 David Kilzer (:ddkilzer) 2015-12-26 04:21:18 PST
Created attachment 267925 [details]
Rebaseline onto r194421 (build is likely broken again)
Comment 25 David Kilzer (:ddkilzer) 2015-12-27 17:38:52 PST
(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.  :)
Comment 26 David Kilzer (:ddkilzer) 2015-12-31 01:22:32 PST
(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.