WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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-
Details
Formatted Diff
Diff
Patch v2
(2.30 KB, patch)
2017-04-19 18:22 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Patch
(1.51 KB, patch)
2017-04-20 12:28 PDT
,
Brent Fulgham
no flags
Details
Formatted Diff
Diff
Show Obsolete
(1)
View All
Add attachment
proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1
2017-04-19 18:08:11 PDT
<
rdar://problem/31722618
>
Brent Fulgham
Comment 2
2017-04-19 18:09:25 PDT
Created
attachment 307534
[details]
Patch
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
Created
attachment 307613
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug