RESOLVED FIXED 171024
[Win] REGRESSION(r215486): Windows Release build is broken
https://bugs.webkit.org/show_bug.cgi?id=171024
Summary [Win] REGRESSION(r215486): Windows Release build is broken
Brent Fulgham
Reported 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.
Attachments
Patch (1.55 KB, patch)
2017-04-19 18:09 PDT, Brent Fulgham
mark.lam: review-
Patch v2 (2.30 KB, patch)
2017-04-19 18:22 PDT, Brent Fulgham
no flags
Patch (1.51 KB, patch)
2017-04-20 12:28 PDT, Brent Fulgham
no flags
Radar WebKit Bug Importer
Comment 1 2017-04-19 18:08:11 PDT
Brent Fulgham
Comment 2 2017-04-19 18:09:25 PDT
Mark Lam
Comment 3 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.
Mark Lam
Comment 4 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.
Brent Fulgham
Comment 5 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?
Brent Fulgham
Comment 6 2017-04-19 18:22:30 PDT
Created attachment 307536 [details] Patch v2
Mark Lam
Comment 7 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.
WebKit Commit Bot
Comment 8 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>
WebKit Commit Bot
Comment 9 2017-04-19 21:37:02 PDT
All reviewed patches have been landed. Closing bug.
Fujii Hironori
Comment 10 2017-04-20 02:54:58 PDT
Reopened. Still broken.
Fujii Hironori
Comment 11 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>
Brent Fulgham
Comment 12 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.
Brent Fulgham
Comment 13 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.
Alexey Proskuryakov
Comment 14 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>
Brent Fulgham
Comment 15 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.
Brent Fulgham
Comment 16 2017-04-20 12:28:54 PDT
Mark Lam
Comment 17 2017-04-20 12:29:24 PDT
Comment on attachment 307613 [details] Patch r=me
Mark Lam
Comment 18 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.
WebKit Commit Bot
Comment 19 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>
WebKit Commit Bot
Comment 20 2017-04-20 13:15:00 PDT
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.