Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2560803002:
Don't assume a FrameHost from FrameView. (Closed)
DescriptionDon't assume a FrameHost from FrameView.
This fixes a crash that happens when a detached plugin gets updated.
BUG=671331
Patch Set 1 #Patch Set 2 : Patch #
Total comments: 1
Messages
Total messages: 22 (7 generated)
The CQ bit was checked by bokan@chromium.org to run a CQ dry run
Description was changed from ========== Don't assume FrameView has a host(). This fixes a crash that happens when a detached plugin gets updated. BUG=671331 ========== to ========== Don't assume FrameView has a host(). This fixes a crash that happens when a detached plugin gets updated. BUG=671331 ==========
bokan@chromium.org changed reviewers: + dcheng@chromium.org
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 ========== Don't assume FrameView has a host(). This fixes a crash that happens when a detached plugin gets updated. BUG=671331 ========== to ========== Don't assume a FrameHost from FrameView. This fixes a crash that happens when a detached plugin gets updated. BUG=671331 ==========
ptal
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
(Sorry for not replying yet, I need to think about this a bit more. I was at an offsite today... I'll reply on this again tomorrow)
ping
https://codereview.chromium.org/2560803002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/2560803002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameView.cpp:335: if (FrameHost* frameHost = m_frame->host()) What's the stack where we hit this and it's null? In theory, we should only be calling this when the frame has a currently active (i.e. not detached) Document.
On 2016/12/12 21:32:46, dcheng wrote: > https://codereview.chromium.org/2560803002/diff/20001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/2560803002/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/frame/FrameView.cpp:335: if (FrameHost* frameHost > = m_frame->host()) > What's the stack where we hit this and it's null? In theory, we should only be > calling this when the frame has a currently active (i.e. not detached) Document. #0 0x7fcbd407c51e base::debug::StackTrace::StackTrace() #1 0x7fcbd407c05f base::debug::(anonymous namespace)::StackDumpSignalHandler() #2 0x7fcbd44d2330 <unknown> #3 0x7fcbb9bbba5c WTF::VectorBufferBase<>::buffer() #4 0x7fcbba5244c9 blink::FrameHost::globalRootScrollerController() #5 0x7fcbba530b43 blink::FrameView::dispose() #6 0x7fcbba656e25 blink::HTMLFrameOwnerElement::UpdateSuspendScope::performDeferredWidgetTreeOperations() #7 0x7fcbba6573d5 blink::HTMLFrameOwnerElement::UpdateSuspendScope::~UpdateSuspendScope() #8 0x7fcbba0c49c5 blink::ContainerNode::removeChildren() #9 0x7fcbba6388ce blink::HTMLElement::setInnerText() #10 0x7fcbbb2ad70a blink::HTMLElementV8Internal::innerTextAttributeSetter() #11 0x7fcbbb2ad5df blink::HTMLElementV8Internal::innerTextAttributeSetterCallback() #12 0x7fcbc83eb716 v8::internal::FunctionCallbackArguments::Call() #13 0x7fcbc84b9eb3 v8::internal::(anonymous namespace)::HandleApiCallHelper<>() #14 0x7fcbc84b8fa3 v8::internal::Builtins::InvokeApiFunction() #15 0x7fcbc89fb55c v8::internal::Object::SetPropertyWithAccessor() #16 0x7fcbc8a14b38 v8::internal::Object::SetPropertyInternal() #17 0x7fcbc8a146a6 v8::internal::Object::SetProperty() #18 0x7fcbc894c921 v8::internal::StoreIC::Store() #19 0x7fcbc89546a2 v8::internal::__RT_impl_Runtime_StoreIC_Miss() (This doesn't happen in my reduced test case but does in the clusterfuzz testcase) Presumably we've queued up the disposal but detached the FrameView in the mean time?
Ping, should we land this guard while the larger issue of deferred widget updates is resolved?
Sorry, I'm still puzzling over this. I don't understand why we're calling updateWidget on a detached plugin element. https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra... appears like it attempts to protect against this. At this point, I wouldn't expect the layout object to point back to any Node: I would have expected clearing the children to clear the node from the LayoutEmbeddedObject when we detach the layout tree: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Node.... So why isn't this happening?
On 2016/12/14 17:30:38, dcheng wrote: > Sorry, I'm still puzzling over this. I don't understand why we're calling > updateWidget on a detached plugin element. > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra... > appears like it attempts to protect against this. At this point, I wouldn't > expect the layout object to point back to any Node: I would have expected > clearing the children to clear the node from the LayoutEmbeddedObject when we > detach the layout tree: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Node.... > > So why isn't this happening? I think it's because the EmbeddedObjectSet we're iterating over in FrameView::updateWidgets is LayoutObjects. In Node::detachLayoutTree we clear the Node->LayoutObject pointer but as far as I can tell we never clear the LayoutObject->Node link. When we go to FrameView::updateWidgets, we've detached the LayoutTree but the LayoutObject still has the pointer back to its Element.
On 2016/12/14 21:57:12, bokan wrote: > On 2016/12/14 17:30:38, dcheng wrote: > > Sorry, I'm still puzzling over this. I don't understand why we're calling > > updateWidget on a detached plugin element. > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra... > > appears like it attempts to protect against this. At this point, I wouldn't > > expect the layout object to point back to any Node: I would have expected > > clearing the children to clear the node from the LayoutEmbeddedObject when we > > detach the layout tree: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Node.... > > > > So why isn't this happening? > > > I think it's because the EmbeddedObjectSet we're iterating over in > FrameView::updateWidgets is LayoutObjects. In Node::detachLayoutTree we clear > the Node->LayoutObject pointer but as far as I can tell we never clear the > LayoutObject->Node link. When we go to FrameView::updateWidgets, we've detached > the LayoutTree but the LayoutObject still has the pointer back to its Element. Actually, hang on, that's for the viewportSizeChanged call. Let me look a little closer at the dispose case.
On 2016/12/14 22:06:44, bokan wrote: > On 2016/12/14 21:57:12, bokan wrote: > > On 2016/12/14 17:30:38, dcheng wrote: > > > Sorry, I'm still puzzling over this. I don't understand why we're calling > > > updateWidget on a detached plugin element. > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra... > > > appears like it attempts to protect against this. At this point, I wouldn't > > > expect the layout object to point back to any Node: I would have expected > > > clearing the children to clear the node from the LayoutEmbeddedObject when > we > > > detach the layout tree: > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Node.... > > > > > > So why isn't this happening? > > > > > > I think it's because the EmbeddedObjectSet we're iterating over in > > FrameView::updateWidgets is LayoutObjects. In Node::detachLayoutTree we clear > > the Node->LayoutObject pointer but as far as I can tell we never clear the > > LayoutObject->Node link. When we go to FrameView::updateWidgets, we've > detached > > the LayoutTree but the LayoutObject still has the pointer back to its Element. > > Actually, hang on, that's for the viewportSizeChanged call. Let me look a little > closer at the dispose case. Sorry, just realized the above answers your question as to how we call updateWidget. I got confused and thought you were asking about calling dispose. We can get into FrameView::dispose on a detached Widget via: #1 0x7f5f79139df9 blink::moveWidgetToParentSoon() #2 0x7f5f7913ac30 blink::HTMLFrameOwnerElement::setWidget() #3 0x7f5f791afba2 blink::HTMLPlugInElement::detachLayoutTree() #4 0x7f5f78ba66a5 blink::ContainerNode::removeBetween() #5 0x7f5f78ba6a51 blink::ContainerNode::removeChildren() #6 0x7f5f7911ae3e blink::HTMLElement::setInnerText() #7 0x7f5f79d9022a blink::HTMLElementV8Internal::innerTextAttributeSetter() Which defers the disposal until we destruct HTMLFrameOwnerElement::UpdateSuspendScope in removeChildren() but I don't see anything guarding against that. Maybe I'm missing where that's going of the rails?
On 2016/12/14 21:57:12, bokan wrote: > On 2016/12/14 17:30:38, dcheng wrote: > > Sorry, I'm still puzzling over this. I don't understand why we're calling > > updateWidget on a detached plugin element. > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra... > > appears like it attempts to protect against this. At this point, I wouldn't > > expect the layout object to point back to any Node: I would have expected > > clearing the children to clear the node from the LayoutEmbeddedObject when we > > detach the layout tree: > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Node.... > > > > So why isn't this happening? > > > I think it's because the EmbeddedObjectSet we're iterating over in > FrameView::updateWidgets is LayoutObjects. In Node::detachLayoutTree we clear > the Node->LayoutObject pointer but as far as I can tell we never clear the > LayoutObject->Node link. When we go to FrameView::updateWidgets, we've detached > the LayoutTree but the LayoutObject still has the pointer back to its Element. As far as I can tell, detachLayoutTree() should call LayoutObject::destroy(). LayoutEmbeddedObject is a LayoutPart. LayoutPart::destroy() does this: void LayoutPart::destroy() { willBeDestroyed(); // We call clearNode here because LayoutPart is ref counted. This call to // destroy may not actually destroy the layout object. We can keep it around // because of references from the FrameView class. (The actual destruction of // the class happens in postDestroy() which is called from deref()). // // But, we've told the system we've destroyed the layoutObject, which happens // when the DOM node is destroyed. So there is a good change the DOM node this // object points too is invalid, so we have to clear the node so we make sure // we don't access it in the future. clearNode(); deref(); } which should clear the Node, unless I'm missing something?
On 2016/12/14 23:16:49, dcheng wrote: > On 2016/12/14 21:57:12, bokan wrote: > > On 2016/12/14 17:30:38, dcheng wrote: > > > Sorry, I'm still puzzling over this. I don't understand why we're calling > > > updateWidget on a detached plugin element. > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Fra... > > > appears like it attempts to protect against this. At this point, I wouldn't > > > expect the layout object to point back to any Node: I would have expected > > > clearing the children to clear the node from the LayoutEmbeddedObject when > we > > > detach the layout tree: > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/dom/Node.... > > > > > > So why isn't this happening? > > > > > > I think it's because the EmbeddedObjectSet we're iterating over in > > FrameView::updateWidgets is LayoutObjects. In Node::detachLayoutTree we clear > > the Node->LayoutObject pointer but as far as I can tell we never clear the > > LayoutObject->Node link. When we go to FrameView::updateWidgets, we've > detached > > the LayoutTree but the LayoutObject still has the pointer back to its Element. > > As far as I can tell, detachLayoutTree() should call LayoutObject::destroy(). > LayoutEmbeddedObject is a LayoutPart. LayoutPart::destroy() does this: > > void LayoutPart::destroy() { > willBeDestroyed(); > // We call clearNode here because LayoutPart is ref counted. This call to > // destroy may not actually destroy the layout object. We can keep it around > // because of references from the FrameView class. (The actual destruction of > // the class happens in postDestroy() which is called from deref()). > // > // But, we've told the system we've destroyed the layoutObject, which happens > // when the DOM node is destroyed. So there is a good change the DOM node this > // object points too is invalid, so we have to clear the node so we make sure > // we don't access it in the future. > clearNode(); > deref(); > } > > which should clear the Node, unless I'm missing something? Ah, yah, sorry - you're right, I missed that LayoutEmbeddedObject was a LayoutPart. Actually, when we call updateWidgets we haven't yet detached the LayoutEbmeddedObject but we have detached the Frame. The stack traces and extra detail are in https://bugs.chromium.org/p/chromium/issues/detail?id=671331#c2 but the gist of it is: - When we detach the <embed> from its Frame, we stop all loaders - This causes us to checkCompleted on the parent frame which happens to be completed - We then *synchronously* dispatch the load event on the parent which then creates and touches an <object> causing updateWidgets. Note we haven't actually removed the <embed> from the DOM at this point.
ping
After poking at this a bunch, I think I understand what's causing this and submitted https://codereview.chromium.org/2586633002/. I think this should address the crashes, so we should be able to close this.
awesome, thanks dcheng! |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
