Move the Widget hierarchy to the Oilpan heap.

Move the Widget hierarchy to the Oilpan heap.

Turn Widget into a GCable object, along with its derived objects
(PluginView, FrameView, Scrollbar, PopupContainer).

R=haraken
BUG=340522
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=183583

[sof, 2014-09-25 22:04:08]

sigbjornf@opera.com changed reviewers:
+ oilpan-reviews@chromium.org

[sof, 2014-09-25 22:04:09]

Please take a look.

This is now ready for some inspection. At the outset, moving Widget to the heap
was motivated by the following:

 - Wanted to remove all Persistents on FrameView, and just generally simplify
the relationship between it and LocalFrame.
 - Have the WebPluginContainerImpl finalization become a bit simpler also.

This provides both, along with making the other Widget derived objects be heap
allocated. However, some of the disposal and finalization steps "feels" as if
they emulate refptr-like lifetime handling too closely, so as to be correct and
safe.

IOW, I think it still an open question if this is something we want to do over
how Widget/Scrollbar/FrameView/WebPluginContainer is currently handled.

[haraken, 2014-09-26 09:19:23]

haraken@chromium.org changed reviewers:
+ haraken@chromium.org

[haraken, 2014-09-26 09:19:25]

Thanks for the complicated CL :-)

Here is the first round of comments!

https://codereview.chromium.org/603193005/diff/40001/Source/core/accessibilit...
File Source/core/accessibility/AXScrollView.h (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/accessibilit...
Source/core/accessibility/AXScrollView.h:83: ScrollView* m_scrollView;

Just to confirm: m_scrollView shouldn't become a stale pointer because
m_scrollView is explicitly cleared in AXScrollView::detach(), right?

https://codereview.chromium.org/603193005/diff/40001/Source/core/css/MediaQue...
File Source/core/css/MediaQueryEvaluatorTest.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluatorTest.cpp:80:
RefPtrWillBePersistent<MediaQuerySet> querySet = nullptr;

RefPtrWillBeRawPtr ?

https://codereview.chromium.org/603193005/diff/40001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluatorTest.cpp:129:
RefPtrWillBePersistent<FrameView> view = pageHolder->frame().view();

RefPtrWillBeRawPtr ?

https://codereview.chromium.org/603193005/diff/40001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluatorTest.cpp:132:
RefPtrWillBePersistent<MediaQuerySet> querySet =
MediaQuerySet::create("foobar");

RefPtrWillBeRawPtr ?

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:175: setHasVerticalScrollbar(false);

Would you elaborate on why it's OK to remove setHasHorizontalScrollbar() and
setHasVerticalScrollbar() from oilpan builds?

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:256: setScrollbarModes(ScrollbarAuto,
ScrollbarAuto);

Can we share this code with enabledAutoSizeMode() ?

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:215: m_view->dispose();

Just help me understand: Why is it OK to promptly dispose the FrameView here?
Isn't it possible there are other holders of the FrameView?

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/Remote...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/Remote...
Source/core/frame/RemoteFrame.cpp:42: m_view = view;

Don't we need to call m_view->dispose() here?

https://codereview.chromium.org/603193005/diff/40001/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:45:
DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<WidgetToParentMap>, map,
(adoptPtrWillBeNoop(new WidgetToParentMap())));

Just to confirm: This is a persistent map of strong members. Won't it cause
leaks?

https://codereview.chromium.org/603193005/diff/40001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:184: toPluginView(widget)->dispose();

Hmm, the fact that the plugin container needs a prompt disposal adds a lot of
complexity to the code base. Can't we somehow remove the need of the prompt
disposal from the plugin container? (I have no idea though.)

https://codereview.chromium.org/603193005/diff/40001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.h (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.h:162: RefPtrWillBePersistent<Widget>
m_persistedPluginWidget;

Can we avoid this persistent handle by using the disposer pattern?

  PersistentHeapHashMap<WeakMember<HTMLFrameOwnerElement>,
OwnPtr<HTMLFrameOwnerElementDisposer>>;

You can destruct the Widget in the HTMLFrameOwnerElementDisposer's destructor.

