Bug 50312 – Assertion failure in WebCore::HTMLFrameElementBase::insertedIntoDocument()

• Component: Frames
• Status: RESOLVED FIXED

[Jeff Johnson]

Created attachment 75255 [details]
Crash log

Steps to Reproduce:
1) Build WebKit svn r73008 Debug configuration from source
2) run-safari
3) Load http://sports.yahoo.com/

Build Date & Platform:
Mac OS X 10.6.5, Safari 5.0.3, WebKit svn rr73008

Additional Information:
ASSERTION FAILED: !renderer()
(/Users/Shared/source/WebKit/WebCore/html/HTMLFrameElementBase.cpp:188 virtual void WebCore::HTMLFrameElementBase::insertedIntoDocument())

[Eric Seidel (no email)]

*** Bug 75390 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

This does not reproduce for me in a Debug build, but it may be ad-related.

[Eric Seidel (no email)]

*** Bug 71177 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

*** Bug 54711 has been marked as a duplicate of this bug. ***

[Eric Seidel (no email)]

Given the 4 reports, I'm sure folks are hitting this in a debug build.  But w/o a consistent repro, it's very difficult to move forward on.  Fixing this bug will help us better understand the insanity of our current plugin creation code. :)

[Eric Seidel (no email)]

I mailed webkit-dev asking for reproductions.  Unfortunately, I typo'd.  It's not plugins, but rather frames.  Still likely caused by ads of course. :)  I have plugins on the brain from working on HTMLEmbedElement I guess. :)

[Eric Seidel (no email)]

The ASSERT is from http://trac.webkit.org/changeset/67182 (as mentioned in several dups).

[Eric Seidel (no email)]

iframe loading being done in the renderer is also related to plugin (object/embed) loading being done in the renderer.  See bug 45049.

[Eric Seidel (no email)]

The question with this ASSERT is to figure out why we're ending up with a renderer() between when the DOM object creation happens and when insertedIntoDocument is called.

[Eric Seidel (no email)]

We may be hitting this ASSERT due to bad interactions with the magic-iframe code.  There was a security bug which added some of this complexity: https://bugs.webkit.org/show_bug.cgi?id=68267

[Eric Seidel (no email)]

It's possible that we're creating a renderer, then trying to move the frame.

void HTMLFrameElementBase::willRemove()
{
    if (m_remainsAliveOnRemovalFromTree)
        return;

    HTMLFrameOwnerElement::willRemove();
}

can block the zeroing of renderer(), but then 

