Bug 185401 - The PDF context menu should not be created in the WebContent process.
Summary: The PDF context menu should not be created in the WebContent process.
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Per Arne Vollan
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-07 16:36 PDT by Per Arne Vollan
Modified: 2018-05-08 13:34 PDT (History)
5 users (show)

See Also:


Attachments
Patch (14.30 KB, patch)
2018-05-07 16:56 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (14.42 KB, patch)
2018-05-07 17:09 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews206 for win-future (12.73 MB, application/zip)
2018-05-07 19:41 PDT, Build Bot
no flags Details
Patch (14.90 KB, patch)
2018-05-08 11:21 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (14.93 KB, patch)
2018-05-08 11:31 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff
Patch (15.16 KB, patch)
2018-05-08 12:21 PDT, Per Arne Vollan
thorton: review+
Details | Formatted Diff | Diff
Patch (15.26 KB, patch)
2018-05-08 13:06 PDT, Per Arne Vollan
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Per Arne Vollan 2018-05-07 16:36:42 PDT
The menu should be created in the UI process.
Comment 1 Per Arne Vollan 2018-05-07 16:56:08 PDT
Created attachment 339771 [details]
Patch
Comment 2 Per Arne Vollan 2018-05-07 17:09:33 PDT
Created attachment 339774 [details]
Patch
Comment 3 Radar WebKit Bug Importer 2018-05-07 17:14:41 PDT
<rdar://problem/40043120>
Comment 4 Tim Horton 2018-05-07 17:28:11 PDT
Comment on attachment 339774 [details]
Patch

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

> Source/WebKit/Shared/mac/PDFContextMenu.h:28
> +#if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)

Why would we change the behavior of PDFKit based on whether this feature is enabled or not? Seems like this should just be the behavior of PDFPlugin, period.

> Source/WebKit/UIProcess/WebPageProxy.h:1029
> +#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)

None of these should be PLATFORM(MAC), they should all be ENABLE(PDFKIT_PLUGIN). And not ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:82
> +- (id) menuItem;

No space after )

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:572
> +    auto menuTarget = [[WKPDFMenuTarget alloc] init];
> +    auto nsMenu = [[NSMenu alloc] init];

Aren’t these leaking??

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:589
> +    _NSPopUpCarbonMenu3(nsMenu, nil, nil, contextMenu.m_point, -1, nil, 0, nil, NSPopUpMenuTypeContext, nil);

I’m a bit concerned how much different this is from WebContextMenuClient::showContextMenu. Can we share some of that? Or at least do the same thing, so we’re not so confused later?

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1582
> +            auto item = [nsMenu itemAtIndex:i];

Can’t NSMenus have NSMenu children? This might fall apart in that case.

> Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1592
> +        webFrame()->page()->sendSync(Messages::WebPageProxy::ShowPDFContextMenu(contextMenu), Messages::WebPageProxy::ShowPDFContextMenu::Reply(selectedIndex));

Do we usually block the Web Content process main thread on popup menus? Is that necessary?
Comment 5 Tim Horton 2018-05-07 17:41:29 PDT
(In reply to Tim Horton from comment #4)
> Comment on attachment 339774 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339774&action=review
> 
> > Source/WebKit/Shared/mac/PDFContextMenu.h:28
> > +#if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> 
> Why would we change the behavior of PDFKit based on whether this feature is
> enabled or not? Seems like this should just be the behavior of PDFPlugin,
> period.
> 
> > Source/WebKit/UIProcess/WebPageProxy.h:1029
> > +#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> 
> None of these should be PLATFORM(MAC), they should all be
> ENABLE(PDFKIT_PLUGIN). And not ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> 
> > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:82
> > +- (id) menuItem;
> 
> No space after )
> 
> > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:572
> > +    auto menuTarget = [[WKPDFMenuTarget alloc] init];
> > +    auto nsMenu = [[NSMenu alloc] init];
> 
> Aren’t these leaking??
> 
> > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:589
> > +    _NSPopUpCarbonMenu3(nsMenu, nil, nil, contextMenu.m_point, -1, nil, 0, nil, NSPopUpMenuTypeContext, nil);
> 
> I’m a bit concerned how much different this is from
> WebContextMenuClient::showContextMenu. Can we share some of that? Or at
> least do the same thing, so we’re not so confused later?
> 
> > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1582
> > +            auto item = [nsMenu itemAtIndex:i];
> 
> Can’t NSMenus have NSMenu children? This might fall apart in that case.

I guess not, it’s that you have an NSMenuItem child with a submenu. It doesn’t happen now but you should handle it gracefully.

> > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1592
> > +        webFrame()->page()->sendSync(Messages::WebPageProxy::ShowPDFContextMenu(contextMenu), Messages::WebPageProxy::ShowPDFContextMenu::Reply(selectedIndex));
> 
> Do we usually block the Web Content process main thread on popup menus? Is
> that necessary?
Comment 6 Build Bot 2018-05-07 19:41:37 PDT
Comment on attachment 339774 [details]
Patch

Attachment 339774 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7601845

