Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(212)

Issue 2920853005: Fix a crash related to detaching a frame embedding pepper plugin with selection (Closed)

Created:
3 years, 6 months ago by EhsanK
Modified:
3 years, 6 months ago
Reviewers:
yosin_UTC9, dcheng
CC:
blink-reviews, blink-reviews-frames_chromium.org, chromium-reviews
Target Ref:
refs/heads/master
Project:
chromium
Visibility:
Public.

Description

Fix a crash related to detaching a frame embedding pepper plugin with selection When a focused pepper plugin loses focus, RenderWidget will send an update for selection bounds. This could be due to pepper plugin's destruction. One way this could happen is that the frame embedding the plugin is detached. On the other hand, from https://codereview.chromium.org/2001083002 on FrameSelection is "explicitly managed" and the method FrameSelectino::IsAvailable() should be called before using the object. This currently is not respected in the implementation of some selection methods inside WebFrameWidgetImpl. Right now a frame loses focus half way through the detach process. Before losing focus the document of the frame is destroyed and there is a transient state were a focused (local) frame does not have a document. This causes issues when there is a request for selection information. Specifically, destroying the document removes Context from SelectionEditor and FrameSelection. Then when the frame detach leads to destruction of the plugin, RenderWidget decides to update SelectionBounds which will ask the focused frame for it. Since the detaching frame is till focused, its FrameSelection is used. However, before using the selection, WebFrameWidgetImpl does not verify if the selection is available (which means its document is alive). This CL will fix the issue by verifying that the selection is available before using either selection or SelectionEditor which will avoid the UAF explained above. BUG=727910 Review-Url: https://codereview.chromium.org/2920853005 Cr-Commit-Position: refs/heads/master@{#477136} Committed: https://chromium.googlesource.com/chromium/src/+/d83b1d819305cde5cb6ccd372b32690dd6bca78b

Patch Set 1 #

Patch Set 2 : Approach II: Verify selection is available before computing visible selection #

Patch Set 3 : nits #

Unified diffs Side-by-side diffs Delta from patch set Stats (+11 lines, -5 lines) Patch
M third_party/WebKit/Source/web/WebFrameWidgetImpl.cpp View 1 2 4 chunks +11 lines, -5 lines 0 comments Download

Messages

Total messages: 26 (18 generated)
EhsanK
Please take a look.
3 years, 6 months ago (2017-06-02 15:50:34 UTC) #11
dcheng
1) I guess this is porting the logic from WebViewImpl. Any idea if we can ...
3 years, 6 months ago (2017-06-02 16:25:47 UTC) #12
yosin_UTC9
Code change itself is good since they are copied from WebViewImpl. I don't know the ...
3 years, 6 months ago (2017-06-05 03:59:53 UTC) #13
EhsanK
On 2017/06/05 03:59:53, yosin_UTC9 wrote: > Code change itself is good since they are copied ...
3 years, 6 months ago (2017-06-05 12:36:32 UTC) #15
dcheng
We've been putting common logic into WebFrameWidgetBase. The CL itself LGTM; we can work on ...
3 years, 6 months ago (2017-06-05 18:27:05 UTC) #16
EhsanK
On 2017/06/05 18:27:05, dcheng wrote: > We've been putting common logic into WebFrameWidgetBase. > > ...
3 years, 6 months ago (2017-06-05 18:58:42 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2920853005/40001
3 years, 6 months ago (2017-06-05 20:33:20 UTC) #22
commit-bot: I haz the power
3 years, 6 months ago (2017-06-06 01:04:49 UTC) #26
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/d83b1d819305cde5cb6ccd372b32...

Powered by Google App Engine
This is Rietveld 408576698