UpdateWidget() can fire beforeload event synchronously blowing away RenderArena and its associated …

UpdateWidget() can fire beforeload event synchronously blowing away RenderArena and its associated RenderObjects. In that case, we bail-out and clear m_widgetUpdateSet set. 

BUG=226696
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=148933

[cevans, 2013-04-17 20:03:14]

On 2013/04/17 19:19:33, inferno wrote:

I think the RenderArena lifetime is tied to a owning Document? If so, couldn't
we just RefPtr the Document during the operation that is causing trouble?

[inferno, 2013-04-17 20:05:47]

On 2013/04/17 20:03:14, cevans wrote:
> On 2013/04/17 19:19:33, inferno wrote:
> 
> I think the RenderArena lifetime is tied to a owning Document? If so, couldn't
> we just RefPtr the Document during the operation that is causing trouble?

RefPtr Document won't help since RenderArena is freed in explicit
Document::detach() call, not in Document destructor.

[abarth-chromium, 2013-04-17 22:30:12]

Can we write a test?

[inferno, 2013-04-17 22:34:13]

On 2013/04/17 22:30:12, abarth wrote:
> Can we write a test?

The fix causes a hard crash, so i think the test can't run on our test
infrastructure.

[eseidel, 2013-04-18 00:13:00]

https://codereview.chromium.org/14329005/diff/1/Source/core/page/FrameView.cpp
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/14329005/diff/1/Source/core/page/FrameView.cp...
Source/core/page/FrameView.cpp:2288: RELEASE_ASSERT(ownerElement->renderer() ==
embeddedObject);
Why is crashing the right thing to do here?  What does just returning do?

If we returned then we could test this. :)

[inferno, 2013-04-18 00:24:33]

On 2013/04/18 00:13:00, Eric Seidel (Google) wrote:
> https://codereview.chromium.org/14329005/diff/1/Source/core/page/FrameView.cpp
> File Source/core/page/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/14329005/diff/1/Source/core/page/FrameView.cp...
> Source/core/page/FrameView.cpp:2288: RELEASE_ASSERT(ownerElement->renderer()
==
> embeddedObject);
> Why is crashing the right thing to do here?  What does just returning do?
> 
> If we returned then we could test this. :)

The caller in the chain iterates over remaining renderobjects. So, the bailouts
needs to be added there. But now that i think about it, there is a case which
this patch misses, which is if only blow the next renderobject in the vector
(without blowing document), that will still be use after free. Need to think
more on the fix.

for (size_t i = 0; i < size; ++i) {
        RenderObject* object = objects[i];
        updateWidget(object);
        m_widgetUpdateSet->remove(object);
    }

    RenderArena* arena = m_frame->document()->renderArena();
    for (size_t i = 0; i < size; ++i) {
        RenderObject* object = objects[i];
        if (object->isEmbeddedObject()) {
            RenderEmbeddedObject* embeddedObject =
static_cast<RenderEmbeddedObject*>(object);
            embeddedObject->deref(arena);
        }
    }

[eseidel, 2013-04-18 02:14:22]

Please add a test and then I'm ready to r+.

https://codereview.chromium.org/14329005/diff/1/Source/core/page/FrameView.cpp
File Source/core/page/FrameView.cpp (right):

https://codereview.chromium.org/14329005/diff/1/Source/core/page/FrameView.cp...
Source/core/page/FrameView.cpp:2264: RefPtr<Element> ownerElement =
toElement(object->node());
I don't believe that holding onto the Element will cause the Document to stay
alive (the Document holds the RenderArena).

https://codereview.chromium.org/14329005/diff/1/Source/core/page/FrameView.cp...
Source/core/page/FrameView.cpp:2288: RELEASE_ASSERT(ownerElement->renderer() ==
embeddedObject);
I wouldn't ASSERT, I would just return here.  We don't need to crash in this
case.  Not notifying the widget of it's correct position is at worst a display
bug, and in this case, the widget is gone, so we just want to stop trying to
talk to it!

[inferno, 2013-04-18 16:34:51]

On 2013/04/18 02:14:22, Eric Seidel (Google) wrote:
> Please add a test and then I'm ready to r+.
> 
> https://codereview.chromium.org/14329005/diff/1/Source/core/page/FrameView.cpp
> File Source/core/page/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/14329005/diff/1/Source/core/page/FrameView.cp...
> Source/core/page/FrameView.cpp:2264: RefPtr<Element> ownerElement =
> toElement(object->node());
> I don't believe that holding onto the Element will cause the Document to stay
> alive (the Document holds the RenderArena).

RefPtring Element is not meant to keep Document alive, but to keep itself alive.
After updateWidget call, we are using it to check whether its renderer is
correct and we don't want to keep it as a weak pointer which can get destroyed
somehow.

> 
>
https://codereview.chromium.org/14329005/diff/1/Source/core/page/FrameView.cp...
> Source/core/page/FrameView.cpp:2288: RELEASE_ASSERT(ownerElement->renderer()
==
> embeddedObject);
> I wouldn't ASSERT, I would just return here.  We don't need to crash in this
> case.  Not notifying the widget of it's correct position is at worst a display
> bug, and in this case, the widget is gone, so we just want to stop trying to
> talk to it!

