Bug 107036 - [GTK] Remove null check for WebPopupMenuProxy::Client in WebPopupMenuProxyGtk::showPopupMenu
Summary: [GTK] Remove null check for WebPopupMenuProxy::Client in WebPopupMenuProxyGtk...
Status: RESOLVED INVALID
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Zan Dobersek
URL:
Keywords: Gtk, LayoutTestFailure
Depends on: 128495
Blocks: 104114
  Show dependency tree / graph
 
Reported: 2013-01-16 11:07 PST by Zan Dobersek
Modified: 2014-02-09 02:24 PST (History)
4 users (show)

See Also:

Attachments
Patch (1.82 KB, patch)
2013-01-16 11:40 PST, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Patch (1.72 KB, patch)
2013-01-20 05:10 PST, Zan Dobersek 		no flags Details | Formatted Diff | Diff
Patch (1.73 KB, patch)
2013-01-20 05:13 PST, Zan Dobersek 		kling: review+
Details | Formatted Diff | Diff
Show Obsolete (2) View All Add an attachment (proposed patch, testcase, etc.)

Note You need to log in before you can comment on or make changes to this bug.
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.