Bug 181683 - PAL should link to JavaScriptCore rather than WTF
Summary: PAL should link to JavaScriptCore rather than WTF
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Michael Catanzaro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-01-16 08:03 PST by Michael Catanzaro
Modified: 2018-01-16 11:44 PST (History)
6 users (show)

See Also:


Attachments
Patch (2.21 KB, patch)
2018-01-16 09:29 PST, Michael Catanzaro
no flags Details | Formatted Diff | Diff
Patch (3.27 KB, patch)
2018-01-16 10:06 PST, Michael Catanzaro
annulen: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2018-01-16 08:03:36 PST
Currently we are accidentally linking WTF into both libjavascriptcoregtk and libwebkit2gtk, which could in theory cause problems similar to bug #179914. There are only two possible solutions:

 * Ensure WTF always built as a shared library if JavaScriptCore is built as a shared library
 * Change PAL to link to JavaScriptCore instead of WTF

The problem here is caused by layer hopping. It can never be a problem if each library only links to the next lowest-layered library.

The downside is this allows PAL to use JavaScriptCore symbols, which is not super desirable. I came up with a hack yesterday for PAL to link to WTF usually, but link to JavaScriptCore instead if ${JavaScriptCore_LIBRARY_TYPE} MATCHES "SHARED" AND ${WTF_LIBRARY_TYPE} MATCHES "STATIC". But that is getting too complex and introduces more possibility for platform-dependent build failures. Better to just accept that PAL will depend on JavaScriptCore.

Note: I'm not brave enough to update the XCode build. We can use that to enforce that no JavaScriptCore symbols actually get used, I guess.
Comment 1 Michael Catanzaro 2018-01-16 08:04:38 PST
(In reply to Michael Catanzaro from comment #0)
> The problem here is caused by layer hopping. It can never be a problem if
> each library only links to the next lowest-layered library.

Note that, to my knowledge, PAL is the only lib we have that violates this rule.
Comment 2 Konstantin Tokarev 2018-01-16 08:19:00 PST
>Note that, to my knowledge, PAL is the only lib we have that violates this rule.

Not at all. WebKit links to WebCore and JSC, and there are more examples in Tools
Comment 3 Michael Catanzaro 2018-01-16 08:20:59 PST
(In reply to Konstantin Tokarev from comment #2)
> >Note that, to my knowledge, PAL is the only lib we have that violates this rule.
> 
> Not at all. WebKit links to WebCore and JSC, and 

Ah yes, but it shouldn't need to do that.

> there are more examples in Tools

No doubt :)
Comment 4 Michael Catanzaro 2018-01-16 09:29:43 PST
Created attachment 331394 [details]
Patch
Comment 5 Michael Catanzaro 2018-01-16 09:34:17 PST
Comment on attachment 331394 [details]
Patch

It's not right, I should change WebCore to not link directly to JSC now.
Comment 6 Michael Catanzaro 2018-01-16 10:06:55 PST
Created attachment 331396 [details]
Patch
Comment 7 Konstantin Tokarev 2018-01-16 10:20:44 PST
Comment on attachment 331396 [details]
Patch

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

> Source/WebCore/PAL/pal/CMakeLists.txt:26
> +    JavaScriptCore${DEBUG_SUFFIX}

In the perfect world it should be PUBLIC if JSC is SHARED and PRIVATE otherwise. Or, it could be specified as PRIVATE here and ports who build JSC as shared can add it as INTERFACE in a separate command.

But it's ok in this form for now too.
Comment 8 Michael Catanzaro 2018-01-16 11:41:12 PST
(In reply to Konstantin Tokarev from comment #7)
> In the perfect world it should be PUBLIC if JSC is SHARED and PRIVATE
> otherwise.

This would be easiest... currently we make almost no use of library dependency visibility, though. Let's explore that in future patches.
Comment 9 Michael Catanzaro 2018-01-16 11:41:53 PST
Committed r226989: <https://trac.webkit.org/changeset/226989>
Comment 10 Radar WebKit Bug Importer 2018-01-16 11:44:57 PST
<rdar://problem/36552175>