[Win] REGRESSION(r215486): Windows Release build is broken

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Brent Fulgham]

Threading the isolatedWorld state through the event handling caused the Release build of Windows WebKit to become upset that it could not find Heap::vm() and Heap::mutatorFence.

This patch exposes these two methods so that the linker is happy.

[Radar WebKit Bug Importer]

<rdar://problem/31722618>

[Brent Fulgham]

Created attachment 307534 [details]
Patch

[Mark Lam]

Comment on attachment 307534 [details]
Patch

This is not right because these 2 methods are inlined methods.  There's nothing to link against.

[Mark Lam]

(In reply to Mark Lam from comment #3)
> Comment on attachment 307534 [details]
> Patch
> 
> This is not right because these 2 methods are inlined methods.  There's
> nothing to link against.

See HeapInlines.h.

[Brent Fulgham]

(In reply to Mark Lam from comment #4)
> (In reply to Mark Lam from comment #3)
> > Comment on attachment 307534 [details]
> > Patch
> > 
> > This is not right because these 2 methods are inlined methods.  There's
> > nothing to link against.
> 
> See HeapInlines.h.

So should WebKit/win/... sources be including HeapInlines.h, or is some JSC header supposed to be including it, and isn't?

[Brent Fulgham]

Created attachment 307536 [details]
Patch v2

[Mark Lam]

Comment on attachment 307536 [details]
Patch v2

r=me.  I think this is fine.  Alternatively, you can #include <JavaScriptCore/JSCInlines.h> which will include many inline goodies.  #include'ing HeapInlines.h is probably best for now.  JSCInlines.h is recommended if you need to include multiple *Inlines.h files.

[WebKit Commit Bot]

Comment on attachment 307536 [details]
Patch v2

Clearing flags on attachment: 307536

Committed r215551: <http://trac.webkit.org/changeset/215551>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Fujii Hironori]

Reopened. Still broken.

[Fujii Hironori]

This patch make Windows build happy. But, I'm not sure this is a right fix.

diff --git a/Source/WebCore/bindings/js/JSDOMGlobalObject.h b/Source/WebCore/bindings/js/JSDOMGlobalObject.h
index fb63f24..d07d17b 100644
--- a/Source/WebCore/bindings/js/JSDOMGlobalObject.h
+++ b/Source/WebCore/bindings/js/JSDOMGlobalObject.h
@@ -28,6 +28,7 @@
 
 #include "PlatformExportMacros.h"
 #include "WebCoreJSBuiltinInternals.h"
+#include <heap/HeapInlines.h>
 #include <heap/LockDuringMarking.h>
 #include <runtime/JSGlobalObject.h>
 #include <runtime/StructureInlines.h>

[Brent Fulgham]

(In reply to Fujii Hironori from comment #11)
> This patch make Windows build happy. But, I'm not sure this is a right fix.
> 
> diff --git a/Source/WebCore/bindings/js/JSDOMGlobalObject.h
> b/Source/WebCore/bindings/js/JSDOMGlobalObject.h
> index fb63f24..d07d17b 100644
> --- a/Source/WebCore/bindings/js/JSDOMGlobalObject.h
> +++ b/Source/WebCore/bindings/js/JSDOMGlobalObject.h
> @@ -28,6 +28,7 @@
>  
>  #include "PlatformExportMacros.h"
>  #include "WebCoreJSBuiltinInternals.h"
> +#include <heap/HeapInlines.h>
>  #include <heap/LockDuringMarking.h>
>  #include <runtime/JSGlobalObject.h>
>  #include <runtime/StructureInlines.h>

Another option might be to add "#include <JavaScriptCore/JSCInlines.h>" to the Source/WebKit/win/WebKitPrefix.h file. That would at least limit the change to the one platform that is unhappy.

I'm trying that locally.

[Brent Fulgham]

(In reply to Brent Fulgham from comment #12)
> Another option might be to add "#include <JavaScriptCore/JSCInlines.h>" to
> the Source/WebKit/win/WebKitPrefix.h file. That would at least limit the
> change to the one platform that is unhappy.
> 
> I'm trying that locally.

That doesn't work either.

I think I've figured it out; I believe the various AllInOne.cpp files are getting prematurely optimized by the compiler, and the inlines for these two methods are getting removed because it doesn't see them being used.

When WebKit.dll and TestWebCoreLib get built, they attempt to access them, but the defines have been optimized away.

I'm going to try adding the inline includes to the AllInOne files and see if that resolves it.

[Alexey Proskuryakov]

> Another option might be to add "#include <JavaScriptCore/JSCInlines.h>" to the Source/WebKit/win/WebKitPrefix.h file.

I think that the reason for having separate inline implementation files is that it's not desirable to include them everywhere (compilation time issues, I believe). We only want to include them from individual .cpp files, not from headers.

But, we do include them from some headers!

Source/WebCore//bindings/js/JSCustomXPathNSResolver.h:#include <runtime/JSCInlines.h>
Source/WebCore//bridge/c/c_utility.h:#include <runtime/JSCInlines.h>
Source/WebCore//bridge/jsc/BridgeJSC.h:#include <runtime/JSCInlines.h>
Source/WebCore//platform/SerializedPlatformRepresentation.h:#include <runtime/JSCInlines.h>

[Brent Fulgham]

(In reply to Fujii Hironori from comment #11)
> This patch make Windows build happy. But, I'm not sure this is a right fix.
> 
> diff --git a/Source/WebCore/bindings/js/JSDOMGlobalObject.h
> b/Source/WebCore/bindings/js/JSDOMGlobalObject.h
> index fb63f24..d07d17b 100644
> --- a/Source/WebCore/bindings/js/JSDOMGlobalObject.h
> +++ b/Source/WebCore/bindings/js/JSDOMGlobalObject.h
> @@ -28,6 +28,7 @@
>  
>  #include "PlatformExportMacros.h"
>  #include "WebCoreJSBuiltinInternals.h"
> +#include <heap/HeapInlines.h>
>  #include <heap/LockDuringMarking.h>
>  #include <runtime/JSGlobalObject.h>
>  #include <runtime/StructureInlines.h>

I can't get anything else to work, so let's go with this.

[Brent Fulgham]

Created attachment 307613 [details]
Patch

[Mark Lam]

Comment on attachment 307613 [details]
Patch

r=me

[Mark Lam]

Comment on attachment 307536 [details]
Patch v2

Un-obsoleting the previous patch because it was correct and landed.  We didn't revert it.

[WebKit Commit Bot]

Comment on attachment 307613 [details]
Patch

Clearing flags on attachment: 307613

Committed r215575: <http://trac.webkit.org/changeset/215575>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.