Bug 171029 - [AppleWin] The procedure entry point ?waitForThreadCompletion@WTF@@YAHI@Z could not be located in the dynamic link library WebKitQuartzCoreAdditions.dll
Summary: [AppleWin] The procedure entry point ?waitForThreadCompletion@WTF@@YAHI@Z cou...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Yusuke Suzuki
URL:
Keywords: InRadar
Depends on: 170502
Blocks:
  Show dependency treegraph
 
Reported: 2017-04-19 18:54 PDT by Fujii Hironori
Modified: 2018-04-30 03:28 PDT (History)
11 users (show)

See Also:


Attachments
dumpbin /imports WebKitQuartzCoreAdditions.dll (7.83 KB, text/plain)
2017-04-24 01:09 PDT, Fujii Hironori
no flags Details
Patch (6.97 KB, patch)
2017-04-24 02:42 PDT, Yusuke Suzuki
bfulgham: review+
bfulgham: commit-queue-
Details | Formatted Diff | Diff
Patch (6.67 KB, patch)
2017-04-24 19:10 PDT, Yusuke Suzuki
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Fujii Hironori 2017-04-19 18:54:43 PDT
[AppleWin] The procedure entry point ?waitForThreadCompletion@WTF@@YAHI@Z could not be located in the dynamic link library WebKitQuartzCoreAdditions.dll

AppleWin port debug build trunk@215506
While starting MiniBrowser, a message box is shown.

> The procedure entry point ?waitForThreadCompletion@WTF@@YAHI@Z could not be located in the dynamic link library C:\Program Files (x86)\Common Files\Apple\Apple Application Support\WebKitQuartzCoreAdditions.dll

Pushing its OK button, MiniBrowser seems working fine.
waitForThreadCompletion was removed in Bug 170502.
Comment 1 Fujii Hironori 2017-04-19 18:59:46 PDT
Is Bug 25348 Comment 22 a similar issue?
Comment 2 Yusuke Suzuki 2017-04-20 03:01:16 PDT
Alex, can we update WebKitQuartzCoreAdditions.dll thing?
Or should we leave the binary compatible layer over the new threading mechanism?
It is ok to add such a layere if it is necessary.
Comment 3 Alex Christensen 2017-04-20 13:25:08 PDT
Not sure.  Brent has updated those libraries before.  I haven't.
We could add a stub waitForThreadCompletion for binary compatibility if it's no problem.
Comment 4 Fujii Hironori 2017-04-24 01:09:08 PDT
Created attachment 307960 [details]
dumpbin /imports WebKitQuartzCoreAdditions.dll

Here are the symbols imported from WTF.dll.
Comment 5 Yusuke Suzuki 2017-04-24 01:12:11 PDT
(In reply to Fujii Hironori from comment #4)
> Created attachment 307960 [details]
> dumpbin /imports WebKitQuartzCoreAdditions.dll
> 
> Here are the symbols imported from WTF.dll.

OK, let's create a workaround for Windows.
It seems that we need to have

+ waitForThreadCompletion
+ createThread

Ideally, in the next update, these things should be removed.
Comment 6 Yusuke Suzuki 2017-04-24 02:42:52 PDT
Created attachment 307962 [details]
Patch
Comment 7 Brent Fulgham 2017-04-24 08:48:10 PDT
Yes -- I'll try to get this fixed ASAP. The WebKitQuartzCoreAdditions relies on WTF for some of its threading functions, and after the API was changed they are no longer compatible.
Comment 8 Brent Fulgham 2017-04-24 09:46:31 PDT
Comment on attachment 307962 [details]
Patch

Please, no! Let's just fix the problem.
Comment 9 Radar WebKit Bug Importer 2017-04-24 09:49:06 PDT
<rdar://problem/31789014>
Comment 10 Brent Fulgham 2017-04-24 12:15:52 PDT
Comment on attachment 307962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307962&action=review

> Source/WTF/ChangeLog:9
> +        waitForThreadCompletion and createThread. This patch implements the both on the top of the new APIs.

This patch implements BOTH ON TOP of the new APIs.

> Source/WTF/wtf/ThreadHolder.cpp:-53
> -        threadSpecificSet(m_key, new ThreadHolder(thread));

Let's keep this ugly:
#if !OS(WINDOWS)
threadSpecificSet ...
#else
// FIXME: Remove this workaround code once <rdar://problem/31793213> is fixed.

> Source/WTF/wtf/ThreadHolder.cpp:55
> +        platformInitialize(holder);

#endif

> Source/WTF/wtf/ThreadHolder.h:78
> +    static void platformInitialize(ThreadHolder*);

I think this should be "#if OS(WINDOWS)", too.

> Source/WTF/wtf/ThreadHolderPthreads.cpp:52
> +}

