Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2563313002:
Clear the owner element's widget in Document::shutdown(). (Closed)
|
Created:
4 years ago by Mariusz Mlynski Modified:
4 years ago CC:
blink-reviews, chromium-reviews, haraken Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionClear the owner element's widget in Document::shutdown().
FrameView::dispose() doesn't guarantee that the owner's widget is cleared
and this could be problematic when it's overwritten in
LocalFrame::createView() a short time later.
BUG=673170
Committed: https://crrev.com/71b91a424a00dce5ff5ff4cf2e7b52fc70136f36
Cr-Commit-Position: refs/heads/master@{#438977}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Expanded the comment. #Patch Set 3 : Missing semicolon. #
Total comments: 2
Patch Set 4 : Clear the widget in Document::shutdown(). #
Messages
Total messages: 37 (12 generated)
Description was changed from ========== Clear the owner element's widget unconditionally in FrameView::dispose. If the widget isn't cleared in FrameView::dispose then it happens in LocalFrame::createView shortly anyway, and the latter might be problematic. BUG=673170 ========== to ========== Clear the owner element's widget unconditionally in FrameView::dispose(). If the widget isn't cleared in FrameView::dispose() then it happens in LocalFrame::createView() shortly anyway, and the latter might be problematic. BUG=673170 ==========
The CQ bit was checked by dcheng@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: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
On 2016/12/12 23:17:10, commit-bot: I haz the power wrote: > Dry run: Try jobs failed on following builders: > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) Hmm, I'm trying to find the exact point of failure, but all I'm getting is not very descriptive pages or 403s. @dcheng, any clue what happened here?
On 2016/12/12 23:49:01, Mariusz Mlynski wrote: > On 2016/12/12 23:17:10, commit-bot: I haz the power wrote: > > Dry run: Try jobs failed on following builders: > > linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, > > > https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) > > Hmm, I'm trying to find the exact point of failure, but all I'm getting is not > very descriptive pages or 403s. @dcheng, any clue what happened here? I think the bot just got really confused, we can ignore it. The main reason I haven't approved this yet is I recall making a similar change in the past, and lots of things exploded, so I'm trying to figure out what changed.
dcheng@chromium.org changed reviewers: + sigbjornf@opera.com
I've been reading over this, and I haven't really been able to convince myself that this won't inadvertently wreak havoc. At the very least, I think we should keep the TODO. +sof, who I'm hoping remembers the situations where this widget + owner element mismatch can occur… (I'm going to try a local build that asserts on this and see what layout tests explode as well)
On 2016/12/13 07:43:40, dcheng wrote: > I've been reading over this, and I haven't really been able to convince myself > that this won't inadvertently wreak havoc. At the very least, I think we should > keep the TODO. > > +sof, who I'm hoping remembers the situations where this widget + owner element > mismatch can occur… > > (I'm going to try a local build that asserts on this and see what layout tests > explode as well) This is an older check, hailing from https://bugs.webkit.org/show_bug.cgi?id=9952 I can't recall it being touched as part of making Widget and frames be GC managed, but I can go over the details.
On 2016/12/13 07:43:40, dcheng wrote: > I've been reading over this, and I haven't really been able to convince myself > that this won't inadvertently wreak havoc. At the very least, I think we should > keep the TODO. > > +sof, who I'm hoping remembers the situations where this widget + owner element > mismatch can occur… > > (I'm going to try a local build that asserts on this and see what layout tests > explode as well) Mismatches occur when a plugin element loads a frame (widget A of type FrameView) and then loads a plugin (widget B of type WebPluginContainerImpl). In this case, the frame's view is A and the frame element's owned widget is B. Implementing the TODO would probably require that the frame is unloaded when a plugin takes over, and that's a much bigger functional change. If you have any particular scenarios that this patch might cause to break then I'd be happy to go over that and analyze. As noted in the patch description, the widget clear does happen, just a little bit later for apparently historical reasons (thanks for digging out the link, sof). Webpages are unlikely to perceive any difference unless they're up to no good.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
Lgtm with some comments. https://codereview.chromium.org/2563313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2563313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:354: // points to another Widget. See https://crbug.com/673170 for an example. Can we add a commented out DCHECK here with the widget condition you removed? That way folks know what the comment is about. Also add the description you had in the review for how we get there here.
https://codereview.chromium.org/2563313002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2563313002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/frame/FrameView.cpp:354: // points to another Widget. See https://crbug.com/673170 for an example. On 2016/12/13 23:46:22, esprehn wrote: > Can we add a commented out DCHECK here with the widget condition you removed? > That way folks know what the comment is about. Also add the description you had > in the review for how we get there here. Done. The comment would logically better belong inside the if clause, but I think it's more readable like this. Let me know if you prefer otherwise.
On 2016/12/14 at 00:18:03, marius.mlynski wrote: > https://codereview.chromium.org/2563313002/diff/1/third_party/WebKit/Source/c... > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/2563313002/diff/1/third_party/WebKit/Source/c... > third_party/WebKit/Source/core/frame/FrameView.cpp:354: // points to another Widget. See https://crbug.com/673170 for an example. > On 2016/12/13 23:46:22, esprehn wrote: > > Can we add a commented out DCHECK here with the widget condition you removed? > > That way folks know what the comment is about. Also add the description you had > > in the review for how we get there here. > > Done. The comment would logically better belong inside the if clause, but I think it's more readable like this. Let me know if you prefer otherwise. Looks fine to me, I usually put it before as well. :)
lgtm https://codereview.chromium.org/2563313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2563313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:358: // DCHECK(ownerElement->ownedWidget() == this); Nit: let's remove commented out code. I don't think I completely understand what's going on here, but let's stop the bleeding first =)
https://codereview.chromium.org/2563313002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2563313002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:358: // DCHECK(ownerElement->ownedWidget() == this); On 2016/12/14 02:23:00, dcheng wrote: > Nit: let's remove commented out code. This code is what @esprehn asked for :D I think we might want to leave it to make sure the condition described in the comment is unambiguous.
On 2016/12/14 02:36:50, Mariusz Mlynski wrote: > https://codereview.chromium.org/2563313002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/2563313002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameView.cpp:358: // > DCHECK(ownerElement->ownedWidget() == this); > On 2016/12/14 02:23:00, dcheng wrote: > > Nit: let's remove commented out code. > > This code is what @esprehn asked for :D I think we might want to leave it to > make sure the condition described in the comment is unambiguous. Hm, OK. Well, I don't feel strongly about it; but commented-out code seems likely to be a potential victim of cleanup. An alternate way to phrase it would be: We should DCHECK(ownerElement->ownedWidget() == this) here, but [...] More importantly, I still haven't convinced myself this is a safe transformation. My understanding is the mismatched widgets can happen if an UpdateSuspendScope is active. When we call setWidget(), |m_widget| is immediately changed, but the actual widget tree mutations are deferred. How do we guarantee when we're performing the deferred widget tree operations that we don't incorrectly clobber |m_widget|?
In blink we often leave commented out asserts with an explanation saying why we can't assert it yet but want to.
On 2016/12/14 03:49:45, esprehn wrote: > In blink we often leave commented out asserts with an explanation saying why we > can't assert it yet but want to. OK, I guess I'm not accustomed to that myself, but that seems reasonable. I'm OK with landing this and seeing what explodes. If we're really lucky, nothing will explode...
On 2016/12/14 03:44:46, dcheng wrote: > More importantly, I still haven't convinced myself this is a safe > transformation. My understanding is the mismatched widgets can happen if an > UpdateSuspendScope is active. Yes, this can happen, but to clarify, the |ownerElement->ownedWidget() != this| mismatch doesn't require an UpdateSuspendScope if this is what you mean. It happens when HTMLPlugInElement::loadPlugin calls setWidget(widget), which can be in an UpdateSuspendScope or not. > When we call setWidget(), |m_widget| is > immediately changed, but the actual widget tree mutations are deferred. This is correct. > How do we guarantee when we're performing the deferred widget tree operations > that we don't incorrectly clobber |m_widget|? It isn't guaranteed that |m_widget| won't be written, but deferred widget operations are executed while widget updates are still suspended, so as you noted, tree mutations can't happen and the new widget will have no parent. Therefore, this is not practical for the attack scenario we're fixing. Still, it might not be such a bad idea to disallow setting widgets from the UpdateSuspendScope destructor -- that'd probably mean a PerformingDeferredWidgetUpdatesScope, though ;-) Let me know when it's okay to hit the button to land.
On 2016/12/14 08:38:29, Mariusz Mlynski wrote: > On 2016/12/14 03:44:46, dcheng wrote: > > More importantly, I still haven't convinced myself this is a safe > > transformation. My understanding is the mismatched widgets can happen if an > > UpdateSuspendScope is active. > > Yes, this can happen, but to clarify, the |ownerElement->ownedWidget() != this| > mismatch doesn't require an UpdateSuspendScope if this is what you mean. It > happens when HTMLPlugInElement::loadPlugin calls setWidget(widget), which can be > in an UpdateSuspendScope or not. Outside of an update suspend scope, the widget tree updates take place immediately. Even though it uses moveWidgetToParentSoon(), "soon" can be immediately. In that cae, I think the pointers would be synchronized. > > > When we call setWidget(), |m_widget| is > > immediately changed, but the actual widget tree mutations are deferred. > > This is correct. > > > How do we guarantee when we're performing the deferred widget tree operations > > that we don't incorrectly clobber |m_widget|? > > It isn't guaranteed that |m_widget| won't be written, but deferred widget > operations are executed while widget updates are still suspended, so as you > noted, tree mutations can't happen and the new widget will have no parent. > Therefore, this is not practical for the attack scenario we're fixing. Still, it > might not be such a bad idea to disallow setting widgets from the > UpdateSuspendScope destructor -- that'd probably mean a > PerformingDeferredWidgetUpdatesScope, though ;-) > > Let me know when it's okay to hit the button to land. Right -- but this is what I'm imagining: 1. We enter an UpdateSuspendScope. 2. *somehow*, we end up doing a widget tree mutation to an HTMLPlugInElement. This sets |m_widget = newWidget|, but queues the widget tree updates to dispose |oldWidget|. 3. In ~UpdateSuspendScope, we dispose |oldWidget|. Since FrameView::dispose() now unconditionally clears |m_widget|, we end up with a null |m_widget| even though |newWidget| thinks it's attached. Even though |newWidget|'s update will also be processed, its owner element no longer points to it, which also seems problematic.
On 2016/12/14 17:18:35, dcheng wrote: > On 2016/12/14 08:38:29, Mariusz Mlynski wrote: > > On 2016/12/14 03:44:46, dcheng wrote: > > > More importantly, I still haven't convinced myself this is a safe > > > transformation. My understanding is the mismatched widgets can happen if an > > > UpdateSuspendScope is active. > > > > Yes, this can happen, but to clarify, the |ownerElement->ownedWidget() != > this| > > mismatch doesn't require an UpdateSuspendScope if this is what you mean. It > > happens when HTMLPlugInElement::loadPlugin calls setWidget(widget), which can > be > > in an UpdateSuspendScope or not. > > Outside of an update suspend scope, the widget tree updates take place > immediately. Even though it uses moveWidgetToParentSoon(), "soon" can be > immediately. In that cae, I think the pointers would be synchronized. I think we're talking about different pointers -- |ownerElement->ownedWidget()| and |this| desynchronize through an update of the ownerElement's m_widget, and as you noted in #18, m_widget updates are always immediate, both inside and outside of an UpdateSuspendScope. > > > When we call setWidget(), |m_widget| is > > > immediately changed, but the actual widget tree mutations are deferred. > > > > This is correct. > > > > > How do we guarantee when we're performing the deferred widget tree > operations > > > that we don't incorrectly clobber |m_widget|? > > > > It isn't guaranteed that |m_widget| won't be written, but deferred widget > > operations are executed while widget updates are still suspended, so as you > > noted, tree mutations can't happen and the new widget will have no parent. > > Therefore, this is not practical for the attack scenario we're fixing. Still, > it > > might not be such a bad idea to disallow setting widgets from the > > UpdateSuspendScope destructor -- that'd probably mean a > > PerformingDeferredWidgetUpdatesScope, though ;-) > > > > Let me know when it's okay to hit the button to land. > > Right -- but this is what I'm imagining: > 1. We enter an UpdateSuspendScope. > 2. *somehow*, we end up doing a widget tree mutation to an HTMLPlugInElement. > This sets |m_widget = newWidget|, but queues the widget tree updates to dispose > |oldWidget|. > 3. In ~UpdateSuspendScope, we dispose |oldWidget|. Since FrameView::dispose() > now unconditionally clears |m_widget|, we end up with a null |m_widget| even > though |newWidget| thinks it's attached. Even though |newWidget|'s update will > also be processed, its owner element no longer points to it, which also seems > problematic. Okay, I see what you mean here. So basically, the expected behavior of FrameView::dispose() would be different depending on whether it's called from Document::shutdown() or from a widget operation. In Document::shutdown(), we really mean to clear the owner element's widget, because the subsequent clobber might be dangerous. How about we call ownerElement->setWidget(null) directly from Document::shutdown() and annotate that we do this once again because FrameView::dispose() might skip it in special circumstances? Sort of hacky, but that'd be 100% safe.
On 2016/12/14 20:18:13, Mariusz Mlynski wrote: > On 2016/12/14 17:18:35, dcheng wrote: > > On 2016/12/14 08:38:29, Mariusz Mlynski wrote: > > > On 2016/12/14 03:44:46, dcheng wrote: > > > > More importantly, I still haven't convinced myself this is a safe > > > > transformation. My understanding is the mismatched widgets can happen if > an > > > > UpdateSuspendScope is active. > > > > > > Yes, this can happen, but to clarify, the |ownerElement->ownedWidget() != > > this| > > > mismatch doesn't require an UpdateSuspendScope if this is what you mean. It > > > happens when HTMLPlugInElement::loadPlugin calls setWidget(widget), which > can > > be > > > in an UpdateSuspendScope or not. > > > > Outside of an update suspend scope, the widget tree updates take place > > immediately. Even though it uses moveWidgetToParentSoon(), "soon" can be > > immediately. In that cae, I think the pointers would be synchronized. > > I think we're talking about different pointers -- |ownerElement->ownedWidget()| > and |this| desynchronize through an update of the ownerElement's m_widget, and > as you noted in #18, m_widget updates are always immediate, both inside and > outside of an UpdateSuspendScope. > > > > > When we call setWidget(), |m_widget| is > > > > immediately changed, but the actual widget tree mutations are deferred. > > > > > > This is correct. > > > > > > > How do we guarantee when we're performing the deferred widget tree > > operations > > > > that we don't incorrectly clobber |m_widget|? > > > > > > It isn't guaranteed that |m_widget| won't be written, but deferred widget > > > operations are executed while widget updates are still suspended, so as you > > > noted, tree mutations can't happen and the new widget will have no parent. > > > Therefore, this is not practical for the attack scenario we're fixing. > Still, > > it > > > might not be such a bad idea to disallow setting widgets from the > > > UpdateSuspendScope destructor -- that'd probably mean a > > > PerformingDeferredWidgetUpdatesScope, though ;-) > > > > > > Let me know when it's okay to hit the button to land. > > > > Right -- but this is what I'm imagining: > > 1. We enter an UpdateSuspendScope. > > 2. *somehow*, we end up doing a widget tree mutation to an HTMLPlugInElement. > > This sets |m_widget = newWidget|, but queues the widget tree updates to > dispose > > |oldWidget|. > > 3. In ~UpdateSuspendScope, we dispose |oldWidget|. Since FrameView::dispose() > > now unconditionally clears |m_widget|, we end up with a null |m_widget| even > > though |newWidget| thinks it's attached. Even though |newWidget|'s update will > > also be processed, its owner element no longer points to it, which also seems > > problematic. > > Okay, I see what you mean here. So basically, the expected behavior of > FrameView::dispose() would be different depending on whether it's called from > Document::shutdown() or from a widget operation. In Document::shutdown(), we > really mean to clear the owner element's widget, because the subsequent clobber > might be dangerous. > > How about we call ownerElement->setWidget(null) directly from > Document::shutdown() and annotate that we do this once again because > FrameView::dispose() might skip it in special circumstances? Sort of hacky, but > that'd be 100% safe. That seems reasonable to me.
Description was changed from ========== Clear the owner element's widget unconditionally in FrameView::dispose(). If the widget isn't cleared in FrameView::dispose() then it happens in LocalFrame::createView() shortly anyway, and the latter might be problematic. BUG=673170 ========== to ========== Clear the owner element's widget in Document::shutdown(). FrameView::dispose() doesn't guarantee that the owner's widget is cleared and this could be problematic when it's overwritten in LocalFrame::createView() a short time later. BUG=673170 ==========
On 2016/12/15 09:57:23, dcheng wrote: > > How about we call ownerElement->setWidget(null) directly from > > Document::shutdown() and annotate that we do this once again because > > FrameView::dispose() might skip it in special circumstances? Sort of hacky, > but > > that'd be 100% safe. > > That seems reasonable to me. PTAL.
LGTM
@esprehn: does it still LG?
lgtm
Btw thanks for adding the great comments!
The CQ bit was checked by marius.mlynski@gmail.com
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": 60001, "attempt_start_ts": 1481838660474590,
"parent_rev": "e313095402d20cd3ba8dd9781d5473242470824b", "commit_rev":
"3032352e4606715b1f5a86aa047c53b430d7470a"}
Message was sent while issue was closed.
Description was changed from ========== Clear the owner element's widget in Document::shutdown(). FrameView::dispose() doesn't guarantee that the owner's widget is cleared and this could be problematic when it's overwritten in LocalFrame::createView() a short time later. BUG=673170 ========== to ========== Clear the owner element's widget in Document::shutdown(). FrameView::dispose() doesn't guarantee that the owner's widget is cleared and this could be problematic when it's overwritten in LocalFrame::createView() a short time later. BUG=673170 Review-Url: https://codereview.chromium.org/2563313002 ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Clear the owner element's widget in Document::shutdown(). FrameView::dispose() doesn't guarantee that the owner's widget is cleared and this could be problematic when it's overwritten in LocalFrame::createView() a short time later. BUG=673170 Review-Url: https://codereview.chromium.org/2563313002 ========== to ========== Clear the owner element's widget in Document::shutdown(). FrameView::dispose() doesn't guarantee that the owner's widget is cleared and this could be problematic when it's overwritten in LocalFrame::createView() a short time later. BUG=673170 Committed: https://crrev.com/71b91a424a00dce5ff5ff4cf2e7b52fc70136f36 Cr-Commit-Position: refs/heads/master@{#438977} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/71b91a424a00dce5ff5ff4cf2e7b52fc70136f36 Cr-Commit-Position: refs/heads/master@{#438977} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
