For future work, Mac WebKit2 WebProcess needs a shim. It makes sense to land a patch of project file and empty implementation voodoo here in a separate patch. In radar as <rdar://problem/8814289>
Created attachment 93037 [details] Patch v1
Retitling to "Mac WebKit2 WebProcess needs a shim to make prompts appear to be from the UIProcess", and staged patches will come through here today.
Created attachment 93783 [details] Patch #1 - Add/initialize empty shim
I think this is bug 51599 (or blocking that bug).
Comment on attachment 93783 [details] Patch #1 - Add/initialize empty shim View in context: https://bugs.webkit.org/attachment.cgi?id=93783&action=review > Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:148 > + else To be future proof, this should check for a WebProcess process type.
(In reply to comment #5) > (From update of attachment 93783 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93783&action=review > > > Source/WebKit2/UIProcess/Launcher/mac/ProcessLauncherMac.mm:148 > > + else > > To be future proof, this should check for a WebProcess process type. Done. Landed in r86678
Created attachment 93790 [details] Patch #2 - Add stubs for the first wave of shimmed methods, calling through to the actual functions for now.
Comment on attachment 93790 [details] Patch #2 - Add stubs for the first wave of shimmed methods, calling through to the actual functions for now. Attachment 93790 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/8709216
Will make sure I fix that build error before landing - I think SnowLeopard just needs an include there.
Yup, worked for me locally. Landed in r86686
Created attachment 93796 [details] Patch #3 - Add CoreIPC stuff for when we marshall to the UIProcess and back
Attachment 93796 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit..." exit_code: 1 Source/WebKit2/Shared/mac/SecItemRequestData.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4] Source/WebKit2/Shared/mac/SecItemResponseData.h:32: Code inside a namespace should not be indented. [whitespace/indent] [4] Total errors found: 2 in 8 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 93796 [details] Patch #3 - Add CoreIPC stuff for when we marshall to the UIProcess and back View in context: https://bugs.webkit.org/attachment.cgi?id=93796&action=review > Source/WebKit2/Shared/mac/SecItemRequestData.cpp:71 > + if (!expectAttributes) > + return true; > + > + if (!CoreIPC::decode(decoder, secItemRequestData.m_attributesToMatch)) > + return false; Could you change this to if (expectAttributes && !CoreIPC::decode(decoder, secItemRequestData.m_attributesToMatch) return false; We prefer having all the if statements in decoders return false and then a single return true at the end. > Source/WebKit2/Shared/mac/SecItemResponseData.h:46 > + CFTypeRef leakResultObject() { return m_resultObject.leakRef(); } Any reason why this couldn't return a RetainPtr<CFTypeRef> instead? (and be called releaseResultObject).
(In reply to comment #13) > (From update of attachment 93796 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93796&action=review > > > Source/WebKit2/Shared/mac/SecItemRequestData.cpp:71 > > + if (!expectAttributes) > > + return true; > > + > > + if (!CoreIPC::decode(decoder, secItemRequestData.m_attributesToMatch)) > > + return false; > > Could you change this to > > if (expectAttributes && !CoreIPC::decode(decoder, secItemRequestData.m_attributesToMatch) > return false; > > We prefer having all the if statements in decoders return false and then a single return true at the end. Yup! > > > Source/WebKit2/Shared/mac/SecItemResponseData.h:46 > > + CFTypeRef leakResultObject() { return m_resultObject.leakRef(); } > > Any reason why this couldn't return a RetainPtr<CFTypeRef> instead? (and be called releaseResultObject). No reason. I'll do it. I really want to get around to my "add PassRetainPtr<>" spare-time project one of these days...
I went with just returning a RetainPtr and not clearing the member variable - not really important to do so. This is an opportunity for cleanup once we have PassRetainPtr<> Landed in r86688
Created attachment 93802 [details] Patch #4 - Implement the shim, and have the UIProcess do the work.
Comment on attachment 93802 [details] Patch #4 - Implement the shim, and have the UIProcess do the work. View in context: https://bugs.webkit.org/attachment.cgi?id=93802&action=review > Source/WebKit2/WebProcess/mac/WebProcessMac.mm:275 > + OwnPtr<SecItemAPIContext> context = adoptPtr(new SecItemAPIContext); I think you can allocate the SecItemAPIContext on the stack here. > Source/WebKit2/WebProcess/mac/WebProcessMac.mm:303 > + OwnPtr<SecItemAPIContext> context = adoptPtr(new SecItemAPIContext); Ditto. > Source/WebKit2/WebProcess/mac/WebProcessMac.mm:330 > + OwnPtr<SecItemAPIContext> context = adoptPtr(new SecItemAPIContext); Ditto. > Source/WebKit2/WebProcess/mac/WebProcessMac.mm:356 > + OwnPtr<SecItemAPIContext> context = adoptPtr(new SecItemAPIContext); Ditto.
Comment on attachment 93802 [details] Patch #4 - Implement the shim, and have the UIProcess do the work. View in context: https://bugs.webkit.org/attachment.cgi?id=93802&action=review > Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm:39 > + CFTypeRef resultObject; > + OSStatus resultCode; There is no reason to declare this early. > Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm:50 > + CFTypeRef resultObject; > + OSStatus resultCode; There is no reason to declare this early. > Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm:61 > + OSStatus resultCode; There is no reason to declare this early. > Source/WebKit2/UIProcess/mac/WebProcessProxyMac.mm:71 > + OSStatus resultCode; There is no reason to declare this early. > Source/WebKit2/WebProcess/mac/WebProcessMac.mm:250 > +struct SecItemAPIContext > +{ { on the wrong line. > Source/WebKit2/WebProcess/mac/WebProcessMac.mm:287 > + SecItemAPIContext* context = (SecItemAPIContext*)voidContext; We don't generally use c-style casts.
A bunch of tests are crashing on WebKit2 bots after this change: http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28WebKit2%20Tests%29/builds/11734 with the following error: dyld: could not load inserted library: /Volumes/Big/WebKit-BuildSlave/snowleopard-intel-release-tests-wk2/build/WebKitBuild/Release/WebKit2.framework/WebProcess.app/Contents/MacOS/WebProcessShim.dylib I'm not sure if this is due to this changeset or the bot is being flaky.
(In reply to comment #19) > A bunch of tests are crashing on WebKit2 bots after this change: > http://build.webkit.org/builders/SnowLeopard%20Intel%20Release%20%28WebKit2%20Tests%29/builds/11734 > > with the following error: > dyld: could not load inserted library: /Volumes/Big/WebKit-BuildSlave/snowleopard-intel-release-tests-wk2/build/WebKitBuild/Release/WebKit2.framework/WebProcess.app/Contents/MacOS/WebProcessShim.dylib > > I'm not sure if this is due to this changeset or the bot is being flaky. Well, these changesets introduced the concept of the WebProcessShim.dylib. But why it's not getting built on the bots is an unanswered question. Exploring.
BTW, forgot to mention: Part #4 landed in r86692
It's QTKitServer crashing, and it's crashing because we're trying to install the WebProcessShim when we launch it.
(In reply to comment #22) > It's QTKitServer crashing, and it's crashing because we're trying to install the WebProcessShim when we launch it. While I continue to explore the real fix to this, the tests should be back up and running after the temporary fix that is r86724