https://codereview.chromium.org/603193005/diff/40001/Source/core/page/DragCon...
File Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/page/DragCon...
Source/core/page/DragController.cpp:219: if (RefPtrWillBeRawPtr<FrameView> v
ALLOW_UNUSED = mainFrame->view()) {

Why is ALLOW_UNUSED needed?

https://codereview.chromium.org/603193005/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderScrollbar.h (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderScrollbar.h:96:
WillBePersistentHeapHashMap<unsigned, RawPtrWillBeMember<RenderScrollbarPart> >
m_parts;

Can we use a disposer pattern to delay the finalization of the
RenderScrollbarPart objects?

  PersistentHeapHashMap<WeakMember<RenderScrollbar>,
OwnPtr<RenderScrollbarDisposer>>;


You can destruct RenderScrollbarPart objects in the destructor of
RenderScrollbarDisposer.

https://codereview.chromium.org/603193005/diff/40001/Source/platform/scroll/S...
File Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/platform/scroll/S...
Source/platform/scroll/Scrollbar.cpp:100: 

Shall we add ASSERT(m_scrollableArea) ?

https://codereview.chromium.org/603193005/diff/40001/Source/platform/scroll/S...
File Source/platform/scroll/Scrollbar.h (right):

https://codereview.chromium.org/603193005/diff/40001/Source/platform/scroll/S...
Source/platform/scroll/Scrollbar.h:175: // when it is destructed.

Is there any reason you cannot simply use m_scrollableArea->animator() in
Scrollbar's destructor?

https://codereview.chromium.org/603193005/diff/40001/Source/web/PopupContaine...
File Source/web/PopupContainer.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/web/PopupContaine...
Source/web/PopupContainer.cpp:102: // FIXME: can ~PopupListBox take care this?

I'm not sure if this is a FIXME.

Now that the PopupListBox and the PopupContainer live and die together, we don't
need the setParent(0) code in the destructor, do we?

https://codereview.chromium.org/603193005/diff/40001/Source/web/PopupListBox.h
File Source/web/PopupListBox.h (right):

https://codereview.chromium.org/603193005/diff/40001/Source/web/PopupListBox....
Source/web/PopupListBox.h:285: Vector<PopupItem*> m_items;

Not related to this CL: We should use Vector<OwnPtr<PopupItem>>. Then we can
remove clear() from PopupListBox's destructor.

https://codereview.chromium.org/603193005/diff/40001/Source/web/PopupMenuChro...
File Source/web/PopupMenuChromium.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/web/PopupMenuChro...
Source/web/PopupMenuChromium.cpp:54: hide();

Help me understand: Where are disconnectClient and hide() called in oilpan
builds?

https://codereview.chromium.org/603193005/diff/40001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/603193005/diff/40001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.h:207:
RawPtrWillBeWeakMember<HTMLPlugInElement> m_element;

This doesn't need any weak semantics, so I'd prefer using a RawPtr.

[sof, 2014-09-26 09:41:05]

https://codereview.chromium.org/603193005/diff/40001/Source/core/css/MediaQue...
File Source/core/css/MediaQueryEvaluatorTest.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluatorTest.cpp:80:
RefPtrWillBePersistent<MediaQuerySet> querySet = nullptr;
On 2014/09/26 09:19:25, haraken wrote:
> 
> RefPtrWillBeRawPtr ?

We had several rounds of this in ImageBitmapTest & the conclusion is that we
need to use RefPtrWillBePersistent<> in tests to reliably hold onto heap
objects.

Should a GC strike from within this test method, it won't scan the stack. Unless
something has changed recently.

[haraken, 2014-09-26 09:50:26]

https://codereview.chromium.org/603193005/diff/40001/Source/core/css/MediaQue...
File Source/core/css/MediaQueryEvaluatorTest.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/css/MediaQue...
Source/core/css/MediaQueryEvaluatorTest.cpp:80:
RefPtrWillBePersistent<MediaQuerySet> querySet = nullptr;
On 2014/09/26 09:41:05, sof wrote:
> On 2014/09/26 09:19:25, haraken wrote:
> > 
> > RefPtrWillBeRawPtr ?
> 
> We had several rounds of this in ImageBitmapTest & the conclusion is that we
> need to use RefPtrWillBePersistent<> in tests to reliably hold onto heap
> objects.
> 
> Should a GC strike from within this test method, it won't scan the stack.
Unless
> something has changed recently.

oh, this is in a test. Makes sense to use a Persistent.

[sof, 2014-09-28 08:05:53]

https://codereview.chromium.org/603193005/diff/40001/Source/core/accessibilit...
File Source/core/accessibility/AXScrollView.h (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/accessibilit...
Source/core/accessibility/AXScrollView.h:83: ScrollView* m_scrollView;
On 2014/09/26 09:19:25, haraken wrote:
> 
> Just to confirm: m_scrollView shouldn't become a stale pointer because
> m_scrollView is explicitly cleared in AXScrollView::detach(), right?

That's correct, and detaching happens when the AX object cache is updated(and
finalized.)

I don't understand (yet) why it is non-leaky to keep Persistent<> back
references from AXObject-derived objects to on-heap ones, esp something as "big"
as ScrollView, so I've not done so here.

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:175: setHasVerticalScrollbar(false);
On 2014/09/26 09:19:25, haraken wrote:
> 
> Would you elaborate on why it's OK to remove setHasHorizontalScrollbar() and
> setHasVerticalScrollbar() from oilpan builds?

It is not OK to have those scrollbars not do the equivalent of what
setHas*Scrollbar(false) does, but we clearly cannot achieve that from within
this finalizer.

Instead, the Scrollbars have access to the ScrollAnimator of their ScrollView
(which FrameView derives from) that they need to unregister with and will take
care of that bit themselves. See the Oilpan-specific parts in ~Scrollbar.

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:215: m_view->dispose();
On 2014/09/26 09:19:25, haraken wrote:
> 
> Just help me understand: Why is it OK to promptly dispose the FrameView here?
> Isn't it possible there are other holders of the FrameView?

A key question. I think it would be safer if the disposal happened at the
outermost "protect" holder, should there be any. This would only be observable
with Oilpan enabled, but the FrameViews of detached frames could be observable
by others as being in a disposed state.

The only way (I know of) to delay the disposal is to have a custom
protector-like RAII object, like what was proposed for as-eager-as-could-be
frame finalization. But perhaps there are other ways to go about it; thoughts?

[sof, 2014-09-28 08:20:32]

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/LocalF...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/LocalF...
Source/core/frame/LocalFrame.cpp:215: m_view->dispose();
On 2014/09/28 08:05:53, sof wrote:
> On 2014/09/26 09:19:25, haraken wrote:
> > 
> > Just help me understand: Why is it OK to promptly dispose the FrameView
here?
> > Isn't it possible there are other holders of the FrameView?
> 
> A key question. I think it would be safer if the disposal happened at the
> outermost "protect" holder, should there be any. This would only be observable
> with Oilpan enabled, but the FrameViews of detached frames could be observable
> by others as being in a disposed state.
> 
> The only way (I know of) to delay the disposal is to have a custom
> protector-like RAII object, like what was proposed for as-eager-as-could-be
> frame finalization. But perhaps there are other ways to go about it; thoughts?

Forgot to say -- no tests exercise code paths that rely on there not being a
disposed FrameView upon the return of a call that detached a frame. I don't know
if any of the Frame/FrameView protected uses depend on that not happening; but
it seems like a subtle, extra assumption to add.

[sof, 2014-09-28 15:11:26]

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/FrameV...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/FrameV...
Source/core/frame/FrameView.cpp:256: setScrollbarModes(ScrollbarAuto,
ScrollbarAuto);
On 2014/09/26 09:19:25, haraken wrote:
> 
> Can we share this code with enabledAutoSizeMode() ?

Not sure I fully understand the suggestion, but shared a bit of code wrt fixing
size to frame size.

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/Remote...
File Source/core/frame/RemoteFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/Remote...
Source/core/frame/RemoteFrame.cpp:42: m_view = view;
On 2014/09/26 09:19:25, haraken wrote:
> 
> Don't we need to call m_view->dispose() here?

Not needed, as ~RemoteFrameView is empty.

https://codereview.chromium.org/603193005/diff/40001/Source/core/html/HTMLFra...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:45:
DEFINE_STATIC_LOCAL(OwnPtrWillBePersistent<WidgetToParentMap>, map,
(adoptPtrWillBeNoop(new WidgetToParentMap())));
On 2014/09/26 09:19:25, haraken wrote:
> 
> Just to confirm: This is a persistent map of strong members. Won't it cause
> leaks?

The map contains shortlived (Widget, FrameView) pairs -- put there when widget
hierarchy updates are temporarily suspended -- so this will not retain objects
any longer than it needs to.

https://codereview.chromium.org/603193005/diff/40001/Source/core/html/HTMLFra...
Source/core/html/HTMLFrameOwnerElement.cpp:184: toPluginView(widget)->dispose();
On 2014/09/26 09:19:25, haraken wrote:
> 
> Hmm, the fact that the plugin container needs a prompt disposal adds a lot of
> complexity to the code base. Can't we somehow remove the need of the prompt
> disposal from the plugin container? (I have no idea though.)

When the plugin is destroyed, the underlying plugin may perform an arbitrary
amount of work, which may well involve accessing the frame and its document etc.

So, this needs to be done before any of those are finalized. Doing it at a later
stage (e.g., as part of finalizing the plugin container itself) is just not
safe, I came to realize. I agree that it is not simple to handle correctly &
reliably - it's driven me to distraction more than once :) - but the complexity
seems unavoidable in general.

(This issue touches directly on https://codereview.chromium.org/603193005/#msg2
-- the careful use of dispose() over PluginViews feels unnatural and low-level.
But I don't have any better alternatives other than not moving Widget to the
heap. Not what I suggest we do, but it's an option.)

https://codereview.chromium.org/603193005/diff/40001/Source/core/page/DragCon...
File Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/page/DragCon...
Source/core/page/DragController.cpp:219: if (RefPtrWillBeRawPtr<FrameView> v
ALLOW_UNUSED = mainFrame->view()) {
On 2014/09/26 09:19:25, haraken wrote:
> 
> Why is ALLOW_UNUSED needed?

I didn't want to perturb this code too much (creating a scoped FrameView protect
+ checking that it is non-null, in one step), but the FrameView isn't used
within the "if".

And as RawPtr() has a trivial destructor, compilers will complain that 'v' is
bound, but not used.

https://codereview.chromium.org/603193005/diff/40001/Source/web/PopupContaine...
File Source/web/PopupContainer.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/web/PopupContaine...
Source/web/PopupContainer.cpp:102: // FIXME: can ~PopupListBox take care this?
On 2014/09/26 09:19:25, haraken wrote:
> 
> I'm not sure if this is a FIXME.
> 
> Now that the PopupListBox and the PopupContainer live and die together, we
don't
> need the setParent(0) code in the destructor, do we?

It's a non-Oilpan FIXME, but I will just leave it be & have removed the
question. It is tidier to remove yourself as parent in the dtor, and perhaps
this is somehow needed should the listbox be able to survive past this
PopupContainer. ?

(Not an issue with Oilpan, as the two will be finalized in the same sweep.)

[sof, 2014-09-28 17:03:52]

https://codereview.chromium.org/603193005/diff/40001/Source/core/html/HTMLPlu...
File Source/core/html/HTMLPlugInElement.h (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/html/HTMLPlu...
Source/core/html/HTMLPlugInElement.h:162: RefPtrWillBePersistent<Widget>
m_persistedPluginWidget;
On 2014/09/26 09:19:25, haraken wrote:
> 
> Can we avoid this persistent handle by using the disposer pattern?
> 
>   PersistentHeapHashMap<WeakMember<HTMLFrameOwnerElement>,
> OwnPtr<HTMLFrameOwnerElementDisposer>>;
> 
> You can destruct the Widget in the HTMLFrameOwnerElementDisposer's destructor.

I don't think that would work: disposing of the widget by the time the
HTMLFrameOwnerElement is GCed would be too late. i.e., this needs to happen when
the HTMLPlugInElement is detached/disconnected
(HTMLPlugInElement::disconnectContentFrame().)

https://codereview.chromium.org/603193005/diff/40001/Source/web/PopupMenuChro...
File Source/web/PopupMenuChromium.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/web/PopupMenuChro...
Source/web/PopupMenuChromium.cpp:54: hide();
On 2014/09/26 09:19:25, haraken wrote:
> 
> Help me understand: Where are disconnectClient and hide() called in oilpan
> builds?

Thanks, I didn't follow through on this dtor change. Only hide() needs to be
called, which is now done when the RenderMenuList is destroy()ed (via
disconnectClient().)

The clearing of the 'client' is not really needed, but to keep it regular, it's
also done w/ Oilpan.

https://codereview.chromium.org/603193005/diff/40001/Source/web/WebPluginCont...
File Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/603193005/diff/40001/Source/web/WebPluginCont...
Source/web/WebPluginContainerImpl.h:207:
RawPtrWillBeWeakMember<HTMLPlugInElement> m_element;
On 2014/09/26 09:19:25, haraken wrote:
> 
> This doesn't need any weak semantics, so I'd prefer using a RawPtr.

ok, made it HTMLPlugInElement* again (and adjusted comment.)

[sof, 2014-09-28 21:11:25]

https://codereview.chromium.org/603193005/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderScrollbar.h (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderScrollbar.h:96:
WillBePersistentHeapHashMap<unsigned, RawPtrWillBeMember<RenderScrollbarPart> >
m_parts;
On 2014/09/26 09:19:25, haraken wrote:
> 
> Can we use a disposer pattern to delay the finalization of the
> RenderScrollbarPart objects?
> 
>   PersistentHeapHashMap<WeakMember<RenderScrollbar>,
> OwnPtr<RenderScrollbarDisposer>>;
> 
> 
> You can destruct RenderScrollbarPart objects in the destructor of
> RenderScrollbarDisposer.

That map would have to be global, right? RenderScrollbarPart is also on the
heap, so you presumably would have to generalize to something like

  PersistentHeapHashMap<WeakMember<RenderScrollbar>,
Member<RenderScrollbarPartSet>>

where RenderScrollbarPartSet would be

class RenderScrollbarPartSet : public GarbageCollected<..> {
 ...
 private:
     HeapHashSet<unsigned, Member<RenderScrollbarPart>> m_parts;
};

and then have a weak callback on the HeapHashMap (via a wrapper object that's
also on the heap & what the Persistent<> above holds onto. That weak callback
would then do the needed disposal of the RenderScrollbarPartSet.

An indirect arrangement, but it would save having a per RenderScrollbar
persistent. Does it have other merits?

https://codereview.chromium.org/603193005/diff/40001/Source/platform/scroll/S...
File Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/603193005/diff/40001/Source/platform/scroll/S...
Source/platform/scroll/Scrollbar.cpp:100: 
On 2014/09/26 09:19:25, haraken wrote:
> 
> Shall we add ASSERT(m_scrollableArea) ?

Sure, why not.

https://codereview.chromium.org/603193005/diff/40001/Source/platform/scroll/S...
File Source/platform/scroll/Scrollbar.h (right):

https://codereview.chromium.org/603193005/diff/40001/Source/platform/scroll/S...
Source/platform/scroll/Scrollbar.h:175: // when it is destructed.
On 2014/09/26 09:19:25, haraken wrote:
> 
> Is there any reason you cannot simply use m_scrollableArea->animator() in
> Scrollbar's destructor?

ScrollableArea is a mixin of ScrollView, which is on the heap, so that's not
doable.

(Yes, we should probably declare ScrollableArea as a GC mixin and have it be a
traced Scrollbar Member.)

[haraken, 2014-09-29 14:16:37]

Thanks for working on the complicated bit.

The main concern is the PersistentHeapHashMap in RenderScrollbar.h and the
Persistent handle in HTMLPlugInElement.h. These persistent handles look a bit
too hacky and we might want to look for a better approach for this. (My below
comments wouldn't be that helpful though.) Other parts look good.

https://codereview.chromium.org/603193005/diff/100001/Source/core/accessibili...
File Source/core/accessibility/AXScrollView.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/accessibili...
Source/core/accessibility/AXScrollView.h:82: // could create a leak.

Would you update this comment to clarify that using a raw pointer for
m_scrollView is fine because m_scrollView is explicitly cleared in
AXScrollView::detach() ?

https://codereview.chromium.org/603193005/diff/100001/Source/core/accessibili...
File Source/core/accessibility/AXScrollbar.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/accessibili...
Source/core/accessibility/AXScrollbar.h:64: RefPtrWillBePersistent<Scrollbar>
m_scrollbar;

Hmm, it seems that the fact that AXScrollbar can hold a RefPtrWillBePersistent
to the corresponding Scrollbar implies that AXScrollView can hold a
RefPtrWillBePersistent to the corresponding ScrollView. I wonder why
AXScrollbar's RefPtrWillBePersistent is OK but AXScrollView's
RefPtrWillBePersistent is not OK.

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:155: #if !ENABLE(OILPAN)

Can we add an assertion to oilpan builds to verify that dispose() has been
already called before the destructor is called?

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1526: RefPtrWillBeMember<FrameView>
protect(this);

RefPtrWillBeRawPtr

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:2649: void
FrameView::setLayoutSizeFixedToFrameSizeAndInvalidate(bool isFixed)

Sorry, I now understand that adding this helper function isn't that helpful...

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:219: if (m_view && !view)

What is the '&& !view' check for?

How about calling m_view->dispose() in FrameView::prepareForDetach() ?

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Remot...
File Source/core/frame/RemoteFrameView.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Remot...
Source/core/frame/RemoteFrameView.h:16: class RemoteFrameView FINAL : public
Widget {

Nit: FINAL => final (according to the new coding style rule)

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:177: void
HTMLFrameOwnerElement::disposeWidget(Widget* widget)

Can we avoid adding this static method by defining virtual Widget::dispose() and
overriding it in PluginView ?

Instead of calling HTMLFrameOwnerElement::disposeWidget(child), you can just
call child->dispose().

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:179: // Oilpan: a plugin container
must be disposed of prior to (GC)

I don't fully understand this well, but what happens if we introduce:

  PersistentHeapHashMap<WeakMember<Widget>, OwnPtr<WidgetDisposer>>;

?

Then can we avoid introducing Widget::dispose()?

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:183: if (widget &&
widget->isPluginView())

Is it possible that this method is called with a null widget?

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:83:
HTMLFrameOwnerElement::disposeWidget(m_persistedPluginWidget.get());

Can't we call HTMLFrameOwnerElement::disposeWidget when the HTMLPlugInElement is
removed from the DOM tree?

Ideally we want to call disposeWidget in HTMLPlugInElement::removedFrom() if the
Widget dies before the HTMLPlugInElement, and don't call anything if the Widget
and the HTMLPlugInElement die together.

Then we want to make m_persistedPluginWidget a Member<Widget>.

I wonder what prevents us from doing something like that.

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.h:162: RefPtrWillBePersistent<Widget>
m_persistedPluginWidget;

This is a bit too tricky... I think we need to seek a way to make this a Member.
See my comment in HTMLPluginElement.cpp as well.

https://codereview.chromium.org/603193005/diff/100001/Source/core/page/DragCo...
File Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/page/DragCo...
Source/core/page/DragController.cpp:219: if (RefPtrWillBeRawPtr<FrameView> v
ALLOW_UNUSED = mainFrame->view()) {

How about writing like this?

RefPtrWillBeRawPtr<FrameView> frameView(mainFrame->view());
if (frameView) {
  ...;
}

Will it still cause a compilation issue?

https://codereview.chromium.org/603193005/diff/100001/Source/core/plugins/Plu...
File Source/core/plugins/PluginView.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/plugins/Plu...
Source/core/plugins/PluginView.h:43: class ScriptController;

Why is this needed?

https://codereview.chromium.org/603193005/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderScrollbar.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbar.cpp:77: // to call on a destroyed
scrollbar. See webkit bug 68009.

The culprit of the complexity is this bug. How hard do you think to fix the bug?

https://codereview.chromium.org/603193005/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderScrollbar.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbar.h:96:
WillBePersistentHeapHashMap<unsigned, RawPtrWillBeMember<RenderScrollbarPart> >
m_parts;

Hmm, this looks too tricky as well. The RenderScrollbarPart has a raw pointer
back to this RenderScrollBar. Once we make the raw pointer a
Member<RenderScrollBar> (conceptually we should do this), we will end up
creating a cycle... I guess we're relying on too many fragile assumptions here.

https://codereview.chromium.org/603193005/diff/100001/Source/platform/Widget.h
File Source/platform/Widget.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/platform/Widget....
Source/platform/Widget.h:132: virtual void detach() { }

When is Widget::detach() called?

It's a bit confusing that the Widget hierarchy has both detach() and dispose()
(PluginView has dispose()).

https://codereview.chromium.org/603193005/diff/100001/Source/platform/exporte...
File Source/platform/exported/WebScrollbarImpl.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/platform/exporte...
Source/platform/exported/WebScrollbarImpl.h:35: class PLATFORM_EXPORT
WebScrollbarImpl FINAL : public WebScrollbar {

FINAL => final

https://codereview.chromium.org/603193005/diff/100001/Source/platform/scroll/...
File Source/platform/scroll/ScrollableArea.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/platform/scroll/...
Source/platform/scroll/ScrollableArea.cpp:96: 

mis-edit.

https://codereview.chromium.org/603193005/diff/100001/Source/platform/scroll/...
File Source/platform/scroll/Scrollbar.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.h:167: ScrollableArea* m_scrollableArea;

Given that ScrollableArea is on the heap, can we make this a Member?

https://codereview.chromium.org/603193005/diff/100001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.h:170: ScrollbarTheme* m_theme;

Are you sure that this raw pointer doesn't become a stale pointer?

https://codereview.chromium.org/603193005/diff/100001/Source/web/PopupListBox...
File Source/web/PopupListBox.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/web/PopupListBox...
Source/web/PopupListBox.cpp:853: deleteAllValues(m_items);

It would be better to use Vector<OwnPtr<PopupItem>> though.

https://codereview.chromium.org/603193005/diff/100001/Source/web/PopupMenuChr...
File Source/web/PopupMenuChromium.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/web/PopupMenuChr...
Source/web/PopupMenuChromium.cpp:50: #if !ENABLE(OILPAN)

Can we add an assert to oilpan builds to verify that dispose() has been already
called?

https://codereview.chromium.org/603193005/diff/100001/Source/web/PopupMenuChr...
Source/web/PopupMenuChromium.cpp:97: dispose();

How about making non-oilpan builds call dispose() here as well?

(We don't want to have different behavior between oilpan builds and non-oilpan
builds as much as possible.)

https://codereview.chromium.org/603193005/diff/100001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.cpp:699: void
WebPluginContainerImpl::dispose()

Shall we clear m_element in dispose() ?

[sof, 2014-09-29 14:37:51]

On 2014/09/29 14:16:37, haraken wrote:
> Thanks for working on the complicated bit.
> 
> The main concern is the PersistentHeapHashMap in RenderScrollbar.h and the
> Persistent handle in HTMLPlugInElement.h. These persistent handles look a bit
> too hacky and we might want to look for a better approach for this. (My below
> comments wouldn't be that helpful though.) Other parts look good.
> 

Thanks for all the feedback and questions. Will let this CL rest a bit.

[sof, 2014-10-02 14:03:54]

https://codereview.chromium.org/603193005/diff/100001/Source/core/page/DragCo...
File Source/core/page/DragController.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/page/DragCo...
Source/core/page/DragController.cpp:219: if (RefPtrWillBeRawPtr<FrameView> v
ALLOW_UNUSED = mainFrame->view()) {
On 2014/09/29 14:16:36, haraken wrote:
> 
> How about writing like this?
> 
> RefPtrWillBeRawPtr<FrameView> frameView(mainFrame->view());
> if (frameView) {
>   ...;
> }
> 
> Will it still cause a compilation issue?

Sure, that will work - I just didn't want to accidentally extend the scope of
this protection here & break something for not much gain. But I've now verified
that this not an issue.

https://codereview.chromium.org/603193005/diff/100001/Source/core/plugins/Plu...
File Source/core/plugins/PluginView.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/plugins/Plu...
Source/core/plugins/PluginView.h:43: class ScriptController;
On 2014/09/29 14:16:36, haraken wrote:
> 
> Why is this needed?

Forgot to remove as part of some earlier experiment; now removed.

https://codereview.chromium.org/603193005/diff/100001/Source/core/rendering/R...
File Source/core/rendering/RenderScrollbar.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbar.cpp:77: // to call on a destroyed
scrollbar. See webkit bug 68009.
On 2014/09/29 14:16:36, haraken wrote:
> 
> The culprit of the complexity is this bug. How hard do you think to fix the
bug?

Someone with rendering knowledge is better qualified to answer that. Much better
:)

https://codereview.chromium.org/603193005/diff/100001/Source/platform/Widget.h
File Source/platform/Widget.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/platform/Widget....
Source/platform/Widget.h:132: virtual void detach() { }
On 2014/09/29 14:16:36, haraken wrote:
> 
> When is Widget::detach() called?
> 
> It's a bit confusing that the Widget hierarchy has both detach() and dispose()
> (PluginView has dispose()).

It's called as part of detaching HTMLPlugInElement, to detach plugin containers
only. 

But with the separate dispose() added for PluginView, which seems unavoidable,
detach() isn't providing anything over dispose(). So we can retire that
addition; nice! Now done.

https://codereview.chromium.org/603193005/diff/100001/Source/platform/exporte...
File Source/platform/exported/WebScrollbarImpl.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/platform/exporte...
Source/platform/exported/WebScrollbarImpl.h:35: class PLATFORM_EXPORT
WebScrollbarImpl FINAL : public WebScrollbar {
On 2014/09/29 14:16:36, haraken wrote:
> 
> FINAL => final

Done.

https://codereview.chromium.org/603193005/diff/100001/Source/web/PopupMenuChr...
File Source/web/PopupMenuChromium.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/web/PopupMenuChr...
Source/web/PopupMenuChromium.cpp:50: #if !ENABLE(OILPAN)
On 2014/09/29 14:16:37, haraken wrote:
> 
> Can we add an assert to oilpan builds to verify that dispose() has been
already
> called?

Done.

https://codereview.chromium.org/603193005/diff/100001/Source/web/PopupMenuChr...
Source/web/PopupMenuChromium.cpp:97: dispose();
On 2014/09/29 14:16:37, haraken wrote:
> 
> How about making non-oilpan builds call dispose() here as well?
> 
> (We don't want to have different behavior between oilpan builds and non-oilpan
> builds as much as possible.)

That's always preferable and I suspect that might work out in this case, but my
preference is to handle that separately to reduce risk. Added a FIXME.

https://codereview.chromium.org/603193005/diff/100001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.cpp:699: void
WebPluginContainerImpl::dispose()
On 2014/09/29 14:16:37, haraken wrote:
> 
> Shall we clear m_element in dispose() ?

Done.

[haraken, 2014-10-03 15:02:59]

Is the patch set 8 ready for review?

I added some comments to the patch set 8 but you are probably missing addressing
some of my comments in patch set 6?

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:219: if (m_view && !view)
On 2014/09/29 14:16:36, haraken wrote:
> 
> What is the '&& !view' check for?
> 
> How about calling m_view->dispose() in FrameView::prepareForDetach() ?

Would you reply to this question?

https://codereview.chromium.org/603193005/diff/140001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/603193005/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:157: #if !ENABLE(OILPAN)

Can we add a check to oilpan builds to verify that dispose() has been already
called?

https://codereview.chromium.org/603193005/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1528: RefPtrWillBeMember<FrameView>
protect(this);

RefPtrWillBeRawPtr

https://codereview.chromium.org/603193005/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:2661: void
FrameView::setLayoutSizeFixedToFrameSizeAndInvalidate(bool isFixed)

I'm sorry this helper method looks just untidy... Would you mind removing this
method?

https://codereview.chromium.org/603193005/diff/140001/Source/core/frame/Frame...
File Source/core/frame/FrameViewAutoSizeInfo.h (right):

https://codereview.chromium.org/603193005/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameViewAutoSizeInfo.h:18: class FrameViewAutoSizeInfo FINAL
: public NoBaseWillBeGarbageCollected<FrameViewAutoSizeInfo> {

FINAL => final

[sof, 2014-10-07 11:03:36]

haraken wrote:
> Is the patch set 8 ready for review?
> 
> I added some comments to the patch set 8 but you are probably missing
addressing some of my comments in patch set 6?

Many thanks for the feedback on #8. Two 'meta' issues at play here:

 - Need to carefully verify that any plugin change works out on any supported
platform.
 - Addressing all (and well) the non-trivial lifetime issues here in one
followup is just too much, so I need to chop it up a bit.

Another update now availablee, which I'll need to properly test before having
another go at finalization for plugins and scrollbars. No need to have a look
until the latter is also in place, of course.

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:155: #if !ENABLE(OILPAN)
On 2014/09/29 14:16:35, haraken wrote:
> 
> Can we add an assertion to oilpan builds to verify that dispose() has been
> already called before the destructor is called?

Done.

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:219: if (m_view && !view)
On 2014/09/29 14:16:36, haraken wrote:
> 
> What is the '&& !view' check for?
> 

I've now tweaked the predicate.

> How about calling m_view->dispose() in FrameView::prepareForDetach() ?

That would make the disposal happen _slightly_ earlier -- before the document is
informed of pending destruction, which detaches its RenderView -- which
conflicts with what dispose() assumes:

 ASSERT(m_frame->view() != this || !m_frame->contentRenderer());

i.e., we're disposing something that's not referred to from the LocalFrame _or_
that frame is itself detached & being destructed.

I don't think verifying that condition adds enough value for Oilpan (cf the
assert flag we have for disposal now), but I kept it non-Oilpan by moving it
into the dtor.

IOW.. :-) it makes some sense to have prepareForDetach() also take care of
disposing, hence now done. wdyt?

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLFr...
File Source/core/html/HTMLFrameOwnerElement.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:177: void
HTMLFrameOwnerElement::disposeWidget(Widget* widget)
On 2014/09/29 14:16:36, haraken wrote:
> 
> Can we avoid adding this static method by defining virtual Widget::dispose()
and
> overriding it in PluginView ?
> 
> Instead of calling HTMLFrameOwnerElement::disposeWidget(child), you can just
> call child->dispose().

ok, let's do that instead (as we've reduced our 'virtual' budget on Widget by
one here already.)

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:179: // Oilpan: a plugin container
must be disposed of prior to (GC)
On 2014/09/29 14:16:36, haraken wrote:
> 
> I don't fully understand this well, but what happens if we introduce:
> 
>   PersistentHeapHashMap<WeakMember<Widget>, OwnPtr<WidgetDisposer>>;
> 
> ?
> 
> Then can we avoid introducing Widget::dispose()?

This suggestion doesn't seem to address what the comment tries to explain must
hold when finalizing the plugin (container) -- that it needs to happen while the
frame (and all that it contains) is still alive & not being finalized. Because
it might perform an unbounded&unknown amount of work and operations on those
objects.

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLFr...
Source/core/html/HTMLFrameOwnerElement.cpp:183: if (widget &&
widget->isPluginView())
On 2014/09/29 14:16:36, haraken wrote:
> 
> Is it possible that this method is called with a null widget?

As it is currently used, yes.

https://codereview.chromium.org/603193005/diff/140001/Source/core/frame/Frame...
File Source/core/frame/FrameViewAutoSizeInfo.h (right):

https://codereview.chromium.org/603193005/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameViewAutoSizeInfo.h:18: class FrameViewAutoSizeInfo FINAL
: public NoBaseWillBeGarbageCollected<FrameViewAutoSizeInfo> {
On 2014/10/03 15:02:58, haraken wrote:
> 
> FINAL => final

Done.

[sof, 2014-10-07 13:27:32]

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:83:
HTMLFrameOwnerElement::disposeWidget(m_persistedPluginWidget.get());
On 2014/09/29 14:16:36, haraken wrote:
> 
> Can't we call HTMLFrameOwnerElement::disposeWidget when the HTMLPlugInElement
is
> removed from the DOM tree?
> 
> Ideally we want to call disposeWidget in HTMLPlugInElement::removedFrom() if
the
> Widget dies before the HTMLPlugInElement, and don't call anything if the
Widget
> and the HTMLPlugInElement die together.
> 
> Then we want to make m_persistedPluginWidget a Member<Widget>.
> 
> I wonder what prevents us from doing something like that.

That the plugin element isn't in the tree to start with :-) i.e., we can create
a plugin element outside the tree and load a renderer-less plugin.

See plugins/can-create-without-renderer.html for an example.

The disposal steps you refer to is set up to happen here in this CL if the
plugin element is attached to tree, I think (see
HTMLPlugInElement::disconnectContentFrame().)

[sof, 2014-10-07 15:30:37]

https://codereview.chromium.org/603193005/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderScrollbar.h (right):

https://codereview.chromium.org/603193005/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderScrollbar.h:96:
WillBePersistentHeapHashMap<unsigned, RawPtrWillBeMember<RenderScrollbarPart> >
m_parts;
On 2014/09/26 09:19:25, haraken wrote:
> 
> Can we use a disposer pattern to delay the finalization of the
> RenderScrollbarPart objects?
> 
>   PersistentHeapHashMap<WeakMember<RenderScrollbar>,
> OwnPtr<RenderScrollbarDisposer>>;
> 
> 
> You can destruct RenderScrollbarPart objects in the destructor of
> RenderScrollbarDisposer.

Used a pre-finalizer to handle this instead; no Persistent needed, I hope.

[sof, 2014-10-07 21:59:50]

On 2014/10/07 13:27:32, sof wrote:
>
https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
> File Source/core/html/HTMLPlugInElement.cpp (right):
> 
>
https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
> Source/core/html/HTMLPlugInElement.cpp:83:
> HTMLFrameOwnerElement::disposeWidget(m_persistedPluginWidget.get());
> On 2014/09/29 14:16:36, haraken wrote:
> > 
> > Can't we call HTMLFrameOwnerElement::disposeWidget when the
HTMLPlugInElement
> is
> > removed from the DOM tree?
> > 
> > Ideally we want to call disposeWidget in HTMLPlugInElement::removedFrom() if
> the
> > Widget dies before the HTMLPlugInElement, and don't call anything if the
> Widget
> > and the HTMLPlugInElement die together.
> > 
> > Then we want to make m_persistedPluginWidget a Member<Widget>.
> > 
> > I wonder what prevents us from doing something like that.
> 
> That the plugin element isn't in the tree to start with :-) i.e., we can
create
> a plugin element outside the tree and load a renderer-less plugin.
> 
> See plugins/can-create-without-renderer.html for an example.
> 
> The disposal steps you refer to is set up to happen here in this CL if the
> plugin element is attached to tree, I think (see
> HTMLPlugInElement::disconnectContentFrame().)

To avoid having to keep a Persistent on the plugin element like this, I've
shifted the responsibility of disposing these renderer-less & persisted plugins
to the LocalFrame. The frame weakly refers to these and will dispose of them
when it is detached.

Still waiting for mac&windows plugin test results; might be a bug or two flush
out, depending. But the overall scheme is sound, I think -- wdyt?

[haraken, 2014-10-08 00:28:22]

On 2014/10/07 21:59:50, sof wrote:
> On 2014/10/07 13:27:32, sof wrote:
> >
>
https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
> > File Source/core/html/HTMLPlugInElement.cpp (right):
> > 
> >
>
https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
> > Source/core/html/HTMLPlugInElement.cpp:83:
> > HTMLFrameOwnerElement::disposeWidget(m_persistedPluginWidget.get());
> > On 2014/09/29 14:16:36, haraken wrote:
> > > 
> > > Can't we call HTMLFrameOwnerElement::disposeWidget when the
> HTMLPlugInElement
> > is
> > > removed from the DOM tree?
> > > 
> > > Ideally we want to call disposeWidget in HTMLPlugInElement::removedFrom()
if
> > the
> > > Widget dies before the HTMLPlugInElement, and don't call anything if the
> > Widget
> > > and the HTMLPlugInElement die together.
> > > 
> > > Then we want to make m_persistedPluginWidget a Member<Widget>.
> > > 
> > > I wonder what prevents us from doing something like that.
> > 
> > That the plugin element isn't in the tree to start with :-) i.e., we can
> create
> > a plugin element outside the tree and load a renderer-less plugin.
> > 
> > See plugins/can-create-without-renderer.html for an example.
> > 
> > The disposal steps you refer to is set up to happen here in this CL if the
> > plugin element is attached to tree, I think (see
> > HTMLPlugInElement::disconnectContentFrame().)
> 
> To avoid having to keep a Persistent on the plugin element like this, I've
> shifted the responsibility of disposing these renderer-less & persisted
plugins
> to the LocalFrame. The frame weakly refers to these and will dispose of them
> when it is detached.
> 
> Still waiting for mac&windows plugin test results; might be a bug or two flush
> out, depending. But the overall scheme is sound, I think -- wdyt?

Sounds nice to me!

[haraken, 2014-10-08 00:28:46]

Ping me once this CL is ready for full review :)

[sof, 2014-10-08 06:00:10]

On 2014/10/08 00:28:22, haraken wrote:
> On 2014/10/07 21:59:50, sof wrote:
> > On 2014/10/07 13:27:32, sof wrote:
> > >
> >
>
https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
> > > File Source/core/html/HTMLPlugInElement.cpp (right):
> > > 
> > >
> >
>
https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
> > > Source/core/html/HTMLPlugInElement.cpp:83:
> > > HTMLFrameOwnerElement::disposeWidget(m_persistedPluginWidget.get());
> > > On 2014/09/29 14:16:36, haraken wrote:
> > > > 
> > > > Can't we call HTMLFrameOwnerElement::disposeWidget when the
> > HTMLPlugInElement
> > > is
> > > > removed from the DOM tree?
> > > > 
> > > > Ideally we want to call disposeWidget in
HTMLPlugInElement::removedFrom()
> if
> > > the
> > > > Widget dies before the HTMLPlugInElement, and don't call anything if the
> > > Widget
> > > > and the HTMLPlugInElement die together.
> > > > 
> > > > Then we want to make m_persistedPluginWidget a Member<Widget>.
> > > > 
> > > > I wonder what prevents us from doing something like that.
> > > 
> > > That the plugin element isn't in the tree to start with :-) i.e., we can
> > create
> > > a plugin element outside the tree and load a renderer-less plugin.
> > > 
> > > See plugins/can-create-without-renderer.html for an example.
> > > 
> > > The disposal steps you refer to is set up to happen here in this CL if the
> > > plugin element is attached to tree, I think (see
> > > HTMLPlugInElement::disconnectContentFrame().)
> > 
> > To avoid having to keep a Persistent on the plugin element like this, I've
> > shifted the responsibility of disposing these renderer-less & persisted
> plugins
> > to the LocalFrame. The frame weakly refers to these and will dispose of them
> > when it is detached.
> > 
> > Still waiting for mac&windows plugin test results; might be a bug or two
flush
> > out, depending. But the overall scheme is sound, I think -- wdyt?
> 
> Sounds nice to me!

Plugin tests came out clean.

I'll follow up on some accessibility/ issues and just generally go through the
CL once more before asking for another look.

[sof, 2014-10-08 11:51:59]

On 2014/10/08 06:00:10, sof wrote:
> On 2014/10/08 00:28:22, haraken wrote:
> > On 2014/10/07 21:59:50, sof wrote:
> > > On 2014/10/07 13:27:32, sof wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
> > > > File Source/core/html/HTMLPlugInElement.cpp (right):
> > > > 
> > > >
> > >
> >
>
https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
> > > > Source/core/html/HTMLPlugInElement.cpp:83:
> > > > HTMLFrameOwnerElement::disposeWidget(m_persistedPluginWidget.get());
> > > > On 2014/09/29 14:16:36, haraken wrote:
> > > > > 
> > > > > Can't we call HTMLFrameOwnerElement::disposeWidget when the
> > > HTMLPlugInElement
> > > > is
> > > > > removed from the DOM tree?
> > > > > 
> > > > > Ideally we want to call disposeWidget in
> HTMLPlugInElement::removedFrom()
> > if
> > > > the
> > > > > Widget dies before the HTMLPlugInElement, and don't call anything if
the
> > > > Widget
> > > > > and the HTMLPlugInElement die together.
> > > > > 
> > > > > Then we want to make m_persistedPluginWidget a Member<Widget>.
> > > > > 
> > > > > I wonder what prevents us from doing something like that.
> > > > 
> > > > That the plugin element isn't in the tree to start with :-) i.e., we can
> > > create
> > > > a plugin element outside the tree and load a renderer-less plugin.
> > > > 
> > > > See plugins/can-create-without-renderer.html for an example.
> > > > 
> > > > The disposal steps you refer to is set up to happen here in this CL if
the
> > > > plugin element is attached to tree, I think (see
> > > > HTMLPlugInElement::disconnectContentFrame().)
> > > 
> > > To avoid having to keep a Persistent on the plugin element like this, I've
> > > shifted the responsibility of disposing these renderer-less & persisted
> > plugins
> > > to the LocalFrame. The frame weakly refers to these and will dispose of
them
> > > when it is detached.
> > > 
> > > Still waiting for mac&windows plugin test results; might be a bug or two
> flush
> > > out, depending. But the overall scheme is sound, I think -- wdyt?
> > 
> > Sounds nice to me!
> 
> Plugin tests came out clean.
> 
> I'll follow up on some accessibility/ issues and just generally go through the
> CL once more before asking for another look.

It is now ready. However, two troublesome issues remain:

 - when is it allowed/safe to call FrameView::dispose() if there are other
references to the frame view.
 - how to dispose/destruct a renderer-less widget of a plugin element that's not
in the DOM.

It makes some sense to get the CL ready bar those two (I suspect they'll end up
needing local changes only.) It's back to the drawing board for the last one
though.

[sof, 2014-10-08 11:52:25]

https://codereview.chromium.org/603193005/diff/100001/Source/core/accessibili...
File Source/core/accessibility/AXScrollView.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/accessibili...
Source/core/accessibility/AXScrollView.h:82: // could create a leak.
On 2014/09/29 14:16:35, haraken wrote:
> 
> Would you update this comment to clarify that using a raw pointer for
> m_scrollView is fine because m_scrollView is explicitly cleared in
> AXScrollView::detach() ?
> 

Thanks, clarified.

https://codereview.chromium.org/603193005/diff/100001/Source/core/accessibili...
File Source/core/accessibility/AXScrollbar.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/core/accessibili...
Source/core/accessibility/AXScrollbar.h:64: RefPtrWillBePersistent<Scrollbar>
m_scrollbar;
On 2014/09/29 14:16:35, haraken wrote:
> 
> Hmm, it seems that the fact that AXScrollbar can hold a RefPtrWillBePersistent
> to the corresponding Scrollbar implies that AXScrollView can hold a
> RefPtrWillBePersistent to the corresponding ScrollView. I wonder why
> AXScrollbar's RefPtrWillBePersistent is OK but AXScrollView's
> RefPtrWillBePersistent is not OK.

My reasoning was simply that Scrollbar is a leaf object, and FrameView isn't.
The risk of leak creation mirrors that, to some degree.

I suggest we keep it as a persistent while we wait for the AXObject hierarchy to
be moved to the heap.

https://codereview.chromium.org/603193005/diff/140001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/603193005/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:157: #if !ENABLE(OILPAN)
On 2014/10/03 15:02:58, haraken wrote:
> 
> Can we add a check to oilpan builds to verify that dispose() has been already
> called?

Done.

https://codereview.chromium.org/603193005/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:1528: RefPtrWillBeMember<FrameView>
protect(this);
On 2014/10/03 15:02:58, haraken wrote:
> 
> RefPtrWillBeRawPtr

Done.

https://codereview.chromium.org/603193005/diff/140001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:2661: void
FrameView::setLayoutSizeFixedToFrameSizeAndInvalidate(bool isFixed)
On 2014/10/03 15:02:58, haraken wrote:
> 
> I'm sorry this helper method looks just untidy... Would you mind removing this
> method?

It has been removed/reverted.

https://codereview.chromium.org/603193005/diff/360001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/603193005/diff/360001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:291: // FIXME: Oilpan: is this safe to dispose()
if there are FrameView protections on the stack?
This is one of the main issues left to resolve.

https://codereview.chromium.org/603193005/diff/360001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/603193005/diff/360001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:104: void
HTMLPlugInElement::setPersistedPluginWidget(Widget* widget)
This scheme is not complete and doesn't solve the problematic case of having a
non-tree plugin element with a renderer-less plugin, with the element then
becoming unreachable and eventually swept.

For that case, we need to inject an explicit dispose of the underlying plugin
before the plugin element (and frame) goes away. There's no support for that
here, nor do I see how to achieve it.

It does address the case where the LocalFrame and plugin element die at the same
time, but not before the LocalFrame is detached.

[sof, 2014-10-08 14:43:41]

https://codereview.chromium.org/603193005/diff/360001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/603193005/diff/360001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:291: // FIXME: Oilpan: is this safe to dispose()
if there are FrameView protections on the stack?
On 2014/10/08 11:52:25, sof wrote:
> This is one of the main issues left to resolve.

cf.
https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/LocalF...

[haraken, 2014-10-08 15:15:43]

(I'll take a through look tomorrow.)

https://codereview.chromium.org/603193005/diff/360001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/603193005/diff/360001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:291: // FIXME: Oilpan: is this safe to dispose()
if there are FrameView protections on the stack?
On 2014/10/08 14:43:41, sof wrote:
> On 2014/10/08 11:52:25, sof wrote:
> > This is one of the main issues left to resolve.
> 
> cf.
>
https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/LocalF...

Are there many protecting pointers? If there are only a handful of protecting
pointers, can we probably take a quick look at if it's OK to dispose the
FrameView while protecting the FrameView?

I haven't studied the code, but what this CL is doing basically makes sense to
me --- a FrameView is disposed (but still alive) when the FrameView is
explicitly detached, and the FrameView is collected after the last reference to
the FrameView is lost.

[sof, 2014-10-08 21:44:07]

On 2014/10/08 15:15:43, haraken wrote:
> (I'll take a through look tomorrow.)
> 
>
https://codereview.chromium.org/603193005/diff/360001/Source/core/frame/Frame...
> File Source/core/frame/FrameView.cpp (right):
> 
>
https://codereview.chromium.org/603193005/diff/360001/Source/core/frame/Frame...
> Source/core/frame/FrameView.cpp:291: // FIXME: Oilpan: is this safe to
dispose()
> if there are FrameView protections on the stack?
> On 2014/10/08 14:43:41, sof wrote:
> > On 2014/10/08 11:52:25, sof wrote:
> > > This is one of the main issues left to resolve.
> > 
> > cf.
> >
>
https://codereview.chromium.org/603193005/diff/40001/Source/core/frame/LocalF...
> 
> Are there many protecting pointers? If there are only a handful of protecting
> pointers, can we probably take a quick look at if it's OK to dispose the
> FrameView while protecting the FrameView?
> 
> I haven't studied the code, but what this CL is doing basically makes sense to
> me --- a FrameView is disposed (but still alive) when the FrameView is
> explicitly detached, and the FrameView is collected after the last reference
to
> the FrameView is lost.

34 FrameView protections in this CL; a number of them in EventHandler. But
protections over (Local)Frame shouldn't be ignored either, due to the cycle
between frames and their views.

[sof, 2014-10-08 22:02:41]

On 2014/10/08 11:51:59, sof wrote:
> On 2014/10/08 06:00:10, sof wrote:
> > On 2014/10/08 00:28:22, haraken wrote:
> > > On 2014/10/07 21:59:50, sof wrote:
> > > > On 2014/10/07 13:27:32, sof wrote:
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
> > > > > File Source/core/html/HTMLPlugInElement.cpp (right):
> > > > > 
> > > > >
> > > >
> > >
> >
>
https://codereview.chromium.org/603193005/diff/100001/Source/core/html/HTMLPl...
> > > > > Source/core/html/HTMLPlugInElement.cpp:83:
> > > > > HTMLFrameOwnerElement::disposeWidget(m_persistedPluginWidget.get());
> > > > > On 2014/09/29 14:16:36, haraken wrote:
> > > > > > 
> > > > > > Can't we call HTMLFrameOwnerElement::disposeWidget when the
> > > > HTMLPlugInElement
> > > > > is
> > > > > > removed from the DOM tree?
> > > > > > 
> > > > > > Ideally we want to call disposeWidget in
> > HTMLPlugInElement::removedFrom()
> > > if
> > > > > the
> > > > > > Widget dies before the HTMLPlugInElement, and don't call anything if
> the
> > > > > Widget
> > > > > > and the HTMLPlugInElement die together.
> > > > > > 
> > > > > > Then we want to make m_persistedPluginWidget a Member<Widget>.
> > > > > > 
> > > > > > I wonder what prevents us from doing something like that.
> > > > > 
> > > > > That the plugin element isn't in the tree to start with :-) i.e., we
can
> > > > create
> > > > > a plugin element outside the tree and load a renderer-less plugin.
> > > > > 
> > > > > See plugins/can-create-without-renderer.html for an example.
> > > > > 
> > > > > The disposal steps you refer to is set up to happen here in this CL if
> the
> > > > > plugin element is attached to tree, I think (see
> > > > > HTMLPlugInElement::disconnectContentFrame().)
> > > > 
> > > > To avoid having to keep a Persistent on the plugin element like this,
I've
> > > > shifted the responsibility of disposing these renderer-less & persisted
> > > plugins
> > > > to the LocalFrame. The frame weakly refers to these and will dispose of
> them
> > > > when it is detached.
> > > > 
> > > > Still waiting for mac&windows plugin test results; might be a bug or two
> > flush
> > > > out, depending. But the overall scheme is sound, I think -- wdyt?
> > > 
> > > Sounds nice to me!
> > 
> > Plugin tests came out clean.
> > 
> > I'll follow up on some accessibility/ issues and just generally go through
the
> > CL once more before asking for another look.
> 
> It is now ready. However, two troublesome issues remain:
> 
>  - when is it allowed/safe to call FrameView::dispose() if there are other
> references to the frame view.
>  - how to dispose/destruct a renderer-less widget of a plugin element that's
not
> in the DOM.
> 
> It makes some sense to get the CL ready bar those two (I suspect they'll end
up
> needing local changes only.) It's back to the drawing board for the last one
> though.

Re: second issue - I've now addressed this, as follows:

 - Unattached plugin elements holding a plugin will register themselves with
their LocalFrame (like before.)
 - LocalFrame will via a weak callback observe when a plugin element is no
longer alive.
 - The weak callback will then set a flag on the dying plugin element/container
that it is allowed to dispose the plugin.
 - When the plugin container finalizer runs, it will see the flag set and do the
disposal.
 - That previous step is safe as the LocalFrame is still alive and can be
accessed by the plugin when disposing.
 - The plugin element itself isn't safe to access and is cleared prior to
disposal.

That takes care of the finalization of plugins of unattached plugin elements. If
a frame is detached as usual, it will dispose of any registered plugin elements
that are alive by that point.

The use of a LocalFrame weak callback in this fashion is unusual, but safe.
Other solutions either require more infrastructure support (object
resurrection?) or some elaborate Persistent-based disposer object. Neither
appeals.

Workable?

[sof, 2014-10-09 07:35:34]

(Followups to some Scrollbar-related feedback that I accidentally overlooked.)

https://codereview.chromium.org/603193005/diff/100001/Source/platform/scroll/...
File Source/platform/scroll/Scrollbar.h (right):

https://codereview.chromium.org/603193005/diff/100001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.h:167: ScrollableArea* m_scrollableArea;
On 2014/09/29 14:16:36, haraken wrote:
> 
> Given that ScrollableArea is on the heap, can we make this a Member?

The objects that implement ScrollableArea are not all on the heap. In
particular, RenderLayerScrollableArea isn't.

The ScrollableAreas do control the Scrollbars created, and keep them referenced,
so they'll either die together, or in the case of RenderLayerScrollableArea, a
Scrollbar is explicitly disconnected before the Persistent reference to it is
dropped. Hence, the raw pointer is safe.

Moving RenderLayerScrollableArea to the heap will require some care (cf. its
finalizer), which is the only major obstacle to making ScrollableArea a GC
mixin. That can perhaps be tackled at some later point.

https://codereview.chromium.org/603193005/diff/100001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.h:170: ScrollbarTheme* m_theme;
On 2014/09/29 14:16:36, haraken wrote:
> 
> Are you sure that this raw pointer doesn't become a stale pointer?

It is always a reference to a statically allocated ScrollbarTheme
(RenderScrollbarTheme::renderScrollbarTheme(), ScrollbarTheme::theme(), or
ScrollbarTheme::nativeTheme()), so that should be fine.

We explicitly unregister when finalizing, so stale pointers in the other cannot
arise either. (Not clear that they're ever kept, but still.)

[haraken, 2014-10-10 02:50:32]

Sorry about the review delay. (I can review one complicated patch per day :-)

The approach looks mostly good to me.

https://codereview.chromium.org/603193005/diff/380001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/380001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:742: // renderer-less the opportunity to
dispose of their plugins.

renderer-less => renderer-less plugins

https://codereview.chromium.org/603193005/diff/380001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:777: void
LocalFrame::unregisterPluginElement(HTMLPlugInElement* plugin)

Shall we add:

ASSERT(!ThreadState::isSweepInProgress());

?

https://codereview.chromium.org/603193005/diff/380001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/603193005/diff/380001/Source/core/frame/Local...
Source/core/frame/LocalFrame.h:201: HeapHashSet<WeakMember<HTMLPlugInElement> >
m_pluginElements;

This idea looks nice!

However, I want to understand one thing: Won't it be enough to register a
pre-finalizer to a Widget so that Widget::dispose() is called in the
pre-finalizer?

https://codereview.chromium.org/603193005/diff/380001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/603193005/diff/380001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:103: void
HTMLPlugInElement::setIsAllowedToDisposePlugin()

setIsAllowedToDisposePlugin => shouldDisposePlugin ?

https://codereview.chromium.org/603193005/diff/380001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:118: if (LocalFrame* frame =
oldPlugin->pluginFrame()) {

Help me understand: In a case where oldPlugin->pluginFrame() returns 0, who
called dispose() for the plugin? I'm curious how it's guaranteed that dispose()
is always called for all plugins.

https://codereview.chromium.org/603193005/diff/380001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:125: if (LocalFrame* frame =
plugin->pluginFrame()) {

Help me understand: Who calls dispose() for a plugin if plugin->pluginFrame()
returns 0 (and thus we fail to register the plugin to the frame) ?

https://codereview.chromium.org/603193005/diff/380001/Source/core/rendering/R...
File Source/core/rendering/RenderScrollbar.cpp (right):

https://codereview.chromium.org/603193005/diff/380001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbar.cpp:285: if (updatePartsMap)

Why do you need this condition? Given that m_parts is a HeapHashMap, rehashing
of the hash map is disabled while pre-finalizers are running. Thus I guess we
won't need the m_updatePartsMap flag.

https://codereview.chromium.org/603193005/diff/380001/Source/platform/scroll/...
File Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/603193005/diff/380001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.cpp:86: m_animator =
m_scrollableArea->scrollAnimator();

Sorry, I got confused with this again.

The scrollableArea is off-heap. So it should be OK to touch
m_scrollableArea->m_animator in ~Scrollbar(). Then we won't need m_animator,
will we?

https://codereview.chromium.org/603193005/diff/380001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/603193005/diff/380001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.cpp:729: // the plugin element itself.

Help me understand:

1) Why can't you simply call WebPluginContainerImpl::dispose() in the weak
callback of LocalFrame::m_pluginElements ?

2) If the WebPluginContainerImpl and the LocalFrame die together,
WebPluginContainerImpl::dispose() won't be called. Is this OK?

3) What happens if we register a pre-finalizer of WebPluginContainerImpl and
call WebPluginContainerImpl::dispose() in the pre-finalizer?

https://codereview.chromium.org/603193005/diff/380001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/603193005/diff/380001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.h:217: bool m_isAllowedToDisposePlugin;

m_isAllowedToDisposePlugin => m_shouldDisposePlugin ?

[sof, 2014-10-10 05:22:19]

Thanks very much for the probing questions; followups to those below. Will
separately address the suggested code changes.

https://codereview.chromium.org/603193005/diff/380001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.h (right):

https://codereview.chromium.org/603193005/diff/380001/Source/core/frame/Local...
Source/core/frame/LocalFrame.h:201: HeapHashSet<WeakMember<HTMLPlugInElement> >
m_pluginElements;
On 2014/10/10 02:50:32, haraken wrote:
> 
> This idea looks nice!
> 
> However, I want to understand one thing: Won't it be enough to register a
> pre-finalizer to a Widget so that Widget::dispose() is called in the
> pre-finalizer?

dispose() of the plugin might trigger unbounded amounts of allocation, so
pre-finalizers aren't suitable.

https://codereview.chromium.org/603193005/diff/380001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/603193005/diff/380001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:118: if (LocalFrame* frame =
oldPlugin->pluginFrame()) {
On 2014/10/10 02:50:32, haraken wrote:
> 
> Help me understand: In a case where oldPlugin->pluginFrame() returns 0, who
> called dispose() for the plugin? I'm curious how it's guaranteed that
dispose()
> is always called for all plugins.

The condition cannot happen (see below.)

https://codereview.chromium.org/603193005/diff/380001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:125: if (LocalFrame* frame =
plugin->pluginFrame()) {
On 2014/10/10 02:50:32, haraken wrote:
> 
> Help me understand: Who calls dispose() for a plugin if plugin->pluginFrame()
> returns 0 (and thus we fail to register the plugin to the frame) ?

I don't think any such plugins exist. i.e., WebPluginContainerImpl is the only
implementation of PluginView and it always has a Frame (at least when in this
method.)

https://codereview.chromium.org/603193005/diff/380001/Source/core/rendering/R...
File Source/core/rendering/RenderScrollbar.cpp (right):

https://codereview.chromium.org/603193005/diff/380001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbar.cpp:285: if (updatePartsMap)
On 2014/10/10 02:50:32, haraken wrote:
> 
> Why do you need this condition? Given that m_parts is a HeapHashMap, rehashing
> of the hash map is disabled while pre-finalizers are running. Thus I guess we
> won't need the m_updatePartsMap flag.

ok, I simply prefer not to rely on there being no work/allocation done.

https://codereview.chromium.org/603193005/diff/380001/Source/platform/scroll/...
File Source/platform/scroll/Scrollbar.cpp (right):

https://codereview.chromium.org/603193005/diff/380001/Source/platform/scroll/...
Source/platform/scroll/Scrollbar.cpp:86: m_animator =
m_scrollableArea->scrollAnimator();
On 2014/10/10 02:50:32, haraken wrote:
> 
> Sorry, I got confused with this again.
> 
> The scrollableArea is off-heap. So it should be OK to touch
> m_scrollableArea->m_animator in ~Scrollbar(). Then we won't need m_animator,
> will we?

It _might_ be on the heap, we don't know (i.e., a FrameView is.)

https://codereview.chromium.org/603193005/diff/380001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.cpp (right):

https://codereview.chromium.org/603193005/diff/380001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.cpp:729: // the plugin element itself.
On 2014/10/10 02:50:32, haraken wrote:
> 
> Help me understand:
> 
> 1) Why can't you simply call WebPluginContainerImpl::dispose() in the weak
> callback of LocalFrame::m_pluginElements ?
> 

Unbounded amounts of work and allocation might result, so this wouldn't be
suitable.

> 2) If the WebPluginContainerImpl and the LocalFrame die together,
> WebPluginContainerImpl::dispose() won't be called. Is this OK?
> 

If the LocalFrame hasn't been detached before that time, the plugins won't be.
That's as good as it gets, I think.

> 3) What happens if we register a pre-finalizer of WebPluginContainerImpl and
> call WebPluginContainerImpl::dispose() in the pre-finalizer?

Answered elsewhere; not suitable.

[sof, 2014-10-10 08:40:55]

Thanks again; this completes my followups.

https://codereview.chromium.org/603193005/diff/380001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/380001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:742: // renderer-less the opportunity to
dispose of their plugins.
On 2014/10/10 02:50:32, haraken wrote:
> 
> renderer-less => renderer-less plugins

Done.

https://codereview.chromium.org/603193005/diff/380001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:777: void
LocalFrame::unregisterPluginElement(HTMLPlugInElement* plugin)
On 2014/10/10 02:50:32, haraken wrote:
> 
> Shall we add:
> 
> ASSERT(!ThreadState::isSweepInProgress());
> 
> ?

That should hold, done (but a bit surprised why you thought of the need for it
here.)

https://codereview.chromium.org/603193005/diff/380001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/603193005/diff/380001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:103: void
HTMLPlugInElement::setIsAllowedToDisposePlugin()
On 2014/10/10 02:50:32, haraken wrote:
> 
> setIsAllowedToDisposePlugin => shouldDisposePlugin ?

That's a better verb to use, thanks. disposal isn't optional.

https://codereview.chromium.org/603193005/diff/380001/Source/core/rendering/R...
File Source/core/rendering/RenderScrollbar.cpp (right):

https://codereview.chromium.org/603193005/diff/380001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbar.cpp:285: if (updatePartsMap)
On 2014/10/10 05:22:19, sof wrote:
> On 2014/10/10 02:50:32, haraken wrote:
> > 
> > Why do you need this condition? Given that m_parts is a HeapHashMap,
rehashing
> > of the hash map is disabled while pre-finalizers are running. Thus I guess
we
> > won't need the m_updatePartsMap flag.
> 
> ok, I simply prefer not to rely on there being no work/allocation done.

To drive things forward, dropped the flag & rely on HeapHashMap.

https://codereview.chromium.org/603193005/diff/380001/Source/web/WebPluginCon...
File Source/web/WebPluginContainerImpl.h (right):

https://codereview.chromium.org/603193005/diff/380001/Source/web/WebPluginCon...
Source/web/WebPluginContainerImpl.h:217: bool m_isAllowedToDisposePlugin;
On 2014/10/10 02:50:32, haraken wrote:
> 
> m_isAllowedToDisposePlugin => m_shouldDisposePlugin ?

Done.

[sof, 2014-10-10 12:33:36]

https://codereview.chromium.org/641733004/ will conflict with this, clearly.

But verified to be just simple code motion conflicts; which one will land first?

[sof, 2014-10-11 07:25:09]

On 2014/10/10 12:33:36, sof wrote:
> https://codereview.chromium.org/641733004/ will conflict with this, clearly.
> 
> But verified to be just simple code motion conflicts; which one will land
first?

With that landed as r183555, rebased upto r183571 (patchset 23).

=> ScrollView is no more, merged into FrameView. It shouldn't upset the reading
of this CL, I think/hope.

[haraken, 2014-10-11 17:33:03]

LGTM!

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:2421: const
WillBeHeapHashSet<RefPtrWillBeMember<Widget> >* viewChildren = children();

Can we use ChildrenWidgetSet ?

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:158: visitor->trace(m_pluginElements);

Is it safe to trace m_pluginElements? There is no guarantee about the order of
default weak processing and custom weak processing (i.e., clearWeakMembers).

For clarity, I'd prefer not tracing m_pluginElements and handing everything in
clearWeakMembers.

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:170: (*it)->shouldDisposePlugin();

shouldDisposePlugin() only does simple things and thus can be called in a
pre-finalizer. Can we probably call WebPluginContainerImpl::dispose() in
HTMLPluginElement's pre-finalizer? Then we can remove the weak hash map from
LocalFrame.

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:779:
ASSERT(!ThreadState::current()->isSweepInProgress());

I wanted to add this check since it would cause an issue if
m_pluginElements.remove runs during the sweeping phase. However, there will be
no issue, so the check is meaningless. Sorry, let's remove the check.

https://codereview.chromium.org/603193005/diff/750001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:116: if (LocalFrame* frame =
toPluginView(m_persistedPluginWidget.get())->pluginFrame()) {

Can we add ASSERT(frame) instead of having an if branch?

https://codereview.chromium.org/603193005/diff/750001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:122: if (LocalFrame* frame =
toPluginView(widget)->pluginFrame()) {

Ditto.

https://codereview.chromium.org/603193005/diff/750001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:542: } else if (widget !=
m_persistedPluginWidget) {

The 'widget != m_persistedPluginWidget' check won't be needed since it's checked
in setPersistedPluginWidget.

https://codereview.chromium.org/603193005/diff/750001/Source/core/page/scroll...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/page/scroll...
Source/core/page/scrolling/ScrollingCoordinator.cpp:724: if (const
WillBeHeapHashSet<RefPtrWillBeMember<Widget> >* children =
frameView->children()) {

ChildrenWidgetSet

https://codereview.chromium.org/603193005/diff/750001/Source/core/plugins/Plu...
File Source/core/plugins/PluginOcclusionSupport.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/plugins/Plu...
Source/core/plugins/PluginOcclusionSupport.cpp:167: const
WillBeHeapHashSet<RefPtrWillBeMember<Widget> >* children =
parentFrameView->children();

ChildrenWidgetSet

https://codereview.chromium.org/603193005/diff/750001/Source/core/rendering/R...
File Source/core/rendering/RenderScrollbar.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbar.cpp:93: //

Unnecessary comment.

https://codereview.chromium.org/603193005/diff/750001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbar.cpp:94: updateScrollbarParts(true);

Are you sure that updateScrollbarParts() doesn't do anything that changes an
object graph (because updateScrollbarParts() is called during a pre-finalizer)?

https://codereview.chromium.org/603193005/diff/750001/Source/platform/Widget.h
File Source/platform/Widget.h (right):

https://codereview.chromium.org/603193005/diff/750001/Source/platform/Widget....
Source/platform/Widget.h:132: virtual void dispose() { }

Can we make this a pure virtual method? (Otherwise, we should call
Widget::dispose() in derived dispose() methods.)

https://codereview.chromium.org/603193005/diff/750001/Source/web/WebHelperPlu...
File Source/web/WebHelperPluginImpl.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/web/WebHelperPlu...
Source/web/WebHelperPluginImpl.cpp:58:
WebHelperPluginImpl::~WebHelperPluginImpl()

Just to confirm: Isn't it possible that ~WebHelperPluginImpl() is called during
an Oilpan's GC? (WebHelperPluginImpl is off-heap, but it might be possible that
the last reference to the WebHelperPluginImpl is lost when finalizing some
on-heap object.)

[sof, 2014-10-12 08:16:23]

Thanks! Getting closer..

Need clarification on LocalFrame's weak handling and how we prefer to do that.
And a detail on the safety of WebHelperPluginImpl.

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Frame...
File Source/core/frame/FrameView.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Frame...
Source/core/frame/FrameView.cpp:2421: const
WillBeHeapHashSet<RefPtrWillBeMember<Widget> >* viewChildren = children();
On 2014/10/11 17:33:02, haraken wrote:
> 
> Can we use ChildrenWidgetSet ?

Certainly can, thanks for catching.

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:158: visitor->trace(m_pluginElements);
On 2014/10/11 17:33:02, haraken wrote:
> 
> Is it safe to trace m_pluginElements? There is no guarantee about the order of
> default weak processing and custom weak processing (i.e., clearWeakMembers).
> 

It is safe with the weak callback stack that the implementation keeps. i.e.,
clearWeakMembers() will be invoked before the callback for the weak hash set
impl.

> For clarity, I'd prefer not tracing m_pluginElements and handing everything in
> clearWeakMembers.

Not clear what you're effectively suggesting. I see three possible
interpretations:

 - Stop using Oilpan types (HashSet<HTMLPlugInElement*>) and process that in
   clearWeakMembers() using isAlive() and then later removing the dead entries.

   (A pity to not be able to use Oilpan types to express Oilpan weak reference
    handling! :) )

 - Iterate over a HeapHashSet<WeakMember<HTMLPlugInElement> > checking for
liveness.
   Then mark & register a weak callback for the hash set to be processed in a
later
   callback. Are those operations allowed from within a(nother) weak callback?

 - Somehow emulate what the trace() over weak hash set currently implements,
iterating
   over the elements checking for liveness (and calling shouldDisposePlugin()),
but
   then removing the dead entries. Removing such doesn't appear to be supported
without
   running the risk of a rehash. And how would you mark-but-not-trace the
backing store
   itself. i.e., the functionality that the hash table trace() uses doesn't
appear to
   be supported in "user defined" weak callbacks -- please tell me if I'm
missing some
   details that really do allow this to be expressed.

If the assumption made with what's currently implemented is thought
inappropriate, I can
align with any of the above three -- which one is preferable in that case?

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:170: (*it)->shouldDisposePlugin();
On 2014/10/11 17:33:02, haraken wrote:
> 
> shouldDisposePlugin() only does simple things and thus can be called in a
> pre-finalizer. Can we probably call WebPluginContainerImpl::dispose() in
> HTMLPluginElement's pre-finalizer? Then we can remove the weak hash map from
> LocalFrame.
> 

How would you know if the LocalFrame is still alive to allow that to go ahead?

I don't think this suggestion will work -- we trigger it from here when we
_know_ that the Frame has survived (but the plugin element hasn't.)

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:779:
ASSERT(!ThreadState::current()->isSweepInProgress());
On 2014/10/11 17:33:02, haraken wrote:
> 
> I wanted to add this check since it would cause an issue if
> m_pluginElements.remove runs during the sweeping phase. However, there will be
> no issue, so the check is meaningless. Sorry, let's remove the check.

Gone :)

https://codereview.chromium.org/603193005/diff/750001/Source/core/html/HTMLPl...
File Source/core/html/HTMLPlugInElement.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:116: if (LocalFrame* frame =
toPluginView(m_persistedPluginWidget.get())->pluginFrame()) {
On 2014/10/11 17:33:02, haraken wrote:
> 
> Can we add ASSERT(frame) instead of having an if branch?

Done.

https://codereview.chromium.org/603193005/diff/750001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:122: if (LocalFrame* frame =
toPluginView(widget)->pluginFrame()) {
On 2014/10/11 17:33:02, haraken wrote:
> 
> Ditto.

Done.

https://codereview.chromium.org/603193005/diff/750001/Source/core/html/HTMLPl...
Source/core/html/HTMLPlugInElement.cpp:542: } else if (widget !=
m_persistedPluginWidget) {
On 2014/10/11 17:33:02, haraken wrote:
> 
> The 'widget != m_persistedPluginWidget' check won't be needed since it's
checked
> in setPersistedPluginWidget.

Not quite, but I've now made setPersistendPluginWidget() do so.

https://codereview.chromium.org/603193005/diff/750001/Source/core/page/scroll...
File Source/core/page/scrolling/ScrollingCoordinator.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/page/scroll...
Source/core/page/scrolling/ScrollingCoordinator.cpp:724: if (const
WillBeHeapHashSet<RefPtrWillBeMember<Widget> >* children =
frameView->children()) {
On 2014/10/11 17:33:02, haraken wrote:
> 
> ChildrenWidgetSet

Done.

https://codereview.chromium.org/603193005/diff/750001/Source/core/plugins/Plu...
File Source/core/plugins/PluginOcclusionSupport.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/plugins/Plu...
Source/core/plugins/PluginOcclusionSupport.cpp:167: const
WillBeHeapHashSet<RefPtrWillBeMember<Widget> >* children =
parentFrameView->children();
On 2014/10/11 17:33:03, haraken wrote:
> 
> ChildrenWidgetSet

Done.

https://codereview.chromium.org/603193005/diff/750001/Source/core/rendering/R...
File Source/core/rendering/RenderScrollbar.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbar.cpp:93: //
On 2014/10/11 17:33:03, haraken wrote:
> 
> Unnecessary comment.

Removed the empty comment line.

https://codereview.chromium.org/603193005/diff/750001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbar.cpp:94: updateScrollbarParts(true);
On 2014/10/11 17:33:03, haraken wrote:
> 
> Are you sure that updateScrollbarParts() doesn't do anything that changes an
> object graph (because updateScrollbarParts() is called during a
pre-finalizer)?

Updating the parts heap hash map is handled fine, I'm (now) assuming.

For the destroy() call that this triggers on one or more RenderScrollbarPart
objects, I can only see values being cleared.

https://codereview.chromium.org/603193005/diff/750001/Source/platform/Widget.h
File Source/platform/Widget.h (right):

https://codereview.chromium.org/603193005/diff/750001/Source/platform/Widget....
Source/platform/Widget.h:132: virtual void dispose() { }
On 2014/10/11 17:33:03, haraken wrote:
> 
> Can we make this a pure virtual method? (Otherwise, we should call
> Widget::dispose() in derived dispose() methods.)

Prefer to keep it like this, providing a default implementation.

I don't get why delegation has to be part of the meta protocol for dispose() (it
isn't for other Widget methods, e.g., eventListenersRemoved() just above.)

https://codereview.chromium.org/603193005/diff/750001/Source/web/WebHelperPlu...
File Source/web/WebHelperPluginImpl.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/web/WebHelperPlu...
Source/web/WebHelperPluginImpl.cpp:58:
WebHelperPluginImpl::~WebHelperPluginImpl()
On 2014/10/11 17:33:03, haraken wrote:
> 
> Just to confirm: Isn't it possible that ~WebHelperPluginImpl() is called
during
> an Oilpan's GC? (WebHelperPluginImpl is off-heap, but it might be possible
that
> the last reference to the WebHelperPluginImpl is lost when finalizing some
> on-heap object.)

WebHelperPlugin is a wrapping layer that the embedder holds onto and explicitly
deletes. I think we have to assume that what you describe can somehow happen, in
the general case. (I can't make out how via the EME player code that uses it
atm.)

So, if we end up in ~WebHelperPluginImpl and call dispose() on the Persistent<>
plugin container reference _and_ the LocalFrame of the plugin container will
have been finalized already, then things would break.

Is that really possible, i.e., that the two Persistent<>s to the plugin
container and the object/plugin element won't keep the LocalFrame alive until
they're released (by ~WebHelperPluginImpl)? And would only be sweepable during
the next GC. 

I'm assuming not; if you think that's not sound somehow, we could shore it up
adding a Persistent<> over the LocalFrame they're created in.

[haraken, 2014-10-12 12:56:05]

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:158: visitor->trace(m_pluginElements);
On 2014/10/12 08:16:22, sof wrote:
> On 2014/10/11 17:33:02, haraken wrote:
> > 
> > Is it safe to trace m_pluginElements? There is no guarantee about the order
of
> > default weak processing and custom weak processing (i.e., clearWeakMembers).
> > 
> 
> It is safe with the weak callback stack that the implementation keeps. i.e.,
> clearWeakMembers() will be invoked before the callback for the weak hash set
> impl.
> 
> > For clarity, I'd prefer not tracing m_pluginElements and handing everything
in
> > clearWeakMembers.
> 
> Not clear what you're effectively suggesting. I see three possible
> interpretations:
> 
>  - Stop using Oilpan types (HashSet<HTMLPlugInElement*>) and process that in
>    clearWeakMembers() using isAlive() and then later removing the dead
entries.
> 
>    (A pity to not be able to use Oilpan types to express Oilpan weak reference
>     handling! :) )
> 
>  - Iterate over a HeapHashSet<WeakMember<HTMLPlugInElement> > checking for
> liveness.
>    Then mark & register a weak callback for the hash set to be processed in a
> later
>    callback. Are those operations allowed from within a(nother) weak callback?
> 
>  - Somehow emulate what the trace() over weak hash set currently implements,
> iterating
>    over the elements checking for liveness (and calling
shouldDisposePlugin()),
> but
>    then removing the dead entries. Removing such doesn't appear to be
supported
> without
>    running the risk of a rehash. And how would you mark-but-not-trace the
> backing store
>    itself. i.e., the functionality that the hash table trace() uses doesn't
> appear to
>    be supported in "user defined" weak callbacks -- please tell me if I'm
> missing some
>    details that really do allow this to be expressed.
> 
> If the assumption made with what's currently implemented is thought
> inappropriate, I can
> align with any of the above three -- which one is preferable in that case?

I'm suggesting the first one :)

If I'm understanding code correctly, weak processing for built-in hash maps is
executed during a GC phase (while all other threads are paused). Weak processing
for clearWeakMembers is executed in thread-specific weak processing, which
happens after the GC phase.

Thus, I guess isAlive() in clearWeakMembers always returns true.

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:170: (*it)->shouldDisposePlugin();
On 2014/10/12 08:16:22, sof wrote:
> On 2014/10/11 17:33:02, haraken wrote:
> > 
> > shouldDisposePlugin() only does simple things and thus can be called in a
> > pre-finalizer. Can we probably call WebPluginContainerImpl::dispose() in
> > HTMLPluginElement's pre-finalizer? Then we can remove the weak hash map from
> > LocalFrame.
> > 
> 
> How would you know if the LocalFrame is still alive to allow that to go ahead?
> 
> I don't think this suggestion will work -- we trigger it from here when we
> _know_ that the Frame has survived (but the plugin element hasn't.)

I'm a bit confused. Pre-finalizers are invoked before any object in the thread
heap is swept. So I guess it's guaranteed that the Frame is still alive in the
pre-finalizers.

https://codereview.chromium.org/603193005/diff/750001/Source/core/rendering/R...
File Source/core/rendering/RenderScrollbar.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/rendering/R...
Source/core/rendering/RenderScrollbar.cpp:94: updateScrollbarParts(true);
On 2014/10/12 08:16:22, sof wrote:
> On 2014/10/11 17:33:03, haraken wrote:
> > 
> > Are you sure that updateScrollbarParts() doesn't do anything that changes an
> > object graph (because updateScrollbarParts() is called during a
> pre-finalizer)?
> 
> Updating the parts heap hash map is handled fine, I'm (now) assuming.
> 
> For the destroy() call that this triggers on one or more RenderScrollbarPart
> objects, I can only see values being cleared.

OK, thanks for the verification. I'm worry about a complicated pre-finalizer
because there is no way to verify that the pre-finalizer isn't doing something
forbidden :)

https://codereview.chromium.org/603193005/diff/750001/Source/web/WebHelperPlu...
File Source/web/WebHelperPluginImpl.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/web/WebHelperPlu...
Source/web/WebHelperPluginImpl.cpp:58:
WebHelperPluginImpl::~WebHelperPluginImpl()
On 2014/10/12 08:16:22, sof wrote:
> On 2014/10/11 17:33:03, haraken wrote:
> > 
> > Just to confirm: Isn't it possible that ~WebHelperPluginImpl() is called
> during
> > an Oilpan's GC? (WebHelperPluginImpl is off-heap, but it might be possible
> that
> > the last reference to the WebHelperPluginImpl is lost when finalizing some
> > on-heap object.)
> 
> WebHelperPlugin is a wrapping layer that the embedder holds onto and
explicitly
> deletes. I think we have to assume that what you describe can somehow happen,
in
> the general case. (I can't make out how via the EME player code that uses it
> atm.)
> 
> So, if we end up in ~WebHelperPluginImpl and call dispose() on the
Persistent<>
> plugin container reference _and_ the LocalFrame of the plugin container will
> have been finalized already, then things would break.
> 
> Is that really possible, i.e., that the two Persistent<>s to the plugin
> container and the object/plugin element won't keep the LocalFrame alive until
> they're released (by ~WebHelperPluginImpl)? And would only be sweepable during
> the next GC. 
> 
> I'm assuming not; if you think that's not sound somehow, we could shore it up
> adding a Persistent<> over the LocalFrame they're created in.

Thanks, understood.

Can we add an ASSERT to verify that something unexpected doesn't actually
happen? Probably we can add ASSERT(!ThreadState::SweepInProgress()).

[sof, 2014-10-12 13:09:13]

Thanks for the clarifications; will follow up and go with those.

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:158: visitor->trace(m_pluginElements);
On 2014/10/12 12:56:04, haraken wrote:
> On 2014/10/12 08:16:22, sof wrote:
> > On 2014/10/11 17:33:02, haraken wrote:
> > > 
> > > Is it safe to trace m_pluginElements? There is no guarantee about the
order
> of
> > > default weak processing and custom weak processing (i.e.,
clearWeakMembers).
> > > 
> > 
> > It is safe with the weak callback stack that the implementation keeps. i.e.,
> > clearWeakMembers() will be invoked before the callback for the weak hash set
> > impl.
> > 
> > > For clarity, I'd prefer not tracing m_pluginElements and handing
everything
> in
> > > clearWeakMembers.
> > 
> > Not clear what you're effectively suggesting. I see three possible
> > interpretations:
> > 
> >  - Stop using Oilpan types (HashSet<HTMLPlugInElement*>) and process that in
> >    clearWeakMembers() using isAlive() and then later removing the dead
> entries.
> > 
> >    (A pity to not be able to use Oilpan types to express Oilpan weak
reference
> >     handling! :) )
> > 
> >  - Iterate over a HeapHashSet<WeakMember<HTMLPlugInElement> > checking for
> > liveness.
> >    Then mark & register a weak callback for the hash set to be processed in
a
> > later
> >    callback. Are those operations allowed from within a(nother) weak
callback?
> > 
> >  - Somehow emulate what the trace() over weak hash set currently implements,
> > iterating
> >    over the elements checking for liveness (and calling
> shouldDisposePlugin()),
> > but
> >    then removing the dead entries. Removing such doesn't appear to be
> supported
> > without
> >    running the risk of a rehash. And how would you mark-but-not-trace the
> > backing store
> >    itself. i.e., the functionality that the hash table trace() uses doesn't
> > appear to
> >    be supported in "user defined" weak callbacks -- please tell me if I'm
> > missing some
> >    details that really do allow this to be expressed.
> > 
> > If the assumption made with what's currently implemented is thought
> > inappropriate, I can
> > align with any of the above three -- which one is preferable in that case?
> 
> I'm suggesting the first one :)
> 
> If I'm understanding code correctly, weak processing for built-in hash maps is
> executed during a GC phase (while all other threads are paused). Weak
processing
> for clearWeakMembers is executed in thread-specific weak processing, which
> happens after the GC phase.
> 
> Thus, I guess isAlive() in clearWeakMembers always returns true.

It does not always return true; if no one has marked it by that time, it will
return false.

Which you'll see happening on plugins/can-create-without-renderer.html

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:170: (*it)->shouldDisposePlugin();
On 2014/10/12 12:56:04, haraken wrote:
> On 2014/10/12 08:16:22, sof wrote:
> > On 2014/10/11 17:33:02, haraken wrote:
> > > 
> > > shouldDisposePlugin() only does simple things and thus can be called in a
> > > pre-finalizer. Can we probably call WebPluginContainerImpl::dispose() in
> > > HTMLPluginElement's pre-finalizer? Then we can remove the weak hash map
from
> > > LocalFrame.
> > > 
> > 
> > How would you know if the LocalFrame is still alive to allow that to go
ahead?
> > 
> > I don't think this suggestion will work -- we trigger it from here when we
> > _know_ that the Frame has survived (but the plugin element hasn't.)
> 
> I'm a bit confused. Pre-finalizers are invoked before any object in the thread
> heap is swept. So I guess it's guaranteed that the Frame is still alive in the
> pre-finalizers.

It's still alive, yes. But the plugin is allowed to execute scripts & the like
(== allocations.)

[haraken, 2014-10-12 13:38:23]

Thanks for being persistent!

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:158: visitor->trace(m_pluginElements);
On 2014/10/12 13:09:12, sof wrote:
> On 2014/10/12 12:56:04, haraken wrote:
> > On 2014/10/12 08:16:22, sof wrote:
> > > On 2014/10/11 17:33:02, haraken wrote:
> > > > 
> > > > Is it safe to trace m_pluginElements? There is no guarantee about the
> order
> > of
> > > > default weak processing and custom weak processing (i.e.,
> clearWeakMembers).
> > > > 
> > > 
> > > It is safe with the weak callback stack that the implementation keeps.
i.e.,
> > > clearWeakMembers() will be invoked before the callback for the weak hash
set
> > > impl.
> > > 
> > > > For clarity, I'd prefer not tracing m_pluginElements and handing
> everything
> > in
> > > > clearWeakMembers.
> > > 
> > > Not clear what you're effectively suggesting. I see three possible
> > > interpretations:
> > > 
> > >  - Stop using Oilpan types (HashSet<HTMLPlugInElement*>) and process that
in
> > >    clearWeakMembers() using isAlive() and then later removing the dead
> > entries.
> > > 
> > >    (A pity to not be able to use Oilpan types to express Oilpan weak
> reference
> > >     handling! :) )
> > > 
> > >  - Iterate over a HeapHashSet<WeakMember<HTMLPlugInElement> > checking for
> > > liveness.
> > >    Then mark & register a weak callback for the hash set to be processed
in
> a
> > > later
> > >    callback. Are those operations allowed from within a(nother) weak
> callback?
> > > 
> > >  - Somehow emulate what the trace() over weak hash set currently
implements,
> > > iterating
> > >    over the elements checking for liveness (and calling
> > shouldDisposePlugin()),
> > > but
> > >    then removing the dead entries. Removing such doesn't appear to be
> > supported
> > > without
> > >    running the risk of a rehash. And how would you mark-but-not-trace the
> > > backing store
> > >    itself. i.e., the functionality that the hash table trace() uses
doesn't
> > > appear to
> > >    be supported in "user defined" weak callbacks -- please tell me if I'm
> > > missing some
> > >    details that really do allow this to be expressed.
> > > 
> > > If the assumption made with what's currently implemented is thought
> > > inappropriate, I can
> > > align with any of the above three -- which one is preferable in that case?
> > 
> > I'm suggesting the first one :)
> > 
> > If I'm understanding code correctly, weak processing for built-in hash maps
is
> > executed during a GC phase (while all other threads are paused). Weak
> processing
> > for clearWeakMembers is executed in thread-specific weak processing, which
> > happens after the GC phase.
> > 
> > Thus, I guess isAlive() in clearWeakMembers always returns true.
> 
> It does not always return true; if no one has marked it by that time, it will
> return false.
> 
> Which you'll see happening on plugins/can-create-without-renderer.html

Do you know how it's guaranteed that weak processing for hash tables happens
before custom weak processing?

As far as I see HashTable.h, it looks like the weak processing for hash tables
are registered by registerWeakMember, which means that there is no guarantee in
order in which the weak processing for hash tables and the custom weak
processing are triggered.
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

(Sorry, I was wrong in that the weak processing for hash tables runs in a GC
phase.)

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:170: (*it)->shouldDisposePlugin();
On 2014/10/12 13:09:12, sof wrote:
> On 2014/10/12 12:56:04, haraken wrote:
> > On 2014/10/12 08:16:22, sof wrote:
> > > On 2014/10/11 17:33:02, haraken wrote:
> > > > 
> > > > shouldDisposePlugin() only does simple things and thus can be called in
a
> > > > pre-finalizer. Can we probably call WebPluginContainerImpl::dispose() in
> > > > HTMLPluginElement's pre-finalizer? Then we can remove the weak hash map
> from
> > > > LocalFrame.
> > > > 
> > > 
> > > How would you know if the LocalFrame is still alive to allow that to go
> ahead?
> > > 
> > > I don't think this suggestion will work -- we trigger it from here when we
> > > _know_ that the Frame has survived (but the plugin element hasn't.)
> > 
> > I'm a bit confused. Pre-finalizers are invoked before any object in the
thread
> > heap is swept. So I guess it's guaranteed that the Frame is still alive in
the
> > pre-finalizers.
> 
> It's still alive, yes. But the plugin is allowed to execute scripts & the like
> (== allocations.)

I agree that dispose() does a bunch of things, but shouldDisposePlugin doesn't
do anything complicated, does it?

[sof, 2014-10-12 16:22:13]

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:158: visitor->trace(m_pluginElements);
On 2014/10/12 13:38:23, haraken wrote:
> On 2014/10/12 13:09:12, sof wrote:
> > On 2014/10/12 12:56:04, haraken wrote:
> > > On 2014/10/12 08:16:22, sof wrote:
> > > > On 2014/10/11 17:33:02, haraken wrote:
> > > > > 
> > > > > Is it safe to trace m_pluginElements? There is no guarantee about the
> > order
> > > of
> > > > > default weak processing and custom weak processing (i.e.,
> > clearWeakMembers).
> > > > > 
> > > > 
> > > > It is safe with the weak callback stack that the implementation keeps.
> i.e.,
> > > > clearWeakMembers() will be invoked before the callback for the weak hash
> set
> > > > impl.
> > > > 
> > > > > For clarity, I'd prefer not tracing m_pluginElements and handing
> > everything
> > > in
> > > > > clearWeakMembers.
> > > > 
> > > > Not clear what you're effectively suggesting. I see three possible
> > > > interpretations:
> > > > 
> > > >  - Stop using Oilpan types (HashSet<HTMLPlugInElement*>) and process
that
> in
> > > >    clearWeakMembers() using isAlive() and then later removing the dead
> > > entries.
> > > > 
> > > >    (A pity to not be able to use Oilpan types to express Oilpan weak
> > reference
> > > >     handling! :) )
> > > > 
> > > >  - Iterate over a HeapHashSet<WeakMember<HTMLPlugInElement> > checking
for
> > > > liveness.
> > > >    Then mark & register a weak callback for the hash set to be processed
> in
> > a
> > > > later
> > > >    callback. Are those operations allowed from within a(nother) weak
> > callback?
> > > > 
> > > >  - Somehow emulate what the trace() over weak hash set currently
> implements,
> > > > iterating
> > > >    over the elements checking for liveness (and calling
> > > shouldDisposePlugin()),
> > > > but
> > > >    then removing the dead entries. Removing such doesn't appear to be
> > > supported
> > > > without
> > > >    running the risk of a rehash. And how would you mark-but-not-trace
the
> > > > backing store
> > > >    itself. i.e., the functionality that the hash table trace() uses
> doesn't
> > > > appear to
> > > >    be supported in "user defined" weak callbacks -- please tell me if
I'm
> > > > missing some
> > > >    details that really do allow this to be expressed.
> > > > 
> > > > If the assumption made with what's currently implemented is thought
> > > > inappropriate, I can
> > > > align with any of the above three -- which one is preferable in that
case?
> > > 
> > > I'm suggesting the first one :)
> > > 
> > > If I'm understanding code correctly, weak processing for built-in hash
maps
> is
> > > executed during a GC phase (while all other threads are paused). Weak
> > processing
> > > for clearWeakMembers is executed in thread-specific weak processing, which
> > > happens after the GC phase.
> > > 
> > > Thus, I guess isAlive() in clearWeakMembers always returns true.
> > 
> > It does not always return true; if no one has marked it by that time, it
will
> > return false.
> > 
> > Which you'll see happening on plugins/can-create-without-renderer.html
> 
> Do you know how it's guaranteed that weak processing for hash tables happens
> before custom weak processing?
> 

There are no such guarantees, but the weak callbacks are popped from their stack
when performed, so if you register the weak callback _after_ the tracing of a
weak collection (which does the same kind of registration), then that weak
callback will be performed first, assuming they belong to the same thread.

I understand if that is an implementation detail we shouldn't rely on (but it is
kind of handy to have/make-use-of right here.)

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:170: (*it)->shouldDisposePlugin();
On 2014/10/12 13:38:23, haraken wrote:
> On 2014/10/12 13:09:12, sof wrote:
> > On 2014/10/12 12:56:04, haraken wrote:
> > > On 2014/10/12 08:16:22, sof wrote:
> > > > On 2014/10/11 17:33:02, haraken wrote:
> > > > > 
> > > > > shouldDisposePlugin() only does simple things and thus can be called
in
> a
> > > > > pre-finalizer. Can we probably call WebPluginContainerImpl::dispose()
in
> > > > > HTMLPluginElement's pre-finalizer? Then we can remove the weak hash
map
> > from
> > > > > LocalFrame.
> > > > > 
> > > > 
> > > > How would you know if the LocalFrame is still alive to allow that to go
> > ahead?
> > > > 
> > > > I don't think this suggestion will work -- we trigger it from here when
we
> > > > _know_ that the Frame has survived (but the plugin element hasn't.)
> > > 
> > > I'm a bit confused. Pre-finalizers are invoked before any object in the
> thread
> > > heap is swept. So I guess it's guaranteed that the Frame is still alive in
> the
> > > pre-finalizers.
> > 
> > It's still alive, yes. But the plugin is allowed to execute scripts & the
like
> > (== allocations.)
> 
> I agree that dispose() does a bunch of things, but shouldDisposePlugin doesn't
> do anything complicated, does it?

Yes, it does not, but it should only be called/signalled _iff_ the LocalFrame is
still alive (and we've not managed to cleanly dispose of the plugin already.)

That information isn't locally available to the plugin element in a
pre-finalizer (*). (It is a solution I looked at and considered.)

* - I think it would be somewhat useful if the pre-finalizer method took
Visitor* as an argument.

[sof, 2014-10-12 18:35:36]

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
File Source/core/frame/LocalFrame.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/core/frame/Local...
Source/core/frame/LocalFrame.cpp:158: visitor->trace(m_pluginElements);
On 2014/10/12 16:22:13, sof wrote:
> On 2014/10/12 13:38:23, haraken wrote:
> > On 2014/10/12 13:09:12, sof wrote:
> > > On 2014/10/12 12:56:04, haraken wrote:
> > > > On 2014/10/12 08:16:22, sof wrote:
> > > > > On 2014/10/11 17:33:02, haraken wrote:
> > > > > > 
> > > > > > Is it safe to trace m_pluginElements? There is no guarantee about
the
> > > order
> > > > of
> > > > > > default weak processing and custom weak processing (i.e.,
> > > clearWeakMembers).
> > > > > > 
> > > > > 
> > > > > It is safe with the weak callback stack that the implementation keeps.
> > i.e.,
> > > > > clearWeakMembers() will be invoked before the callback for the weak
hash
> > set
> > > > > impl.
> > > > > 
> > > > > > For clarity, I'd prefer not tracing m_pluginElements and handing
> > > everything
> > > > in
> > > > > > clearWeakMembers.
> > > > > 
> > > > > Not clear what you're effectively suggesting. I see three possible
> > > > > interpretations:
> > > > > 
> > > > >  - Stop using Oilpan types (HashSet<HTMLPlugInElement*>) and process
> that
> > in
> > > > >    clearWeakMembers() using isAlive() and then later removing the dead
> > > > entries.
> > > > > 
> > > > >    (A pity to not be able to use Oilpan types to express Oilpan weak
> > > reference
> > > > >     handling! :) )
> > > > > 
> > > > >  - Iterate over a HeapHashSet<WeakMember<HTMLPlugInElement> > checking
> for
> > > > > liveness.
> > > > >    Then mark & register a weak callback for the hash set to be
processed
> > in
> > > a
> > > > > later
> > > > >    callback. Are those operations allowed from within a(nother) weak
> > > callback?
> > > > > 
> > > > >  - Somehow emulate what the trace() over weak hash set currently
> > implements,
> > > > > iterating
> > > > >    over the elements checking for liveness (and calling
> > > > shouldDisposePlugin()),
> > > > > but
> > > > >    then removing the dead entries. Removing such doesn't appear to be
> > > > supported
> > > > > without
> > > > >    running the risk of a rehash. And how would you mark-but-not-trace
> the
> > > > > backing store
> > > > >    itself. i.e., the functionality that the hash table trace() uses
> > doesn't
> > > > > appear to
> > > > >    be supported in "user defined" weak callbacks -- please tell me if
> I'm
> > > > > missing some
> > > > >    details that really do allow this to be expressed.
> > > > > 
> > > > > If the assumption made with what's currently implemented is thought
> > > > > inappropriate, I can
> > > > > align with any of the above three -- which one is preferable in that
> case?
> > > > 
> > > > I'm suggesting the first one :)
> > > > 
> > > > If I'm understanding code correctly, weak processing for built-in hash
> maps
> > is
> > > > executed during a GC phase (while all other threads are paused). Weak
> > > processing
> > > > for clearWeakMembers is executed in thread-specific weak processing,
which
> > > > happens after the GC phase.
> > > > 
> > > > Thus, I guess isAlive() in clearWeakMembers always returns true.
> > > 
> > > It does not always return true; if no one has marked it by that time, it
> will
> > > return false.
> > > 
> > > Which you'll see happening on plugins/can-create-without-renderer.html
> > 
> > Do you know how it's guaranteed that weak processing for hash tables happens
> > before custom weak processing?
> > 
> 
> There are no such guarantees, but the weak callbacks are popped from their
stack
> when performed, so if you register the weak callback _after_ the tracing of a
> weak collection (which does the same kind of registration), then that weak
> callback will be performed first, assuming they belong to the same thread.
> 
> I understand if that is an implementation detail we shouldn't rely on (but it
is
> kind of handy to have/make-use-of right here.)

I've switched m_pluginElements to HashSet<HTMLPlugInElement*> and adjusted the
weak pointer handling accordingly.

We can follow up and change it to a "higher level" representation later, if that
ends up the conclusion.

https://codereview.chromium.org/603193005/diff/750001/Source/web/WebHelperPlu...
File Source/web/WebHelperPluginImpl.cpp (right):

https://codereview.chromium.org/603193005/diff/750001/Source/web/WebHelperPlu...
Source/web/WebHelperPluginImpl.cpp:58:
WebHelperPluginImpl::~WebHelperPluginImpl()
On 2014/10/12 12:56:04, haraken wrote:
> On 2014/10/12 08:16:22, sof wrote:
> > On 2014/10/11 17:33:03, haraken wrote:
> > > 
> > > Just to confirm: Isn't it possible that ~WebHelperPluginImpl() is called
> > during
> > > an Oilpan's GC? (WebHelperPluginImpl is off-heap, but it might be possible
> > that
> > > the last reference to the WebHelperPluginImpl is lost when finalizing some
> > > on-heap object.)
> > 
> > WebHelperPlugin is a wrapping layer that the embedder holds onto and
> explicitly
> > deletes. I think we have to assume that what you describe can somehow
happen,
> in
> > the general case. (I can't make out how via the EME player code that uses it
> > atm.)
> > 
> > So, if we end up in ~WebHelperPluginImpl and call dispose() on the
> Persistent<>
> > plugin container reference _and_ the LocalFrame of the plugin container will
> > have been finalized already, then things would break.
> > 
> > Is that really possible, i.e., that the two Persistent<>s to the plugin
> > container and the object/plugin element won't keep the LocalFrame alive
until
> > they're released (by ~WebHelperPluginImpl)? And would only be sweepable
during
> > the next GC. 
> > 
> > I'm assuming not; if you think that's not sound somehow, we could shore it
up
> > adding a Persistent<> over the LocalFrame they're created in.
> 
> Thanks, understood.
> 
> Can we add an ASSERT to verify that something unexpected doesn't actually
> happen? Probably we can add ASSERT(!ThreadState::SweepInProgress()).

