Bug 107036

Summary: [GTK] Remove null check for WebPopupMenuProxy::Client in WebPopupMenuProxyGtk::showPopupMenu
Product: WebKit Reporter: Zan Dobersek <zan>
Component: WebKitGTKAssignee: Zan Dobersek <zan>
Status: RESOLVED INVALID    
Severity: Normal CC: andersca, commit-queue, mrobinson, sam
Priority: P2 Keywords: Gtk, LayoutTestFailure
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on: 128495    
Bug Blocks: 104114    
Attachments:
Description Flags
Patch
none
Patch
none
Patch kling: review+

Description Zan Dobersek 2013-01-16 11:07:45 PST
Based on a Coverity warning listed in bug #104114.
Comment 1 Zan Dobersek 2013-01-16 11:40:25 PST
Created attachment 183014 [details]
Patch
Comment 2 Martin Robinson 2013-01-16 11:45:31 PST
Comment on attachment 183014 [details]
Patch

Hrm. Is this correct? Could it be possible for m_client to become null during the execution of the event loop?
Comment 3 Zan Dobersek 2013-01-16 12:03:07 PST
Comment on attachment 183014 [details]
Patch

No, this approach is incorrect.
Comment 4 Zan Dobersek 2013-01-20 05:10:56 PST
Created attachment 183666 [details]
Patch
Comment 5 Zan Dobersek 2013-01-20 05:12:10 PST
This now deserves a better title.
Comment 6 Zan Dobersek 2013-01-20 05:13:32 PST
Created attachment 183667 [details]
Patch
Comment 7 Martin Robinson 2013-01-21 08:14:45 PST
Comment on attachment 183667 [details]
Patch

So you're positive that the client cannot be invalidated by running the main loop?
Comment 8 Zan Dobersek 2013-01-21 12:36:27 PST
(In reply to comment #7)
> (From update of attachment 183667 [details])
> So you're positive that the client cannot be invalidated by running the main loop?

There are three places where the popup can get invalidated.
1. http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/WebPageProxy.cpp#L3065
This invalidation happens after the hidePopupMenu is called on the WebPopupMenuProxy. This call pops down the actual menu, which causes the unmap signal to be handled - there, the loop of the popup menu proxy is stopped and the selected item (if any) is signaled to the client. Only after that is the popup menu proxy invalidated.

2. http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/WebPageProxy.cpp#L3083
This invalidation happens after the call to the showPopupMenu method on the WebPopupMenuProxy returns. This method runs a loop until a selection is made in the popup menu or the popup menu is unmapped. After the loop is stopped (and still in the showPopupMenu) the selected item (if any) is signaled to the client. Again, only after that is the popup menu proxy invalidated.

3. http://trac.webkit.org/browser/trunk/Source/WebKit2/UIProcess/WebPageProxy.cpp#L3094
Same as #1.

So I believe the current code cannot cause the invalidation while the loop is running.
I guess a WebKit2 owner is required to look at this?
Comment 9 Martin Robinson 2013-01-23 08:56:28 PST
Okay. I guess we need a nod from a WebKit2 OWNER.
Comment 10 Zan Dobersek 2013-03-11 09:00:15 PDT
Adding WK2 owners to the CC list.
Comment 11 Zan Dobersek 2014-01-25 10:43:36 PST
Committed r162768: <http://trac.webkit.org/changeset/162768>
Comment 12 WebKit Commit Bot 2014-02-09 02:19:56 PST
Re-opened since this is blocked by bug 128495
Comment 13 Zan Dobersek 2014-02-09 02:24:29 PST
The popup menu proxy can be invalidated through the HidePopupMenu message from the WebKitWebProces before it exits the run loop. Because of that the early return is actually needed.