Bug 60595 - Mac WebKit2 WebProcess needs a shim to make prompts appear to be from the UIProcess
Summary: Mac WebKit2 WebProcess needs a shim to make prompts appear to be from the UIP...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh All
: P2 Normal
Assignee: Brady Eidson
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2011-05-10 16:10 PDT by Brady Eidson
Modified: 2011-06-18 11:56 PDT (History)
7 users (show)

See Also:


Attachments
Patch v1 (29.58 KB, patch)
2011-05-10 16:15 PDT, Brady Eidson
beidson: review-
Details | Formatted Diff | Diff
Patch #1 - Add/initialize empty shim (29.74 KB, patch)
2011-05-17 09:57 PDT, Brady Eidson
andersca: review+
Details | Formatted Diff | Diff
Patch #2 - Add stubs for the first wave of shimmed methods, calling through to the actual functions for now. (7.00 KB, patch)
2011-05-17 10:28 PDT, Brady Eidson
andersca: review+
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch #3 - Add CoreIPC stuff for when we marshall to the UIProcess and back (23.87 KB, patch)
2011-05-17 10:50 PDT, Brady Eidson
andersca: review+
Details | Formatted Diff | Diff
Patch #4 - Implement the shim, and have the UIProcess do the work. (18.89 KB, patch)
2011-05-17 11:33 PDT, Brady Eidson
andersca: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brady Eidson 2011-05-10 16:10:12 PDT
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>
Comment 1 Brady Eidson 2011-05-10 16:15:29 PDT
Created attachment 93037 [details]
Patch v1
Comment 2 Brady Eidson 2011-05-17 09:42:29 PDT
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.
Comment 3 Brady Eidson 2011-05-17 09:57:30 PDT
Created attachment 93783 [details]
Patch #1 - Add/initialize empty shim
Comment 4 mitz 2011-05-17 10:03:08 PDT
I think this is bug 51599 (or blocking that bug).
Comment 5 Anders Carlsson 2011-05-17 10:05:26 PDT
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.
Comment 6 Brady Eidson 2011-05-17 10:16:08 PDT
(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
Comment 7 Brady Eidson 2011-05-17 10:28:20 PDT
Created attachment 93790 [details]
Patch #2 - Add stubs for the first wave of shimmed methods, calling through to the actual functions for now.
Comment 8 WebKit Review Bot 2011-05-17 10:35:00 PDT
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
Comment 9 Brady Eidson 2011-05-17 10:37:52 PDT
Will make sure I fix that build error before landing - I think SnowLeopard just needs an include there.
Comment 10 Brady Eidson 2011-05-17 10:40:10 PDT
Yup, worked for me locally.

Landed in r86686
Comment 11 Brady Eidson 2011-05-17 10:50:10 PDT
Created attachment 93796 [details]
Patch #3 - Add CoreIPC stuff for when we marshall to the UIProcess and back
Comment 12 WebKit Review Bot 2011-05-17 10:52:41 PDT
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 13 Anders Carlsson 2011-05-17 10:55:11 PDT
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).
Comment 14 Brady Eidson 2011-05-17 11:02:45 PDT
(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...
Comment 15 Brady Eidson 2011-05-17 11:07:09 PDT
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
Comment 16 Brady Eidson 2011-05-17 11:33:03 PDT
Created attachment 93802 [details]
Patch #4 - Implement the shim, and have the UIProcess do the work.
Comment 17 Anders Carlsson 2011-05-17 11:38:48 PDT
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 18 Sam Weinig 2011-05-17 11:50:56 PDT
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.
Comment 19 Ryosuke Niwa 2011-05-17 16:15:24 PDT
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.
Comment 20 Brady Eidson 2011-05-17 16:26:02 PDT
(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.
Comment 21 Brady Eidson 2011-05-17 16:48:36 PDT
BTW, forgot to mention:

Part #4 landed in r86692
Comment 22 Brady Eidson 2011-05-17 17:24:37 PDT
It's QTKitServer crashing, and it's crashing because we're trying to install the WebProcessShim when we launch it.
Comment 23 Brady Eidson 2011-05-17 17:30:16 PDT
(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