I'm not super comfortable with having such an assert around longer term, but I
think we need to understand the case better if it really does arise, so added
it.

[sof, 2014-10-12 20:44:08]

The CQ bit was checked by sigbjornf@opera.com

[commit-bot: I haz the power, 2014-10-12 20:44:58]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/603193005/1370001

[commit-bot: I haz the power, 2014-10-12 20:50:11]

Committed patchset #26 (id:1370001) as 183583

[Nico, 2014-10-13 15:41:11]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2014-10-13 15:41:13]

https://codereview.chromium.org/603193005/diff/1370001/Source/web/tests/Scrol...
File Source/web/tests/ScrollAnimatorNoneTest.cpp (right):

https://codereview.chromium.org/603193005/diff/1370001/Source/web/tests/Scrol...
Source/web/tests/ScrollAnimatorNoneTest.cpp:187: :
ScrollAnimatorNone::PerAxisData(m_mockScrollAnimatorNone.get(), 0, 768)
clang/win bot says:

..\..\third_party\WebKit\Source\web\tests\ScrollAnimatorNoneTest.cpp(187,47) : 
warning(clang): field 'm_mockScrollAnimatorNone' is uninitialized when used here
[-Wuninitialized]
            : ScrollAnimatorNone::PerAxisData(m_mockScrollAnimatorNone.get(), 0,
768)
                                              ^