New failing tests:
http/tests/security/contentSecurityPolicy/userAgentShadowDOM/allow-audio.html
Comment 7 Build Bot 2018-05-07 19:41:48 PDT
Created attachment 339791 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 8 Per Arne Vollan 2018-05-08 09:26:58 PDT
(In reply to Tim Horton from comment #4)
> Comment on attachment 339774 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=339774&action=review
> 
> > Source/WebKit/Shared/mac/PDFContextMenu.h:28
> > +#if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> 
> Why would we change the behavior of PDFKit based on whether this feature is
> enabled or not? Seems like this should just be the behavior of PDFPlugin,
> period.
> 
> > Source/WebKit/UIProcess/WebPageProxy.h:1029
> > +#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> 
> None of these should be PLATFORM(MAC), they should all be
> ENABLE(PDFKIT_PLUGIN). And not ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> 
> > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:82
> > +- (id) menuItem;
> 
> No space after )
> 
> > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:572
> > +    auto menuTarget = [[WKPDFMenuTarget alloc] init];
> > +    auto nsMenu = [[NSMenu alloc] init];
> 
> Aren’t these leaking??
> 
> > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:589
> > +    _NSPopUpCarbonMenu3(nsMenu, nil, nil, contextMenu.m_point, -1, nil, 0, nil, NSPopUpMenuTypeContext, nil);
> 
> I’m a bit concerned how much different this is from
> WebContextMenuClient::showContextMenu. Can we share some of that? Or at
> least do the same thing, so we’re not so confused later?
> 
> > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1582
> > +            auto item = [nsMenu itemAtIndex:i];
> 
> Can’t NSMenus have NSMenu children? This might fall apart in that case.
> 
> > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1592
> > +        webFrame()->page()->sendSync(Messages::WebPageProxy::ShowPDFContextMenu(contextMenu), Messages::WebPageProxy::ShowPDFContextMenu::Reply(selectedIndex));
> 
> Do we usually block the Web Content process main thread on popup menus? Is
> that necessary?

Yes, _NSPopUpCarbonMenu3 is blocking the main thread.

Thanks for reviewing! I will update the patch.
Comment 9 Per Arne Vollan 2018-05-08 11:21:38 PDT
Created attachment 339841 [details]
Patch
Comment 10 Per Arne Vollan 2018-05-08 11:31:36 PDT
Created attachment 339846 [details]
Patch
Comment 11 Per Arne Vollan 2018-05-08 11:47:51 PDT
(In reply to Tim Horton from comment #5)
> (In reply to Tim Horton from comment #4)
> > Comment on attachment 339774 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=339774&action=review
> > 
> > > Source/WebKit/Shared/mac/PDFContextMenu.h:28
> > > +#if ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> > 
> > Why would we change the behavior of PDFKit based on whether this feature is
> > enabled or not? Seems like this should just be the behavior of PDFPlugin,
> > period.
> > 
> > > Source/WebKit/UIProcess/WebPageProxy.h:1029
> > > +#if PLATFORM(MAC) && ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> > 
> > None of these should be PLATFORM(MAC), they should all be
> > ENABLE(PDFKIT_PLUGIN). And not ENABLE(WEBPROCESS_WINDOWSERVER_BLOCKING)
> > 
> > > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:82
> > > +- (id) menuItem;
> > 
> > No space after )
> > 
> > > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:572
> > > +    auto menuTarget = [[WKPDFMenuTarget alloc] init];
> > > +    auto nsMenu = [[NSMenu alloc] init];
> > 
> > Aren’t these leaking??
> > 
> > > Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:589
> > > +    _NSPopUpCarbonMenu3(nsMenu, nil, nil, contextMenu.m_point, -1, nil, 0, nil, NSPopUpMenuTypeContext, nil);
> > 
> > I’m a bit concerned how much different this is from
> > WebContextMenuClient::showContextMenu. Can we share some of that? Or at
> > least do the same thing, so we’re not so confused later?
> > 
> > > Source/WebKit/WebProcess/Plugins/PDF/PDFPlugin.mm:1582
> > > +            auto item = [nsMenu itemAtIndex:i];
> > 
> > Can’t NSMenus have NSMenu children? This might fall apart in that case.
> 
> I guess not, it’s that you have an NSMenuItem child with a submenu. It
> doesn’t happen now but you should handle it gracefully.
> 

I would like to handle submenus recursively, but since this is not required at the moment, do you think we could defer that to a follow-up patch?
Comment 12 Tim Horton 2018-05-08 11:58:32 PDT
Sure, what I meant was you should bail/skip the item if it’s a submenu.
Comment 13 Per Arne Vollan 2018-05-08 12:21:07 PDT
Created attachment 339855 [details]
Patch
Comment 14 Tim Horton 2018-05-08 12:30:17 PDT
Comment on attachment 339855 [details]
Patch

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

> Source/WebKit/Shared/mac/PDFContextMenu.h:32
> +    String m_title;

Public fields in structs don’t usually get m_ prefixes, right?

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:79
> +    id menuItem;

should this be “selectedMenuItem”? Also, ObjC members get a leading underscore.

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:81
> +- (instancetype)init;

This is implicit since you’re an NSObject.

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:89
> +    self = [super init];

if (!self)
    return nil;

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:570
> +    auto menuTarget = [[[WKPDFMenuTarget alloc] init] autorelease];
> +    auto nsMenu = [[[NSMenu alloc] init] autorelease];

RetainPtr & adoptNS are the preferred way to solve this problem

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:580
> +        auto nsItem = [[NSMenuItem alloc] init];

This one is a leak too?

> Source/WebKit/UIProcess/mac/WebPageProxyMac.mm:591
> +    NSWindow* window = m_pageClient.platformWindow();

ObjC stars go on the other side
Comment 15 Per Arne Vollan 2018-05-08 13:06:48 PDT
Created attachment 339859 [details]
Patch
Comment 16 Per Arne Vollan 2018-05-08 13:07:47 PDT
Comment on attachment 339859 [details]
Patch

Thanks for reviewing!
Comment 17 WebKit Commit Bot 2018-05-08 13:34:10 PDT
Comment on attachment 339859 [details]
Patch

Clearing flags on attachment: 339859

Committed r231510: <https://trac.webkit.org/changeset/231510>