Bug 61073 – Teach WebKit2 about window.internals

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[WebKit Review Bot]

Teach WebKit2 about window.internals
Requested by dglazkov on #webkit.

[Dimitri Glazkov (Google)]

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?

[Dimitri Glazkov (Google)]

Created attachment 94474 [details]
WIP:Straight-shot approach (does not work)

[Darin Adler]

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.

[Dimitri Glazkov (Google)]

(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!

[Mark Rowe (bdash)]

I believe the distinction is that WebKit reexports WebCore’s symbols, while WebKit2 doesn’t (and shouldn’t).

[Darin Adler]

(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.

[Dimitri Glazkov (Google)]

Should I try to make WTF_EXPORT build on Mac?:

http://trac.webkit.org/browser/trunk/Source/JavaScriptCore/wtf/ExportMacros.h

[Mark Rowe (bdash)]

(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.

[Dimitri Glazkov (Google)]

>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?

[Dimitri Glazkov (Google)]

Created attachment 94713 [details]
IT WORKS

[Dimitri Glazkov (Google)]

(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.

[Dimitri Glazkov (Google)]

Created attachment 95807 [details]
To cook on bots

[Dimitri Glazkov (Google)]

Need Win support before can land.

[Dimitri Glazkov (Google)]

Created attachment 96135 [details]
Patch

[Dimitri Glazkov (Google)]

Comment on attachment 96135 [details]
Patch

Still need Qt support to land this.

[Dimitri Glazkov (Google)]

Dominic, how hard would it be to make this patch work on Win?

[Dominic Cooney]

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.

[Dominic Cooney]

Created attachment 96550 [details]
Bot chow

This combines attachment 96135 [details] and attachment 96548 [details]. I want to bounce this off the try bots.

[Dimitri Glazkov (Google)]

(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.

[Dominic Cooney]

Do you want to gussy this up with a ChangeLog or should I?

[Dimitri Glazkov (Google)]

(In reply to comment #20)
> Do you want to gussy this up with a ChangeLog or should I?

Go for it!

[Dominic Cooney]

Created attachment 96906 [details]
Patch

[Hajime Morrita]

Comment on attachment 96906 [details]
Patch

Rubber-stamped by me. Let' use the commit queue.

[WebKit Review Bot]

Comment on attachment 96906 [details]
Patch

Clearing flags on attachment: 96906

Committed r88625: <http://trac.webkit.org/changeset/88625>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hajime Morrita]

Reverted r88625 for reason:

Breaks SL Webkit2 Tests

Committed r88630: <http://trac.webkit.org/changeset/88630>

[Dominic Cooney]

Created attachment 96938 [details]
Patch

[Dominic Cooney]

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.

[Dimitri Glazkov (Google)]

Mark, what do you think?

[Dominic Cooney]

Created attachment 97065 [details]
Patch

[Dominic Cooney]

Comment on attachment 97065 [details]
Patch

I found this needed to be updated for qt-wk2.

[Mark Rowe (bdash)]

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).

[Mark Rowe (bdash)]

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.

[Dominic Cooney]

Created attachment 97228 [details]
Patch

[Dominic Cooney]

Created attachment 97230 [details]
Patch that uses @rpath

[Dominic Cooney]

Created attachment 97232 [details]
Patch that uses @loader_path

[Dominic Cooney]

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…

[Mark Rowe (bdash)]

(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.

[Mark Rowe (bdash)]

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

[Mark Rowe (bdash)]

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.

[Dominic Cooney]

Created attachment 97293 [details]
Patch

[Dominic Cooney]

(In reply to comment #40)

Thank you for teaching me the ways of dyld and pbxproj! Hopefully this patch better.

[Dominic Cooney]

(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.

[Mark Rowe (bdash)]

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.

[Dominic Cooney]

Created attachment 97541 [details]
Patch

[Dominic Cooney]

Comment on attachment 97541 [details]
Patch

Please take another look.

[Dominic Cooney]

Created attachment 98312 [details]
Incorporates mrowe's changes for Mac production builds.

[WebKit Review Bot]

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>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.