Bug 107036

Summary:

[GTK] Remove null check for WebPopupMenuProxy::Client in WebPopupMenuProxyGtk::showPopupMenu

Product:

WebKit

Reporter:

Zan Dobersek <zan>

Component:

WebKitGTK

Assignee:

Zan Dobersek <zan>

Status:

RESOLVED INVALID

Severity:

Normal

CC:

andersca, commit-queue, mrobinson, sam

Priority:

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+

Zan Dobersek

Reported 2013-01-16 11:07:45 PST

Based on a Coverity warning listed in bug #104114.

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 attachment proposed patch, testcase, etc.

Zan Dobersek

Comment 1 2013-01-16 11:40:25 PST

Created attachment 183014 [details] Patch

Martin Robinson

Comment 2 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?

Zan Dobersek

Comment 3 2013-01-16 12:03:07 PST

Comment on attachment 183014 [details] Patch No, this approach is incorrect.

Zan Dobersek

Comment 4 2013-01-20 05:10:56 PST

Created attachment 183666 [details] Patch

Zan Dobersek

Comment 5 2013-01-20 05:12:10 PST

This now deserves a better title.

Zan Dobersek

Comment 6 2013-01-20 05:13:32 PST

Created attachment 183667 [details] Patch

Martin Robinson

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

Zan Dobersek

Comment 8 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?

Martin Robinson

Comment 9 2013-01-23 08:56:28 PST

Okay. I guess we need a nod from a WebKit2 OWNER.

Zan Dobersek

Comment 10 2013-03-11 09:00:15 PDT

Adding WK2 owners to the CC list.

Zan Dobersek

Comment 11 2014-01-25 10:43:36 PST

Committed r162768: <http://trac.webkit.org/changeset/162768>

WebKit Commit Bot

Comment 12 2014-02-09 02:19:56 PST

Re-opened since this is blocked by bug 128495

Zan Dobersek

Comment 13 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.

Note You need to log in before you can comment on or make changes to this bug.