Well, i made the test this morning, but while writing the fix to recover in this
situation, i came back to my original position. We can bail out of
FrameView::updateWidget, but its caller FrameView::updateWidgets()
maintains/uses two vectors both of which are raw renderobject pointers. One is
m_widgetUpdateSet, other is local Vector<RenderObject*> objects;. After
returning, we cannot iterate into these raw vectors as any of their renderobject
could be destroyed. I thought about converting m_widgetUpdateSet and everything
else into a vector of refptred nodes, but then i saw this, so looks like we keep
trying updatewidgets till we get the non-null element and then call
frameview::updatewidget.

    for (unsigned i = 0; i < maxUpdateWidgetsIterations; i++) {
        if (updateWidgets())
            break;
    }

I dont see a fix for this. I think my patch #1 might be the only way to go for
now, without breaking stuff.

[eseidel, 2013-04-18 20:40:35]

It seems worst-case we could make updateWidget() return a bool, and if the bool
is ever true, bail.

But isn't there another way to detect any such weirdness inside updateWidgets()?

[eseidel, 2013-04-23 20:37:51]

lgtm

https://codereview.chromium.org/14329005/diff/12001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/14329005/diff/12001/Source/core/dom/Document....
Source/core/dom/Document.cpp:2362: if (f && renderObject &&
AXObjectCache::accessibilityEnabled() && axObjectCache()) {
axObjectCache() isn't redundant with accessibilityEnabled? and doesn't cause one
to be lazyily created, correct?

[inferno, 2013-04-23 20:43:42]

https://codereview.chromium.org/14329005/diff/12001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/14329005/diff/12001/Source/core/dom/Document....
Source/core/dom/Document.cpp:2362: if (f && renderObject &&
AXObjectCache::accessibilityEnabled() && axObjectCache()) {
On 2013/04/23 20:37:51, Eric Seidel (Google) wrote:
> axObjectCache() isn't redundant with accessibilityEnabled? and doesn't cause
one
> to be lazyily created, correct?

It is not redundant with accessibilityEnabled. See
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

Let me check with Dominicc if it looks ok to him.

[dmazzoni, 2013-04-23 20:54:07]

https://codereview.chromium.org/14329005/diff/12001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/14329005/diff/12001/Source/core/dom/Document....
Source/core/dom/Document.cpp:2362: if (f && renderObject &&
AXObjectCache::accessibilityEnabled() && axObjectCache()) {
On 2013/04/23 20:37:51, Eric Seidel (Google) wrote:
> axObjectCache() isn't redundant with accessibilityEnabled? and doesn't cause
one
> to be lazyily created, correct?

Nope, this creates an AXObjectCache. If you don't want to, use
existingAXObjectCache().

However, can you explain why you want to make this change? If accessibility is
enabled, we *want* to create an AXObjectCache at this point if there isn't one
already, otherwise the AXLoadComplete will never fire.

It this is causing a problem, we should fix it inside AXObjectCache.

[inferno, 2013-04-23 20:57:27]

On 2013/04/23 20:54:07, Dominic Mazzoni wrote:
>
https://codereview.chromium.org/14329005/diff/12001/Source/core/dom/Document.cpp
> File Source/core/dom/Document.cpp (right):
> 
>
https://codereview.chromium.org/14329005/diff/12001/Source/core/dom/Document....
> Source/core/dom/Document.cpp:2362: if (f && renderObject &&
> AXObjectCache::accessibilityEnabled() && axObjectCache()) {
> On 2013/04/23 20:37:51, Eric Seidel (Google) wrote:
> > axObjectCache() isn't redundant with accessibilityEnabled? and doesn't cause
> one
> > to be lazyily created, correct?
> 
> Nope, this creates an AXObjectCache. If you don't want to, use
> existingAXObjectCache().
> 
> However, can you explain why you want to make this change? If accessibility is
> enabled, we *want* to create an AXObjectCache at this point if there isn't one
> already, otherwise the AXLoadComplete will never fire.
> 
> It this is causing a problem, we should fix it inside AXObjectCache.

Yes, we want a axobjectcache here, see
axObjectCache()->getOrCreate(renderObject); call inside the if. Doing the null
axobjectcache helps to check if we lost our renderer

AXObjectCache* Document::axObjectCache() const
{
.......
    // If the document has already been detached, do not make a new
axObjectCache.
    if (!topDocument->renderer())
        return 0;

[dmazzoni, 2013-04-23 21:08:41]

https://codereview.chromium.org/14329005/diff/12001/Source/core/dom/Document.cpp
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/14329005/diff/12001/Source/core/dom/Document....
Source/core/dom/Document.cpp:2362: if (f && renderObject &&
AXObjectCache::accessibilityEnabled() && axObjectCache()) {
OK, so this is what you want.

As discussed offline, let's store it in a local -
that makes it more clear that you're creating it and just
checking if it's null, and it saves walking up to topDocument
3 - 4 times in a row.

[inferno, 2013-04-23 21:32:08]

Committed patchset #3 manually as r148933 (presubmit successful).