Bug 185401 – The PDF context menu should not be created in the WebContent process.

• Component: WebKit2
• Status: NEW

[Per Arne Vollan]

The menu should be created in the UI process.

[Per Arne Vollan]

Created attachment 339771 [details]
Patch

[Per Arne Vollan]

Created attachment 339774 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/40043120>

[Tim Horton]

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?

[Tim Horton]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Per Arne Vollan]

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

[Per Arne Vollan]

Created attachment 339841 [details]
Patch

[Per Arne Vollan]

Created attachment 339846 [details]
Patch

[Per Arne Vollan]

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

[Tim Horton]

Sure, what I meant was you should bail/skip the item if it’s a submenu.

[Per Arne Vollan]

Created attachment 339855 [details]
Patch

[Tim Horton]

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

[Per Arne Vollan]

Created attachment 339859 [details]
Patch

[Per Arne Vollan]

Comment on attachment 339859 [details]
Patch

Thanks for reviewing!

[WebKit Commit Bot]

Comment on attachment 339859 [details]
Patch

Clearing flags on attachment: 339859

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