void HTMLFrameElementBase::insertedIntoDocument()
{
    HTMLFrameOwnerElement::insertedIntoDocument();

    if (m_remainsAliveOnRemovalFromTree) {
        updateOnReparenting();
        setRemainsAliveOnRemovalFromTree(false);
        return;
    }

should early-return with a different path (before the ASSERT).

It's possible that m_remainsAliveOnRemovalFromTree is somehow changing between when the remove happens and when it's added back...  But I don't see how that would work (since m_remainsAliveOnRemovalFromTree is set by setRemainsAliveOnRemovalFromTree which is only called during Document::adoptNode)

[Dmitry Titov]

(In reply to comment #11)

I seem to have it reproducible, at least occasionally, with a url from bug 71177 in debug build of Chrome. Will try to debug...

[Dmitry Titov]

doesn't seem related to magic iframe. 

However, I'm not sure what to deduct from what I see, maybe this will help Eric: it appears that iframe element gets a renderer before it gets insertedIntoDocument() call, then the whole fragment gets inserted into a document... Here is the callstack where IFrame gets the renderer:


1   0x1a57597c WebCore::HTMLIFrameElement::createRenderer(WebCore::RenderArena*, WebCore::RenderStyle*)
2   0x1a4aa8fa WebCore::NodeRendererFactory::createRenderer()
3   0x1a4aace6 WebCore::NodeRendererFactory::createRendererIfNeeded()
4   0x1a46450d WebCore::Node::createRendererIfNeeded()
5   0x1a4181d0 WebCore::Element::attach()
6   0x1a56f860 WebCore::HTMLFrameElementBase::attach()
7   0x1a35df87 WebCore::ContainerNode::attach()
8   0x1a41820e WebCore::Element::attach()
9   0x1a35df87 WebCore::ContainerNode::attach()
10  0x1a41820e WebCore::Element::attach()
11  0x1a35df87 WebCore::ContainerNode::attach()
12  0x1a41820e WebCore::Element::attach()
13  0x1a35df87 WebCore::ContainerNode::attach()
14  0x1a41820e WebCore::Element::attach()
15  0x1a35df87 WebCore::ContainerNode::attach()
16  0x1a41820e WebCore::Element::attach()
17  0x1a35df87 WebCore::ContainerNode::attach()
18  0x1a41820e WebCore::Element::attach()
19  0x1a35df87 WebCore::ContainerNode::attach()
20  0x1a41820e WebCore::Element::attach()
21  0x1a35df87 WebCore::ContainerNode::attach()
22  0x1a41820e WebCore::Element::attach()
23  0x1a420d84 WebCore::Node::reattach()
24  0x1a418c2d WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
25  0x1a419313 WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
26  0x1a419313 WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
27  0x1a419313 WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
28  0x1a419313 WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
29  0x1a419313 WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
30  0x1a419313 WebCore::Element::recalcStyle(WebCore::Node::StyleChange)
31  0x1a38a5fc WebCore::Document::recalcStyle(WebCore::Node::StyleChange)
32  0x1a38be89 WebCore::Document::styleSelectorChanged(WebCore::StyleSelectorUpdateFlag)
33  0x1a391f17 WebCore::Document::removePendingSheet()
34  0x1a4fa71e WebCore::StyleElement::sheetLoaded(WebCore::Document*)
35  0x1a5e68f6 WebCore::HTMLStyleElement::sheetLoaded()
36  0x1ad7e6a4 WebCore::CSSStyleSheet::checkLoaded()
37  0x1a4fa4b0 WebCore::StyleElement::createSheet(WebCore::Element*, int, WTF::String const&)
38  0x1a4f99bf WebCore::StyleElement::process(WebCore::Element*)
39  0x1a4f96f3 WebCore::StyleElement::insertedIntoDocument(WebCore::Document*, WebCore::Element*)
40  0x1a5e631c WebCore::HTMLStyleElement::insertedIntoDocument()
41  0x1a35e15d WebCore::ContainerNode::insertedIntoDocument()
42  0x1a417bd1 WebCore::Element::insertedIntoDocument()
43  0x1a35e15d WebCore::ContainerNode::insertedIntoDocument()
44  0x1a417bd1 WebCore::Element::insertedIntoDocument()
45  0x1a35e15d WebCore::ContainerNode::insertedIntoDocument()
46  0x1a417bd1 WebCore::Element::insertedIntoDocument()
47  0x1a35bba2 WebCore::notifyChildInserted(WebCore::Node*)
48  0x1a35b1b1 WebCore::ContainerNode::appendChild(WTF::PassRefPtr<WebCore::Node>, int&, bool)
49  0x1a460666 WebCore::Node::appendChild(WTF::PassRefPtr<WebCore::Node>, int&, bool)
50  0x1ab8fa44 WebCore::V8Node::appendChildCallback(v8::Arguments const&)
51  0x19407466 v8::internal::MaybeObject* v8::internal::HandleApiCallHelper<false>(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*)
52  0x19406f2a v8::internal::Builtin_Impl_HandleApiCall(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*)
53  0x193fcd5d v8::internal::Builtin_HandleApiCall(v8::internal::(anonymous namespace)::BuiltinArguments<(v8::internal::BuiltinExtraArguments)1>, v8::internal::Isolate*)

[Dmitry Titov]

One more bit of data: the fragment of DOM is first assembled from script, then attached to the actual document. This triggers a tree walk with insertedIntoDocument() on each element. Before the tree walk reaches the iframe, it reaches the Style element. Since the whole subtree is now InDocument(), that triggers sync style recalc on a subtree, which includes the iframe. The iframe gets a renderer. Then, during the same tree walk, the iframe gets insertedIntoDocument() and fires that ASSERT.

[Eric Seidel (no email)]

Thank you.  I have a reduction now:

<body>
<script>
var subtree = document.createElement('div');
var styleElement = document.createElement('style');
styleElement.textContent = "iframe { border: 3px solid blue; }";
subtree.appendChild(styleElement);
subtree.appendChild(document.createElement('iframe'));
document.body.appendChild(subtree);
</script>

[Eric Seidel (no email)]

I originally wrote the following:

// Loads may cause synchronous javascript execution (e.g. beforeload or
// src=javascript), which could try to access the renderer before the normal
// parser machinery would call lazyAttach() and set us as needing style
// resolve.  Any code which expects this to be attached will resolve style
// before using renderer(), so this will make sure we attach in time.
if (!renderer()) {
    // This recalc is unecessary if we already have a renderer, as can be the case
    // if an earlier element's insertedIntoDocument() already forced a style recalc.
    // FIXME: Normally lazyAttach marks the renderer as attached(), but we don't
    // want to do that here, as as callers expect to call attach() right after
    // this and attach() will ASSERT(!attached())
    lazyAttach(DoNotSetAttached);
}

But I've since modified the code to just remove this lazyAttach.  As far as I can tell, it's unnecessary.  Unfortunately the tests are not behaving well on my machine, so I can't be 100% sure.

[Eric Seidel (no email)]

Created attachment 121333 [details]
Patch

[Eric Seidel (no email)]

We'll see if the EWS bots have any better luck running the tests than I did.

[WebKit Review Bot]

Comment on attachment 121333 [details]
Patch

Attachment 121333 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11147337

New failing tests:
fast/frames/frame-js-url-clientWidth.html
fast/frames/frameElement-widthheight.html
fast/frames/iframe-js-url-clientWidth.html
fast/forms/basic-textareas.html

[Eric Seidel (no email)]

Comment on attachment 121333 [details]
Patch

Sorry, I should have cleared the review.  I don't think this can be landed as-is.  I need to finish investigating.

[Eric Seidel (no email)]

Created attachment 123156 [details]
Patch

[WebKit Review Bot]

Comment on attachment 123156 [details]
Patch

Clearing flags on attachment: 123156

Committed r105463: <http://trac.webkit.org/changeset/105463>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.