Bug 60595 – Mac WebKit2 WebProcess needs a shim to make prompts appear to be from the UIProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

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>

[Brady Eidson]

Created attachment 93037 [details]
Patch v1

[Brady Eidson]

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.

[Brady Eidson]

Created attachment 93783 [details]
Patch #1 - Add/initialize empty shim

I think this is bug 51599 (or blocking that bug).

[Anders Carlsson]

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.

[Brady Eidson]

(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

[Brady Eidson]

Created attachment 93790 [details]
Patch #2 - Add stubs for the first wave of shimmed methods, calling through to the actual functions for now.

[WebKit Review Bot]

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

[Brady Eidson]

Will make sure I fix that build error before landing - I think SnowLeopard just needs an include there.

[Brady Eidson]

Yup, worked for me locally.

Landed in r86686

[Brady Eidson]

Created attachment 93796 [details]
Patch #3 - Add CoreIPC stuff for when we marshall to the UIProcess and back

[WebKit Review Bot]

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.

[Anders Carlsson]

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).

[Brady Eidson]

(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...

[Brady Eidson]

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

[Brady Eidson]

Created attachment 93802 [details]
Patch #4 - Implement the shim, and have the UIProcess do the work.

[Anders Carlsson]

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.

[Sam Weinig]

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.

[Ryosuke Niwa]

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.

[Brady Eidson]

(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.

[Brady Eidson]

BTW, forgot to mention:

Part #4 landed in r86692

[Brady Eidson]

It's QTKitServer crashing, and it's crashing because we're trying to install the WebProcessShim when we launch it.

[Brady Eidson]

(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