Teach WebKit2 about window.internals Requested by dglazkov on #webkit.
The trickiest part here is linking. A straight-shot approach of just linking libWebCoreTestSupport.a into WebKitTestRunnerInjectedBundle fails with these errors: Undefined symbols: "__ZN7WebCore21getCachedDOMStructureEPNS_17JSDOMGlobalObjectEPKN3JSC9ClassInfoE", referenced from: __ZN7WebCore20JSInternalsPrototype4selfEPN3JSC9ExecStateEPNS1_14JSGlobalObjectE in libWebCoreTestSupport.a(JSInternals.o) __ZN7WebCore4toJSEPN3JSC9ExecStateEPNS_17JSDOMGlobalObjectEPNS_9InternalsE in libWebCoreTestSupport.a(JSInternals.o) "__ZN7WebCore12JSDOMWrapper12noWeakVtableEv", referenced from: __ZTVN7WebCore11JSInternalsE in libWebCoreTestSupport.a(JSInternals.o) "__ZN7WebCore17cacheDOMStructureEPNS_17JSDOMGlobalObjectEPN3JSC9StructureEPKNS2_9ClassInfoE", referenced from: __ZN7WebCore20JSInternalsPrototype4selfEPN3JSC9ExecStateEPNS1_14JSGlobalObjectE in libWebCoreTestSupport.a(JSInternals.o) __ZN7WebCore4toJSEPN3JSC9ExecStateEPNS_17JSDOMGlobalObjectEPNS_9InternalsE in libWebCoreTestSupport.a(JSInternals.o) These symbols are exported by WebCore framework, but when WebKit2 links, it somehow doesn't carry over these symbols (although nm says some WebCore symbols are exported). How can I make this work?
Created attachment 94474 [details] WIP:Straight-shot approach (does not work)
Here’s my guess: I think that Mac WebKit2 uses export macros and visibility to control what’s exported, unlike WebKit and WebCore. So any symbols we want export need to be marked with a GCC visibility attribute. We use the WK_EXPORT macro to do this for WebKit2’s public API and we’d have to do something similar for the WebCore symbols.
(In reply to comment #3) > Here’s my guess: I think that Mac WebKit2 uses export macros and visibility to control what’s exported, unlike WebKit and WebCore. So any symbols we want export need to be marked with a GCC visibility attribute. We use the WK_EXPORT macro to do this for WebKit2’s public API and we’d have to do something similar for the WebCore symbols. Oh cool. I'll try that. WC_EXPORT, here we come!
I believe the distinction is that WebKit reexports WebCore’s symbols, while WebKit2 doesn’t (and shouldn’t).
(In reply to comment #5) > I believe the distinction is that WebKit reexports WebCore’s symbols, while WebKit2 doesn’t (and shouldn’t). WebKit2 wants to reexport these particular WebCore symbols.
Should I try to make WTF_EXPORT build on Mac?: http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/ExportMacros.h
(In reply to comment #6) > (In reply to comment #5) > > I believe the distinction is that WebKit reexports WebCore’s symbols, while WebKit2 doesn’t (and shouldn’t). > > WebKit2 wants to reexport these particular WebCore symbols. My understanding is that the WebCore symbols need to be accessible to DRT and WebKitTestRunner by virtue of them linking in libWebCoreTestSupport. Those projects can’t access them directly because WebCore has a fixed list of clients that can link against it. Having WebKit2 reexport the symbols would work around this because it is on the allowable client list. Alternatively, WebCoreTestSupport could be converted to a dylib and this dylib could be added to the allowable client list for WebCore. This would prevent WebKit2 from having to export symbols that are only needed for test purposes.
>Having WebKit2 reexport the symbols would work around this because it is on the allowable client list. How do I do that? Docs, pointers, pseudocode, or hand-drawings are appreciated. >Alternatively, WebCoreTestSupport could be converted to a dylib and this dylib could be added to the allowable client list for WebCore. This would prevent WebKit2 from having to export symbols that are only needed for test purposes. Is this still going to work for WebKit1?
Created attachment 94713 [details] IT WORKS
(In reply to comment #10) > Created an attachment (id=94713) [details] > IT WORKS Thanks for your suggestions Mark! I made the damn thing work by using the dylib approach.
Created attachment 95807 [details] To cook on bots
Need Win support before can land.
Created attachment 96135 [details] Patch
Comment on attachment 96135 [details] Patch Still need Qt support to land this.
Dominic, how hard would it be to make this patch work on Win?
Created attachment 96548 [details] Patch on top of 96135 that should make this work on Windows Here is a patch on top of attachment 96135 [details] that should make this work on Windows. I will upload a single patch containing both to bounce it off the try bots.
Created attachment 96550 [details] Bot chow This combines attachment 96135 [details] and attachment 96548 [details]. I want to bounce this off the try bots.
(In reply to comment #18) > Created an attachment (id=96550) [details] > Bot chow > > This combines attachment 96135 [details] and attachment 96548 [details]. I want to bounce this off the try bots. I know I hero when I see one. This looks great.
Do you want to gussy this up with a ChangeLog or should I?
(In reply to comment #20) > Do you want to gussy this up with a ChangeLog or should I? Go for it!
Created attachment 96906 [details] Patch
Comment on attachment 96906 [details] Patch Rubber-stamped by me. Let' use the commit queue.
Comment on attachment 96906 [details] Patch Clearing flags on attachment: 96906 Committed r88625: <http://trac.webkit.org/changeset/88625>
All reviewed patches have been landed. Closing bug.
Reverted r88625 for reason: Breaks SL Webkit2 Tests Committed r88630: <http://trac.webkit.org/changeset/88630>
Created attachment 96938 [details] Patch
Comment on attachment 96938 [details] Patch Another try at this. I found out that although mac-wk2 WebKitTestRunner built, it failed at runtime because the WebProcess.app in the WebKit2 framework couldn't find the WebCoreTestSupport dylib. So I have added a build step to WebKitTestRunner to copy the dylib into the WebKitTestRunnerInjectedBundle bundle, and then update the bundle to refer to the dylib via @loader_path instead of @executable_path.
Mark, what do you think?
Created attachment 97065 [details] Patch
Comment on attachment 97065 [details] Patch I found this needed to be updated for qt-wk2.
Dominic emailed me to ask my thoughts on the changes to make the Mac WebKit2 side of things work. Here’s what I had to say: There are two ways of solving this that come to mind: 1) Use @loader_path in the dylib install path and copy the dylib in to the injected bundle's wrapper. 2) Use @rpath in the dylib install path and pass an appropriate -rpath flag when linking the injected bundle and DumpRenderTree. I lean in the direction of the latter since it avoids having to copy the dylib, which can be difficult to do correctly for all configurations. The idea is that the image doing the loading can provide a set of paths to search when the @rpath token is encountered in a load command. This means that DumpRenderTree and the injected bundle could specify different rpaths to account for the fact that the dylib lives at a different location relative to the loading image in each case (something like . and ../../.. respectively).
Comment on attachment 97065 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97065&action=review Marking as r- because at the very least the Xcode project changes in this patch aren’t right. I’d suggest investigating the approach using the path that I outlined which would make some of the comments obsolete, but there are some issues that would need to be addressed either way. > Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:98 > + 4181731B138AD39D0057AAA4 /* WebCoreTestSupport.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = WebCoreTestSupport.h; path = ../../Source/WebCore/testing/js/WebCoreTestSupport.h; sourceTree = SOURCE_ROOT; }; It doesn’t make sense to refer to a source file that lives above the root of the project. WebCoreTestSupport.h should be found relative to BUILT_PRODUCTS_DIR like it is in DumpRenderTree.xcodeproj. > Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:571 > + LIBRARY_SEARCH_PATHS = ( > + "$(inherited)", > + "\"$(SRCROOT)/../../WebKitBuild/Debug\"", > + ); It’s never correct to hard-code paths to your build directory in the Xcode project. > Tools/WebKitTestRunner/mac/run-install-name-tool.sh:31 > +install_name_tool -change '@executable_path/libWebCoreTestSupport.dylib' '@loader_path/libWebCoreTestSupport.dylib' "${BUILT_PRODUCTS_DIR}/${EXECUTABLE_PATH}" This is an incredibly roundabout approach. @executable_path resolves to the path to the main executable of the process. @loader_path resolves to the path to the image that is loading the library. The latter is almost always what is desired, and it would obviously give the correct behavior for both DumpRenderTree and the injected bundle if the dylib is going to live alongside the image that loads it. In other words, it would be more sensible to change the install path at build time in WebCore.xcodeproj.
Created attachment 97228 [details] Patch
Created attachment 97230 [details] Patch that uses @rpath
Created attachment 97232 [details] Patch that uses @loader_path
Thanks for the feedback! I have cleaned up the issues you pointed out with paths into files outside the project and into build output. I tried using @loader_path and friends. I think there are three ways to do this: 1. Just set the install name on libWebCoreTestSupport.dylib to point into BUILT_PRODUCTS_DIR (attachment 97228 [details]). This is how WebCore.framework does it. 2. Use @rpath, and have DumpRenderTree specify -rpath @executable_path/. and WebKitTestRunnerInjectedBundle specify -rpath @loader_path/../../.. (attachment 97230 [details]). This avoids copying the file, but I understand that @rpath is not available on Tiger. I guess that is acceptable, though. 3. Use @loader_path and add a copy files build step to copy libWebCoreTestSupport.dylib into WebKitTestRunnerInjectedBundle.bundle (attachment 97232 [details]). The copy is necessary because @loader_path for DumpRenderTree is WebKitBuild/(Debug|Release) but for WebKitTestRunner it is WebKitBuild/(Debug|Release)/WebKit2.framework/WebProcess.app/Contents/MacOS. Which of these approaches do you prefer? Or am I still missing something…
(In reply to comment #37) > Thanks for the feedback! > > I have cleaned up the issues you pointed out with paths into files outside the project and into build output. > > I tried using @loader_path and friends. I think there are three ways to do this: > > 1. Just set the install name on libWebCoreTestSupport.dylib to point into BUILT_PRODUCTS_DIR (attachment 97228 [details]). This is how WebCore.framework does it. This won’t work in the general case without further changes since it would also require that DYLD_LIBRARY_PATH be set whenever DYLD_FRAMEWORK_PATH is set. None of our scripts do that at present. > 2. Use @rpath, and have DumpRenderTree specify -rpath @executable_path/. and WebKitTestRunnerInjectedBundle specify -rpath @loader_path/../../.. (attachment 97230 [details]). This avoids copying the file, but I understand that @rpath is not available on Tiger. I guess that is acceptable, though. Tiger isn’t supported so this isn’t a concern. > 3. Use @loader_path and add a copy files build step to copy libWebCoreTestSupport.dylib into WebKitTestRunnerInjectedBundle.bundle (attachment 97232 [details]). The copy is necessary because @loader_path for DumpRenderTree is WebKitBuild/(Debug|Release) but for WebKitTestRunner it is WebKitBuild/(Debug|Release)/WebKit2.framework/WebProcess.app/Contents/MacOS. > Which of these approaches do you prefer? Or am I still missing something… As I mentioned earlier, I think the rpath approach is the best one to pursue.
Comment on attachment 97232 [details] Patch that uses @loader_path View in context: https://bugs.webkit.org/attachment.cgi?id=97232&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:26099 > + INSTALL_PATH = "$(INSTALL_PATH_$(REAL_PLATFORM_NAME))"; These should all be set in the .xcconfig file. > Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:93 > +/* Begin PBXCopyFilesBuildPhase section */ > + A6ED12C213A8787A002D845D /* Copy WebCoreTestSupport into Bundle */ = { > + isa = PBXCopyFilesBuildPhase; > + buildActionMask = 2147483647; > + dstPath = ""; > + dstSubfolderSpec = 6; > + files = ( > + A6ED12BD13A87876002D845D /* libWebCoreTestSupport.dylib in Copy WebCoreTestSupport into Bundle */, > + ); > + name = "Copy WebCoreTestSupport into Bundle"; > + runOnlyForDeploymentPostprocessing = 0; > + }; > +/* End PBXCopyFilesBuildPhase section */ I didn’t think this was necessary for
Comment on attachment 97230 [details] Patch that uses @rpath View in context: https://bugs.webkit.org/attachment.cgi?id=97230&action=review > Source/WebCore/WebCore.xcodeproj/project.pbxproj:26099 > + INSTALL_PATH = "$(INSTALL_PATH_$(REAL_PLATFORM_NAME))"; These should all be set in the .xcconfig file. It’s not clear to me why the Production one is set to this value either, when the others are set to @rpath. > Tools/DumpRenderTree/DumpRenderTree.xcodeproj/project.pbxproj:939 > + LD_RUNPATH_SEARCH_PATHS = "@executable_path/."; As I mentioned in an earlier comment, it’s almost never useful to use @executable_path instead of @loader_path. Given that I think it would be clearer to use @loader_path here. These should live in the .xcconfig file as well. > Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:98 > + A664BC6813A5F18F009A7B25 /* libWebCoreTestSupport.dylib */ = {isa = PBXFileReference; lastKnownFileType = "compiled.mach-o.dylib"; path = libWebCoreTestSupport.dylib; sourceTree = BUILT_PRODUCTS_DIR; }; It looks like you’ve added libWebCoreTestSupport.dylib to the Xcode project multiple times? > Tools/WebKitTestRunner/WebKitTestRunner.xcodeproj/project.pbxproj:535 > + LD_RUNPATH_SEARCH_PATHS = "@loader_path/../../.."; These should live in the .xcconfig file.
Created attachment 97293 [details] Patch
(In reply to comment #40) Thank you for teaching me the ways of dyld and pbxproj! Hopefully this patch better.
(In reply to comment #42) > (In reply to comment #40) > > Thank you for teaching me the ways of dyld and pbxproj! Hopefully this patch better. Hopefully this patch *is* better.
Comment on attachment 97293 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=97293&action=review > Source/WebCore/Configurations/WebCoreTestSupport.xcconfig:31 > -INSTALL_PATH = $(INSTALL_PATH_$(CONFIGURATION)); > -INSTALL_PATH_Debug = @executable_path; > -INSTALL_PATH_Release = $(INSTALL_PATH_Debug); > -INSTALL_PATH_Production = /usr/local/lib; > +INSTALL_PATH = @rpath; This isn’t correct. It will prevent the dylib from being copied to /usr/local/lib during production builds, which would make it impossible for DumpRenderTree and WebKitTestRunner to find it at build time. I think to make this work for production we want something like: LD_DYLIB_INSTALL_NAME = $(LD_DYLIB_INSTALL_NAME_$(CONFIGURATION)); LD_DYLIB_INSTALL_NAME_Debug = @rpath; LD_DYLIB_INSTALL_NAME_Release = $(LD_DYLIB_INSTALL_NAME_Debug); This will set the install name to @rpath for Debug and Release and leave it at its default for Production (e.g., the file path). I think we can skip setting INSTALL_PATH altogether since I believe /usr/local/lib is the default if it is unset. > Tools/DumpRenderTree/mac/Configurations/DumpRenderTree.xcconfig:25 > +LD_RUNPATH_SEARCH_PATHS = "@loader_path/."; Given the change I suggested above this rpath would be unnecessary for Production builds. I’m not sure that it’s useful to complicate this by conditionalizing our setting of LD_RUNPATH_SEARCH_PATHS though.
Created attachment 97541 [details] Patch
Comment on attachment 97541 [details] Patch Please take another look.
Created attachment 98312 [details] Incorporates mrowe's changes for Mac production builds.
Comment on attachment 98312 [details] Incorporates mrowe's changes for Mac production builds. Clearing flags on attachment: 98312 Committed r89530: <http://trac.webkit.org/changeset/89530>