Bug 84684 – child documents do not immediately clear their rendering trees when parent iframes are set to display: none

• Component: Layout and Rendering
• Status: NEW

[Eric Seidel (no email)]

child documents do not immediately clear their rendering trees when parent iframes are set to display: none

[Eric Seidel (no email)]

Created attachment 138518 [details]
Patch

[Eric Seidel (no email)]

Test originally from: http://webstuff.nfshost.com/tests/outer.html

This came up in trying to understand propagation of style recalcs between documents for <iframe seamless>.  However the test case is rooted in a ref-counting oddity of Widget, rather than a lack of layout/style propagation.  (So i'm still investigating my <iframe seamless> bug, but this should be the correct fix for this issue.)

[Eric Seidel (no email)]

It's not immediately clear to me who is holding onto the Widget* during the call to Element::offsetWidth in the example.

[Eric Seidel (no email)]

If this change does not "smell right" to others who have worked in the widget code, I'm happy to investigate further.

[Eric Seidel (no email)]

Created attachment 138523 [details]
Patch

[Eric Seidel (no email)]

Created attachment 138524 [details]
Stack of when this code is hit

Note that when we're in this stack, the m_refCount is 1 (even after setWidget(0) is called).

It's not immediately obvious to me who is holding onto the reference to the Widget* at this point.

[Eric Seidel (no email)]

widgetRendererMap() is empty at this point too (so that's not the thing holding the Widget ref).

[Eric Seidel (no email)]

Oh.  Nevermind.  So of course m_refCount is still 1 here.  The RendeWidget is appropriately getting released here, but it's unclear to me who's responsibility it is to empty the FrameView of its contents is.

It makes some sense to me that RenderWidget should be (which is why I wrote this patch).

It's getting late and my brain is fogging, so I'll come back to this tomrorow.  I still believe this patch is correct.

I should also note, that when this code is hit, the m_widget has a refCount of 3 (2 after setWidget(0));

[Eric Seidel (no email)]

Another solution to this would be to add some sort of virtual method on Widget which is called when the widget is removed from the Widget tree.  FrameView could implement that and tear down its Document's rendertree at that point.

[WebKit Review Bot]

Comment on attachment 138523 [details]
Patch

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

New failing tests:
scrollbars/hidden-iframe-scrollbar-crash2.html
fullscreen/full-screen-iframe-legacy.html
fullscreen/full-screen-iframe-allowed.html
fast/dom/HTMLObjectElement/children-changed.html
fast/events/mouseover-mouseout2.html
fullscreen/full-screen-iframe-zIndex.html
fullscreen/full-screen-restrictions.html

[WebKit Review Bot]

Created attachment 138620 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Ojan Vafai]

Can you run this test through IE10 and latest Firefox/Opera? It'd be good to make sure we're converging on behavior.

[James Robinson]

This patch moves us to match Firefox.  Opera does something crazy (the offsetWidth inside the iframe seems to never be 0 even if the iframe is display:none, which is silly).  I haven't checked IE.

[Eric Seidel (no email)]

It's unclear to me why my change should have caused fullscreen tests to fail.  Presumably fullscreen is moving widgets and expecting the rendering tree to survive.

I may try a different approach to fixing this, by instead having HTMLIFrameElement propogate the recalcStyle down into the child document whenever the StyleChange is >= Detach.  That has the potential to share code with seamless which is nice.

[Darin Adler]

Comment on attachment 138523 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=138523&action=review

This seems good, but I’m going to say review- because both the test and the code change are not quite right yet, in my opinion.

> Source/WebCore/rendering/RenderWidget.cpp:123
> +    // If the widget is a child FrameView and has been destroyed
> +    // clear the child's render tree now, even though references
> +    // to this widget may live on for a bit.

Nothing in this comment makes it clear that this is needed for correctness. From the way this is worded it sounds like a performance optimization.

> Source/WebCore/rendering/RenderWidget.cpp:128
> +        FrameView* frameView = static_cast<FrameView*>(m_widget.get());
> +        Document* childDocument = frameView->frame()->document();
> +        if (childDocument->attached())
> +            childDocument->detach();

This seems like it should be done with a suitably-named FrameView function rather than code reaching through the frame view and doing this directly on the document. Maybe even a virtual function on Widget?

> LayoutTests/fast/frames/ancestor-style-change.html:10
> +    window.wrapper = document.getElementById("wrapper").style.display = "none";

What is the window.wrapper part of this line for?

[Mike West]

Grabbing this from Eric.

[Mike West]

Created attachment 184976 [details]
WIP