Bug 171024

Summary: [Win] REGRESSION(r215486): Windows Release build is broken
Product: WebKit Reporter: Brent Fulgham <bfulgham>
Component: JavaScriptCoreAssignee: Brent Fulgham <bfulgham>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, bfulgham, buildbot, commit-queue, Hironori.Fujii, keith_miller, mark.lam, msaboff, pvollan, ryanhaddad, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 65589    
Bug Blocks:    
Attachments:
Description Flags
Patch
mark.lam: review-
Patch v2
none
Patch none

Description Brent Fulgham 2017-04-19 18:07:26 PDT
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.
Comment 1 Radar WebKit Bug Importer 2017-04-19 18:08:11 PDT
<rdar://problem/31722618>
Comment 2 Brent Fulgham 2017-04-19 18:09:25 PDT
Created attachment 307534 [details]
Patch
Comment 3 Mark Lam 2017-04-19 18:12:24 PDT
Comment on attachment 307534 [details]
Patch

This is not right because these 2 methods are inlined methods.  There's nothing to link against.
Comment 4 Mark Lam 2017-04-19 18:12:41 PDT
(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.
Comment 5 Brent Fulgham 2017-04-19 18:16:13 PDT
(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?
Comment 6 Brent Fulgham 2017-04-19 18:22:30 PDT
Created attachment 307536 [details]
Patch v2
Comment 7 Mark Lam 2017-04-19 19:26:42 PDT
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.
Comment 8 WebKit Commit Bot 2017-04-19 21:37:00 PDT
Comment on attachment 307536 [details]
Patch v2

Clearing flags on attachment: 307536

Committed r215551: <http://trac.webkit.org/changeset/215551>
Comment 9 WebKit Commit Bot 2017-04-19 21:37:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 10 Fujii Hironori 2017-04-20 02:54:58 PDT
Reopened. Still broken.
Comment 11 Fujii Hironori 2017-04-20 02:56:53 PDT
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>
Comment 12 Brent Fulgham 2017-04-20 08:41:22 PDT
(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.
Comment 13 Brent Fulgham 2017-04-20 08:49:41 PDT
(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.
Comment 14 Alexey Proskuryakov 2017-04-20 09:33:58 PDT
> 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>
Comment 15 Brent Fulgham 2017-04-20 12:24:57 PDT
(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.
Comment 16 Brent Fulgham 2017-04-20 12:28:54 PDT
Created attachment 307613 [details]
Patch
Comment 17 Mark Lam 2017-04-20 12:29:24 PDT
Comment on attachment 307613 [details]
Patch

r=me
Comment 18 Mark Lam 2017-04-20 12:30:23 PDT
Comment on attachment 307536 [details]
Patch v2

Un-obsoleting the previous patch because it was correct and landed.  We didn't revert it.
Comment 19 WebKit Commit Bot 2017-04-20 13:14:58 PDT
Comment on attachment 307613 [details]
Patch

Clearing flags on attachment: 307613

Committed r215575: <http://trac.webkit.org/changeset/215575>
Comment 20 WebKit Commit Bot 2017-04-20 13:15:00 PDT
All reviewed patches have been landed.  Closing bug.