Bug 171260 - WebCore.framework should restrict allowable_clients
Summary: WebCore.framework should restrict allowable_clients
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Aakash Jain
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-04-24 21:30 PDT by Aakash Jain
Modified: 2017-04-25 16:49 PDT (History)
8 users (show)

See Also:


Attachments
Proposed patch (3.27 KB, patch)
2017-04-24 22:16 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff
Proposed patch (3.27 KB, patch)
2017-04-24 22:18 PDT, Aakash Jain
ap: review-
Details | Formatted Diff | Diff
Updated patch (3.29 KB, patch)
2017-04-25 11:16 PDT, Aakash Jain
ap: review-
Details | Formatted Diff | Diff
Updated patch (3.33 KB, patch)
2017-04-25 12:11 PDT, Aakash Jain
ap: review-
Details | Formatted Diff | Diff
Updated patch (3.39 KB, patch)
2017-04-25 13:29 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff
Updated patch (3.54 KB, patch)
2017-04-25 14:57 PDT, Aakash Jain
ap: review+
Details | Formatted Diff | Diff
Updated patch (3.62 KB, patch)
2017-04-25 16:20 PDT, Aakash Jain
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Aakash Jain 2017-04-24 21:30:10 PDT
On Mac OS X, WebCore.framework is beneath the WebKit umbrella framework, which hides its symbols from other frameworks and apps.

We should do something similar on iOS and make WebCore.framework completely private except for linking few specific frameworks and apps.

See <rdar://problem/15145065>.
Comment 1 Aakash Jain 2017-04-24 22:16:47 PDT
Created attachment 308066 [details]
Proposed patch
Comment 2 Aakash Jain 2017-04-24 22:18:48 PDT
Created attachment 308067 [details]
Proposed patch
Comment 3 mitz 2017-04-24 22:20:40 PDT
Comment on attachment 308067 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308067&action=review

> Source/WebCore/Configurations/WebCore.xcconfig:81
> +OTHER_LDFLAGS_BASE = -lsqlite3 -lobjc -lANGLE -framework Metal -allowable_client WebCoreTestSupport -allowable_client WebKit2 -allowable_client WebKitLegacy;

“-allowable_client WebKit2” doesn’t make sense because there’s no client with that name. Perhaps you meant “WebKit”.
Comment 4 Alexey Proskuryakov 2017-04-24 23:14:22 PDT
Comment on attachment 308067 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308067&action=review

> Source/WebCore/Configurations/WebCore.xcconfig:-85
> -OTHER_LDFLAGS_PLATFORM[sdk=macosx*] = $(OTHER_LDFLAGS_BASE) -sub_library libobjc -umbrella WebKit -allowable_client WebCoreTestSupport -allowable_client WebKit2 -allowable_client WebKitLegacy -framework ApplicationServices -framework AudioUnit -framework Carbon -framework Cocoa -framework CoreAudio -framework DataDetectorsCore -framework IOSurface -framework OpenGL -framework SystemConfiguration $(LIBWEBRTC_LDFLAGS);

Funny that "-allowable_client WebKit2" is existing code!
Comment 5 Alexey Proskuryakov 2017-04-25 11:08:32 PDT
Comment on attachment 308067 [details]
Proposed patch

r- for Dan's comment, lack of TVBooks, and also this is breaking the build.
Comment 6 Aakash Jain 2017-04-25 11:16:21 PDT
Created attachment 308118 [details]
Updated patch

Added WebKit, TVBooks as allowable_client.
Comment 7 Alexey Proskuryakov 2017-04-25 11:48:34 PDT
Comment on attachment 308118 [details]
Updated patch

This is still breaking iOS build.
Comment 8 Aakash Jain 2017-04-25 12:11:04 PDT
Created attachment 308126 [details]
Updated patch

Added DumpRenderTree as allowable_client, still testing the patch on my local machine.
Comment 9 Alexey Proskuryakov 2017-04-25 13:01:19 PDT
Comment on attachment 308126 [details]
Updated patch

Now failing with TestWebKitAPI.

I wonder if we even need to link these to WebCore now.
Comment 10 Aakash Jain 2017-04-25 13:29:12 PDT
Created attachment 308134 [details]
Updated patch

Added WebKitTestRunner and TestWebKitAPI as allowable_client.

Will check separately if TestWebKitAPI (and other similar projects) really need to link to WebCore. Will track in separate bugs.
Comment 11 Aakash Jain 2017-04-25 14:57:31 PDT
Created attachment 308151 [details]
Updated patch

Removed WebKit from macOS to match previous configuration.
Comment 12 Alexey Proskuryakov 2017-04-25 15:13:52 PDT
Comment on attachment 308151 [details]
Updated patch

View in context: https://bugs.webkit.org/attachment.cgi?id=308151&action=review

Looks good to me.

> Source/WebCore/Configurations/WebCore.xcconfig:82
> +OTHER_LDFLAGS_BASE_ios = $(OTHER_LDFLAGS_BASE) -framework CFNetwork -framework CoreGraphics -framework CoreText -framework Foundation -framework ImageIO -framework MobileCoreServices -framework OpenGLES -lMobileGestalt $(WK_IOS_BINCOMPAT_LDFLAGS) -allowable_client WebKit -allowable_client iTunesU -allowable_client Casablanca -allowable_client Remote -allowable_client TVBooks -allowable_client DumpRenderTree -allowable_client WebKitTestRunner -allowable_client TestWebKitAPI;

Could you please add a comment with radar numbers for the ones we expect to remove eventually?
Comment 13 Aakash Jain 2017-04-25 16:20:55 PDT
Created attachment 308160 [details]
Updated patch

Added FIXME.
Comment 14 WebKit Commit Bot 2017-04-25 16:49:17 PDT
Comment on attachment 308160 [details]
Updated patch

Clearing flags on attachment: 308160

Committed r215769: <http://trac.webkit.org/changeset/215769>
Comment 15 WebKit Commit Bot 2017-04-25 16:49:19 PDT
All reviewed patches have been landed.  Closing bug.