Make Widget Ref-Counted.

[Beth Dakin]

Here is a patch that makes Widget RefCounted. This fixes three crashes that we know about:

<rdar://problem/7038831> REGRESSION (TOT): In Mail, a crash occurs at WebCore::Widget::afterMouseDown() after clicking To Do’s close box
<rdar://problem/6978804> WER #16: Repro Access Violation in WebCore::PluginView::bindingInstance (1310178023)
     -and-
<rdar://problem/6991251> WER #13: Crash in WebKit! WebCore::PluginView::performRequest+203 (1311461169)

[Beth Dakin]

Created attachment 32530 [details]
Patch

[Dave Hyatt]

Comment on attachment 32530 [details]
Patch


> -    const HashSet<Widget*>* viewChildren = children();
> -    HashSet<Widget*>::const_iterator end = viewChildren->end();
> -    for (HashSet<Widget*>::const_iterator current = viewChildren->begin(); current != end; ++current)
> -        if ((*current)->isFrameView())
> -            static_cast<FrameView*>(*current)->layoutIfNeededRecursive();
> +    const HashSet<RefPtr<Widget> >* viewChildren = children();
> +    HashSet<RefPtr<Widget> >::const_iterator end = viewChildren->end();
> +    for (HashSet<RefPtr<Widget> >::const_iterator current = viewChildren->begin(); current != end; ++current) {
> +        RefPtr<Widget> widget = (*current);
> +        if (widget->isFrameView())
> +            static_cast<FrameView*>(widget.get())->layoutIfNeededRecursive();
> +    }
>  

I don't think the change to use a temporary variable here is helpful, since you've created refcounting churn.  I think you should put it back the way it was and just use |current|.


> Index: WebCore/rendering/RenderApplet.cpp
> ===================================================================
> --- WebCore/rendering/RenderApplet.cpp	(revision 45662)
> +++ WebCore/rendering/RenderApplet.cpp	(working copy)
> @@ -26,6 +26,7 @@
>  #include "HTMLAppletElement.h"
>  #include "HTMLNames.h"
>  #include "HTMLParamElement.h"
> +#include "Widget.h"
>  
>  namespace WebCore {
>  
> @@ -71,7 +72,8 @@ void RenderApplet::createWidgetIfNecessa
>  
>      Frame* frame = document()->frame();
>      ASSERT(frame);
> -    setWidget(frame->loader()->createJavaAppletWidget(IntSize(contentWidth, contentHeight), element, m_args));
> +    RefPtr<Widget> widget = frame->loader()->createJavaAppletWidget(IntSize(contentWidth, contentHeight), element, m_args);
> +    setWidget(widget.get());
>  }
>  

I don't see any reason to use a temporary variable here?  You're at the end of the function, so can't you just say:

setWidget(frame->loader()->createJavaAppletWidget(...));

?

Other comments:

Shouldn't setWidget take a PassRefPtr now, since RenderWidgets will be taking ownership of the widget?

It seems like addChild should arguably take a PassRefPtr too, since the parent will also be taking ownership of the added child.

I take back my objections to this patch.   The fact that Scrollbars and FrameViews were already refcounted anyway makes this a really great cleanup.

Let me see one more round.  Thanks.

[Oliver Hunt]

Comment on attachment 32530 [details]
Patch

Can you make the ScrollView constructor protected so that people can't
accidentally go new ScrollView in future.

The WebKit/win/ChangeLog entry appears to be duplicated

[Anders Carlsson]

(In reply to comment #2)
> (From update of attachment 32530 [details])
> 
> > -    const HashSet<Widget*>* viewChildren = children();
> > -    HashSet<Widget*>::const_iterator end = viewChildren->end();
> > -    for (HashSet<Widget*>::const_iterator current = viewChildren->begin(); current != end; ++current)
> > -        if ((*current)->isFrameView())
> > -            static_cast<FrameView*>(*current)->layoutIfNeededRecursive();
> > +    const HashSet<RefPtr<Widget> >* viewChildren = children();
> > +    HashSet<RefPtr<Widget> >::const_iterator end = viewChildren->end();
> > +    for (HashSet<RefPtr<Widget> >::const_iterator current = viewChildren->begin(); current != end; ++current) {
> > +        RefPtr<Widget> widget = (*current);
> > +        if (widget->isFrameView())
> > +            static_cast<FrameView*>(widget.get())->layoutIfNeededRecursive();
> > +    }
> >  
> 
> I don't think the change to use a temporary variable here is helpful, since
> you've created refcounting churn.  I think you should put it back the way it
> was and just use |current|.
> 

It could still be a temporary variable, just as long as you don't make it a RefPtr.

Widget* widget = (*current).get();

[Beth Dakin]

Created attachment 32533 [details]
New Patch

Here's a new patch that takes all of these comments into account.

[Dave Hyatt]

Comment on attachment 32533 [details]
New Patch

Typo in ChangeLog:

"setWidget() now takes a PassRefPtr. Also eemoved the manual ref of "

"eemoved" should be "removed".

ScrollView::platformAddChild never takes ownership of the Widget, so I don't think you needed to change any of those implementations. Those should just be platformAddChild(Widget*) still.  You can undo that change and avoid having to touch a bunch of ScrollView port files.

With those changes, r=me!

[Dave Hyatt]

There is also some refcounting churn in addChild that you could avoid by using a raw local instead of a RefPtr local.  Then use the PassRefPtr when passing in to m_children.add().

[Beth Dakin]

Fixed with r45679.

[Mark Rowe (bdash)]

Should this bug be closed then?

[Beth Dakin]

Yes!