Bug 84684 - child documents do not immediately clear their rendering trees when parent iframes are set to display: none
Summary: child documents do not immediately clear their rendering trees when parent if...
Status: NEW
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-04-24 01:48 PDT by Eric Seidel (no email)
Modified: 2017-07-18 08:27 PDT (History)
13 users (show)

See Also:


Attachments
Patch (3.75 KB, patch)
2012-04-24 01:49 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Patch (3.75 KB, patch)
2012-04-24 02:14 PDT, Eric Seidel (no email)
no flags Details | Formatted Diff | Diff
Stack of when this code is hit (5.75 KB, text/plain)
2012-04-24 02:18 PDT, Eric Seidel (no email)
no flags Details
Archive of layout-test-results from ec2-cr-linux-01 (6.75 MB, application/zip)
2012-04-24 12:36 PDT, WebKit Review Bot
no flags Details
WIP (6.12 KB, patch)
2013-01-28 06:20 PST, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Eric Seidel (no email) 2012-04-24 01:48:12 PDT
child documents do not immediately clear their rendering trees when parent iframes are set to display: none
Comment 1 Eric Seidel (no email) 2012-04-24 01:49:30 PDT
Created attachment 138518 [details]
Patch
Comment 2 Eric Seidel (no email) 2012-04-24 01:51:32 PDT
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.)
Comment 3 Eric Seidel (no email) 2012-04-24 01:53:57 PDT
It's not immediately clear to me who is holding onto the Widget* during the call to Element::offsetWidth in the example.
Comment 4 Eric Seidel (no email) 2012-04-24 01:54:18 PDT
If this change does not "smell right" to others who have worked in the widget code, I'm happy to investigate further.
Comment 5 Eric Seidel (no email) 2012-04-24 02:14:58 PDT
Created attachment 138523 [details]
Patch
Comment 6 Eric Seidel (no email) 2012-04-24 02:18:03 PDT
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.
Comment 7 Eric Seidel (no email) 2012-04-24 02:18:43 PDT
widgetRendererMap() is empty at this point too (so that's not the thing holding the Widget ref).
Comment 8 Eric Seidel (no email) 2012-04-24 02:26:26 PDT
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));
Comment 9 Eric Seidel (no email) 2012-04-24 02:37:52 PDT
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.
Comment 10 WebKit Review Bot 2012-04-24 12:35:55 PDT
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
Comment 11 WebKit Review Bot 2012-04-24 12:36:11 PDT
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
Comment 12 Ojan Vafai 2012-04-24 13:31:54 PDT
Can you run this test through IE10 and latest Firefox/Opera? It'd be good to make sure we're converging on behavior.
Comment 13 James Robinson 2012-04-24 14:09:46 PDT
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.
Comment 14 Eric Seidel (no email) 2012-05-01 20:39:54 PDT
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.
Comment 15 Darin Adler 2013-01-16 11:17:44 PST
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?
Comment 16 Mike West 2013-01-28 02:29:14 PST
Grabbing this from Eric.
Comment 17 Mike West 2013-01-28 06:20:18 PST
Created attachment 184976 [details]
WIP