1 warning generated.

(didn't matter before because before we only took the address of an
uninitialized variable, but now we're calling methods on an object whose
constructor hasn't yet run)

[sof, 2014-10-13 18:05:36]

On 2014/10/13 15:41:13, Nico (away until Oct 27) wrote:
>
https://codereview.chromium.org/603193005/diff/1370001/Source/web/tests/Scrol...
> File Source/web/tests/ScrollAnimatorNoneTest.cpp (right):
> 
>
https://codereview.chromium.org/603193005/diff/1370001/Source/web/tests/Scrol...
> Source/web/tests/ScrollAnimatorNoneTest.cpp:187: :
> ScrollAnimatorNone::PerAxisData(m_mockScrollAnimatorNone.get(), 0, 768)
> clang/win bot says:
> 
> ..\..\third_party\WebKit\Source\web\tests\ScrollAnimatorNoneTest.cpp(187,47) :

> warning(clang): field 'm_mockScrollAnimatorNone' is uninitialized when used
here
> [-Wuninitialized]
>             : ScrollAnimatorNone::PerAxisData(m_mockScrollAnimatorNone.get(),
0,
> 768)
>                                               ^
> 1 warning generated.
> 
> (didn't matter before because before we only took the address of an
> uninitialized variable, but now we're calling methods on an object whose
> constructor hasn't yet run)

Great, thanks for catching that!

The uninitialized PerAxisData constructor argument turns out to be unused
entirely; tidying up via https://codereview.chromium.org/648273002/

[Nico, 2014-10-14 03:49:07]

On Mon, Oct 13, 2014 at 11:05 AM, <sigbjornf@opera.com> wrote:

> The uninitialized PerAxisData constructor argument turns out to be unused
> entirely; tidying up via https://codereview.chromium.org/648273002/


Thanks :-)

To unsubscribe from this group and stop receiving emails from it, send an email
to blink-reviews+unsubscribe@chromium.org.