|
|
DescriptionFix 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 #Messages
Total messages: 26 (18 generated)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== 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. 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. Then when the frame detach leads to destruction of the plugin, RenderWidget decides to update SelectionBounds which will ask the focused frame for it. This leads to a call to SelectionEditor::GetDocument() which is nullptr at this time. This CL will fix the problem by removing focus from the frame as the first step in detaching process. This way the call to SelectionBounds returns empty and the UAF on GetDocument() is avoided. BUG=727910 ========== to ========== 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. 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 ==========
Description was changed from ========== 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. 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 ========== to ========== 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. 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 ==========
Description was changed from ========== 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. 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 ========== to ========== 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. 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 ==========
ekaramad@chromium.org changed reviewers: + dcheng@chromium.org, yosin@chromium.org
Please take a look.
1) I guess this is porting the logic from WebViewImpl. Any idea if we can merge these blocks soon? It seems hard to maintain two separate blocks of code like this to keep them in sync. 2) Any chance we can update layout tests to cover this case as well?
Code change itself is good since they are copied from WebViewImpl. I don't know the plan of unification of WebViewImpl and WebFrameWidgetImpl or removing both of them. Introducing WebFrameSelectionImpl class, containment pattern, to share code between WebViewImpl and WebFrameWidgetImpl for short term solution? class WebViewImpl { ... WebFrameSelecitonImpl selection_; } WebViewIMpl::SelectionBounds() { selection_.SelectionBounds(); } BTW, could you refer patch[1] in the description for ease of tracking origin of checking selection.IsActive()? I took 11 "git blame" to reach it.
Description was changed from ========== 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. 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 ========== to ========== 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 ==========
On 2017/06/05 03:59:53, yosin_UTC9 wrote: > Code change itself is good since they are copied from WebViewImpl. > > I don't know the plan of unification of WebViewImpl and WebFrameWidgetImpl or > removing > both of them. > > Introducing WebFrameSelectionImpl class, containment pattern, to share code > between WebViewImpl > and WebFrameWidgetImpl for short term solution? It sounds good and I can work on it if need be. But in case it is supposed to be a very short term solution, we might as well consider moving the implementation to WebFrameWidgetImpl. Then we can either: 1- Get all callers use WebFrameWidget, e.g., if (GetWebWidget()->IsWebFrameWidget()) return static_cast<blink::WebFrameWidge*>(GetWebWidget())->SelectionBounds(); 2- For WebViewImpl, call the method on MainFrameImpl()->FrameWidget(). I can work on either approach (follow up patch?). > class WebViewImpl { ... WebFrameSelecitonImpl selection_; } > WebViewIMpl::SelectionBounds() { selection_.SelectionBounds(); } > > > BTW, could you refer patch[1] in the description for ease of tracking origin of > checking > selection.IsActive()? I took 11 "git blame" to reach it. I did. Thanks for the suggestion.
We've been putting common logic into WebFrameWidgetBase. The CL itself LGTM; we can work on unification in a followup
On 2017/06/05 18:27:05, dcheng wrote: > We've been putting common logic into WebFrameWidgetBase. > > The CL itself LGTM; we can work on unification in a followup Thanks! I was planning on moving all these to WebFrameWidgetImpl (after this change lands: https://codereview.chromium.org/2910233002/ - although I don't think I am blocked on it). Since this is causes crashes I will try to land it but will soon submit a test (if possible).
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496694780784040, "parent_rev": "208145bd191e5d51595c96d4173a0c6f4513435b", "commit_rev": "6ea90d2c66c92b3cf245679f770764b5f5b41407"}
CQ is committing da patch. Bot data: {"patchset_id": 40001, "attempt_start_ts": 1496694780784040, "parent_rev": "b89e40c6c7ed40a55cefb0cba6f0c32bed098af4", "commit_rev": "d83b1d819305cde5cb6ccd372b32690dd6bca78b"}
Message was sent while issue was closed.
Description was changed from ========== 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 ========== to ========== 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/+/d83b1d819305cde5cb6ccd372b32... ==========
Message was sent while issue was closed.
Committed patchset #3 (id:40001) as https://chromium.googlesource.com/chromium/src/+/d83b1d819305cde5cb6ccd372b32... |