Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2346863004:
Speculative fix for OnSwapCompositorFrame crasher. (Closed)
DescriptionSpeculative fix for OnSwapCompositorFrame crasher.
Speculative fix for crasher in RenderWidgetHostViewGuest::
OnSwapCompositorFrame(), in case it is caused by guest_ being null.
BUG=646736
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/fb98885220ad4096b09634f154aa4907fda603ba
Cr-Commit-Position: refs/heads/master@{#419586}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Don't try to set surface if guest is destructing. #
Total comments: 1
Messages
Total messages: 33 (18 generated)
Description was changed from ========== Speculative fix for OnSwapCompositorFrame crasher. Speculative fix for crasher in RenderWidgetHostViewGuest:: OnSwapCompositorFrame(), in case it is caused by guest_ being null. BUG=646736 ========== to ========== Speculative fix for OnSwapCompositorFrame crasher. Speculative fix for crasher in RenderWidgetHostViewGuest:: OnSwapCompositorFrame(), in case it is caused by guest_ being null. BUG=646736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by wjmaclean@chromium.org to run a CQ dry run
Description was changed from ========== Speculative fix for OnSwapCompositorFrame crasher. Speculative fix for crasher in RenderWidgetHostViewGuest:: OnSwapCompositorFrame(), in case it is caused by guest_ being null. BUG=646736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Speculative fix for OnSwapCompositorFrame crasher. Speculative fix for crasher in RenderWidgetHostViewGuest:: OnSwapCompositorFrame(), in case it is caused by guest_ being null. BUG=646736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
wjmaclean@chromium.org changed reviewers: + kenrb@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
** tiny cl ** kenrb@ - I'd be interested in your thoughts on this ... not sure if the crasher is really a null guest_, but seems worth trying (or at the very least harmless?).
While a null check there might be reasonable, since there is a null check earlier in the method and in several other places in the file, it doesn't look likely to solve the problem. The crash looks like a use-after-free of something, which is strange because the Guest is stored as a WeakPtr (unless it is getting destroyed on another thread?).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2016/09/15 21:20:41, kenrb wrote: > While a null check there might be reasonable, since there is a null check > earlier in the method and in several other places in the file, it doesn't look > likely to solve the problem. The crash looks like a use-after-free of something, > which is strange because the Guest is stored as a WeakPtr (unless it is getting > destroyed on another thread?). I agree the crash looks like a use-after-free, but as indicated this seems weird unless it's a cross-thread de-allocation issue. But I had thought the null-check might be an inexpensive first step, just to see. Where is the null check (on guest_) earlier in the function? Note that RWHVCF::OnSwapCompositorFrame() does an early out at the beginning based on a null check of frame_connector_, which is sort of similar to guest_ in this scenario. The null check on guest_ at the *end* of the function was necessitated by an earlier crasher, suggesting that a null guest_ is possible, meaning the earlier instances should also be of concern, should they not?
My mistake, there isn't an earlier check in the method. lgtm with a suggestion, although I would be surprised if this fixes the current issue. https://codereview.chromium.org/2346863004/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2346863004/diff/1/content/browser/frame_host/... content/browser/frame_host/render_widget_host_view_guest.cc:286: (guest_ && guest_->has_attached_since_surface_set())) { Would it make more sense to test guest_ at the beginning and early return?
https://codereview.chromium.org/2346863004/diff/1/content/browser/frame_host/... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2346863004/diff/1/content/browser/frame_host/... content/browser/frame_host/render_widget_host_view_guest.cc:286: (guest_ && guest_->has_attached_since_surface_set())) { On 2016/09/16 13:37:31, kenrb wrote: > Would it make more sense to test guest_ at the beginning and early return? I wondered about that, but the comment at the end about "we should finish processing it" reminded me about the earlier bug, which was fixed here: https://codereview.chromium.org/1772393002 You can see that we got into different trouble when we had the early-out at the beginning.
wjmaclean@chromium.org changed reviewers: + creis@chromium.org
creis@ - tiny CL, can you take a look?
Hmm, it sounds like there's a deeper problem here that a null check is not likely to help with. I'll LGTM this given your comment about being consistent with later null checks, but I think you should add a comment at the point that guest_ can become null if you can (since it must be somewhere between the start of the method and what you've added, IIUC). As Ken mentioned, this change won't help with the UaF reported in the crash, so please update the CL subject/description to reflect that we don't expect this to fix the crash.
The CQ bit was checked by wjmaclean@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...
creis@ ptal? See comments below. On 2016/09/16 21:30:44, Charlie Reis (slow) wrote: > Hmm, it sounds like there's a deeper problem here that a null check is not > likely to help with. > > I'll LGTM this given your comment about being consistent with later null checks, > but I think you should add a comment at the point that guest_ can become null if > you can (since it must be somewhere between the start of the method and what > you've added, IIUC). The only place I'm aware of that guest_ can become null is via BrowserPluginGuest's destructor, via the owning WebContentsImpl's destructor. I suppose it's also possible we're looking at a memory corruption issue, though I'm not sure how likely that is. > As Ken mentioned, this change won't help with the UaF reported in the crash, so > please update the CL subject/description to reflect that we don't expect this to > fix the crash. I've spent some time today trying to refresh my understanding of BrowserPluginGuest's lifetime. So far I can find no reason why this code should be problematic (the weak pointer management all seems to be on the same thread, though it is the case that RenderWidgetHostViewGuest outlives BrowserPluginGuest). Taking a cue from RenderWidgetHostViewGuest::Show() and Hide(), I've added a check that BrowserPluginGuest isn't trying to destruct when we're trying to set a new child surface. Not sure if this will help, but at least it seems more likely to have bearing on a potential use-after-free scenario. I'll wait for your thoughts on this before I proceed.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I don't think your change in PS2 would help if it's really a UaF on guest_, but I'm having a hard time understanding what's going on in this crash. I'll also note that the Windows versions of this crash (e.g., f975028d00000000) look like null pointer dereferences, so maybe the null check is all we need after all? Anyway, I'm not opposed to landing PS2 and seeing what effect that has. LGTM. https://codereview.chromium.org/2346863004/diff/20001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_guest.cc (right): https://codereview.chromium.org/2346863004/diff/20001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_guest.cc:312: if (guest_ && !guest_->is_in_destruction()) { If the crash is actually a UaF on guest_, then neither a null check nor a method call on guest_ will avoid it. We'll just get the UaF on this line instead, when calling is_in_destruction. An actual UaF on guest_ might suggest that |this| has been deleted, but we probably would have crashed much earlier in this method if so. Thus, I'm not sure what to recommend.
The CQ bit was checked by wjmaclean@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from kenrb@chromium.org Link to the patchset: https://codereview.chromium.org/2346863004/#ps20001 (title: "Don't try to set surface if guest is destructing.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/19 17:29:22, Charlie Reis (slow) wrote: > I don't think your change in PS2 would help if it's really a UaF on guest_, but > I'm having a hard time understanding what's going on in this crash. I'll also > note that the Windows versions of this crash (e.g., f975028d00000000) look like > null pointer dereferences, so maybe the null check is all we need after all? > > Anyway, I'm not opposed to landing PS2 and seeing what effect that has. LGTM. > > https://codereview.chromium.org/2346863004/diff/20001/content/browser/frame_h... > File content/browser/frame_host/render_widget_host_view_guest.cc (right): > > https://codereview.chromium.org/2346863004/diff/20001/content/browser/frame_h... > content/browser/frame_host/render_widget_host_view_guest.cc:312: if (guest_ && > !guest_->is_in_destruction()) { > If the crash is actually a UaF on guest_, then neither a null check nor a method > call on guest_ will avoid it. We'll just get the UaF on this line instead, when > calling is_in_destruction. > > An actual UaF on guest_ might suggest that |this| has been deleted, but we > probably would have crashed much earlier in this method if so. Thus, I'm not > sure what to recommend. Yeah, it's kind of weird looking. I'm wondering if all the crash data is complete/accurate.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_site_isolation on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
The CQ bit was checked by creis@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Speculative fix for OnSwapCompositorFrame crasher. Speculative fix for crasher in RenderWidgetHostViewGuest:: OnSwapCompositorFrame(), in case it is caused by guest_ being null. BUG=646736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Speculative fix for OnSwapCompositorFrame crasher. Speculative fix for crasher in RenderWidgetHostViewGuest:: OnSwapCompositorFrame(), in case it is caused by guest_ being null. BUG=646736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Speculative fix for OnSwapCompositorFrame crasher. Speculative fix for crasher in RenderWidgetHostViewGuest:: OnSwapCompositorFrame(), in case it is caused by guest_ being null. BUG=646736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Speculative fix for OnSwapCompositorFrame crasher. Speculative fix for crasher in RenderWidgetHostViewGuest:: OnSwapCompositorFrame(), in case it is caused by guest_ being null. BUG=646736 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/fb98885220ad4096b09634f154aa4907fda603ba Cr-Commit-Position: refs/heads/master@{#419586} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/fb98885220ad4096b09634f154aa4907fda603ba Cr-Commit-Position: refs/heads/master@{#419586} |
