child documents do not immediately clear their rendering trees when parent iframes are set to display: none
Created attachment 138518 [details]
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.)
It's not immediately clear to me who is holding onto the Widget* during the call to Element::offsetWidth in the example.
If this change does not "smell right" to others who have worked in the widget code, I'm happy to investigate further.
Created attachment 138523 [details]
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.
widgetRendererMap() is empty at this point too (so that's not the thing holding the Widget ref).
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));
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 on attachment 138523 [details]
Attachment 138523 [details] did not pass chromium-ews (chromium-xvfb):
New failing tests:
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
Can you run this test through IE10 and latest Firefox/Opera? It'd be good to make sure we're converging on behavior.
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.
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 on attachment 138523 [details]
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.
> + // 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.
> + 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?
> + window.wrapper = document.getElementById("wrapper").style.display = "none";
What is the window.wrapper part of this line for?
Grabbing this from Eric.
Created attachment 184976 [details]