Remove this whole thing.

> Source/WTF/wtf/ThreadHolderWin.cpp:72
>  

Add a comment:

"// FIXME: Remove this workaround code once <rdar://problem/31793213> is fixed."

> Source/WTF/wtf/Threading.h:219
> +// https://bugs.webkit.org/show_bug.cgi?id=171029

Remove this Bugzilla bug, since this is the bug where the workaround was added.

Instead, say: "// Remove this workaround code when <rdar://problem/31793213> is fixed."

> Source/WTF/wtf/ThreadingWin.cpp:514
>  

Add a comment:

"// FIXME: Remove this workaround code once <rdar://problem/31793213> is fixed."
Comment 11 Brent Fulgham 2017-04-24 12:17:16 PDT
I looked into it, and I can't fix this as easily as I had hoped, since WebKitQuartzCoreAdditions is installed with Apple Software, not with our own development Zip file.

So, we have to keep this backwards-compatibility code until new Apple software rolls out externally.

So I'm approving your patch, but would like you to add some comments to remind us to remove this when the relevant code ships.
Comment 12 Yusuke Suzuki 2017-04-24 19:09:16 PDT
Comment on attachment 307962 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=307962&action=review

OK, thanks!

>> Source/WTF/ChangeLog:9
>> +        waitForThreadCompletion and createThread. This patch implements the both on the top of the new APIs.
> 
> This patch implements BOTH ON TOP of the new APIs.

Fixed.

>> Source/WTF/wtf/ThreadHolder.cpp:-53
>> -        threadSpecificSet(m_key, new ThreadHolder(thread));
> 
> Let's keep this ugly:
> #if !OS(WINDOWS)
> threadSpecificSet ...
> #else
> // FIXME: Remove this workaround code once <rdar://problem/31793213> is fixed.

Fixed.

>> Source/WTF/wtf/ThreadHolder.cpp:55
>> +        platformInitialize(holder);
> 
> #endif

Fixed.

>> Source/WTF/wtf/ThreadHolder.h:78
>> +    static void platformInitialize(ThreadHolder*);
> 
> I think this should be "#if OS(WINDOWS)", too.

Fixed.

>> Source/WTF/wtf/ThreadHolderPthreads.cpp:52
>> +}
> 
> Remove this whole thing.

OK, dropped.

>> Source/WTF/wtf/ThreadHolderWin.cpp:72
>>  
> 
> Add a comment:
> 
> "// FIXME: Remove this workaround code once <rdar://problem/31793213> is fixed."

Added.

>> Source/WTF/wtf/Threading.h:219
>> +// https://bugs.webkit.org/show_bug.cgi?id=171029
> 
> Remove this Bugzilla bug, since this is the bug where the workaround was added.
> 
> Instead, say: "// Remove this workaround code when <rdar://problem/31793213> is fixed."

OK, fixed this part!

>> Source/WTF/wtf/ThreadingWin.cpp:514
>>  
> 
> Add a comment:
> 
> "// FIXME: Remove this workaround code once <rdar://problem/31793213> is fixed."

Added.
Comment 13 Yusuke Suzuki 2017-04-24 19:10:12 PDT
Created attachment 308045 [details]
Patch
Comment 14 Yusuke Suzuki 2017-04-24 19:15:55 PDT
Committed r215713: <http://trac.webkit.org/changeset/215713>
Comment 15 Yusuke Suzuki 2018-04-30 03:28:16 PDT
Hi, now, one year later, can we remove this workaround?