Oilpan: Move all supplements of Page, Document, and WorkerClients to the managed heap.

Oilpan: Move all supplements of Page, Document, and WorkerClients to the managed heap.

R=ager@chromium.org
BUG=

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172990

[zerny-chromium, 2014-04-29 11:56:31]

This moves document supplements and by transitive cases, page and worker client
supplements. It also moves weak processing of SpeechInputListeners to
SpeechInput now that it is all on heap. PTAL

[sof, 2014-04-29 13:40:20]

I didn't go over the SpeechInput changes in detail, but the supplement(able)
changes look regular&fine to me.

https://codereview.chromium.org/255983003/diff/20001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/255983003/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5673:
WillBeHeapSupplementable<Document>::trace(visitor);
nit: DocumentSupplementable::trace() would be shorter.

[tkent, 2014-04-29 14:26:14]

lgtm except Source/platform/heap/Visitor.h.   I don't understand the Visitor.h
change well.

[Mads Ager (chromium), 2014-04-29 14:56:08]

LGTM!

https://codereview.chromium.org/255983003/diff/20001/Source/core/dom/EventHan...
File Source/core/dom/EventHandlerRegistry.h (right):

https://codereview.chromium.org/255983003/diff/20001/Source/core/dom/EventHan...
Source/core/dom/EventHandlerRegistry.h:19: class EventHandlerRegistry FINAL :
public NoBaseWillBeGarbageCollectedFinalized<EventHandlerRegistry>, public
DocumentSupplement {
Similarly to SpeechInput, this has weak processing via the document that can now
be moved to the right place.

https://codereview.chromium.org/255983003/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/TextControlInnerElements.h (right):

https://codereview.chromium.org/255983003/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/TextControlInnerElements.h:140:
WillBePersistentSpeechInputResultArray m_results;
SpeechInputResultArray and trace it. You can get rid of the
WillBePersistentSpeechInputResultArray type.

https://codereview.chromium.org/255983003/diff/20001/Source/core/speech/Speec...
File Source/core/speech/SpeechInput.cpp (right):

https://codereview.chromium.org/255983003/diff/20001/Source/core/speech/Speec...
Source/core/speech/SpeechInput.cpp:126:
visitor->registerWeakMembers<SpeechInput, &SpeechInput::clearWeakMembers>(this);
Thanks for moving this to the right place. :)

https://codereview.chromium.org/255983003/diff/20001/Source/core/speech/Speec...
File Source/core/speech/SpeechInput.h (right):

https://codereview.chromium.org/255983003/diff/20001/Source/core/speech/Speec...
Source/core/speech/SpeechInput.h:85: WillBeHeapHashMap<int,
RawPtrWillBeWeakMember<SpeechInputListener> > m_listeners;
Thank you!!! :)

[zerny-chromium, 2014-04-29 15:10:35]

Thanks for the reviews.

tkent: the WebCore prefix is needed because the web/ files are in the blink ns.
The new isAlive is because I've added a transition type usage which means a T*
became a RawPtr<T> in the non-oilpan build.

ager: PTAL that you agree on the weak processing.

https://codereview.chromium.org/255983003/diff/20001/Source/core/dom/Document...
File Source/core/dom/Document.cpp (right):

https://codereview.chromium.org/255983003/diff/20001/Source/core/dom/Document...
Source/core/dom/Document.cpp:5673:
WillBeHeapSupplementable<Document>::trace(visitor);
On 2014/04/29 13:40:21, sof wrote:
> nit: DocumentSupplementable::trace() would be shorter.

Done.

https://codereview.chromium.org/255983003/diff/20001/Source/core/dom/EventHan...
File Source/core/dom/EventHandlerRegistry.h (right):

https://codereview.chromium.org/255983003/diff/20001/Source/core/dom/EventHan...
Source/core/dom/EventHandlerRegistry.h:19: class EventHandlerRegistry FINAL :
public NoBaseWillBeGarbageCollectedFinalized<EventHandlerRegistry>, public
DocumentSupplement {
On 2014/04/29 14:56:09, Mads Ager (chromium) wrote:
> Similarly to SpeechInput, this has weak processing via the document that can
now
> be moved to the right place.

Missed that one. Done.

https://codereview.chromium.org/255983003/diff/20001/Source/core/html/shadow/...
File Source/core/html/shadow/TextControlInnerElements.h (right):

https://codereview.chromium.org/255983003/diff/20001/Source/core/html/shadow/...
Source/core/html/shadow/TextControlInnerElements.h:140:
WillBePersistentSpeechInputResultArray m_results;
On 2014/04/29 14:56:09, Mads Ager (chromium) wrote:
> SpeechInputResultArray and trace it. You can get rid of the
> WillBePersistentSpeechInputResultArray type.

Done.

[Mads Ager (chromium), 2014-04-29 16:04:53]

LGTM, thanks for moving the EventHandlerRegistry weak processing to the right
place as well. :)

[haraken, 2014-04-30 02:41:21]

This CL contains multiple changes. I'm OK with this CL but let's consider
splitting this kind of large CL into pieces from a next CL :)

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/CSSSelec...
File Source/core/dom/CSSSelectorWatch.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/CSSSelec...
Source/core/dom/CSSSelectorWatch.h:61: CSSSelectorWatch(Document&);

Add explicit.

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/ContextF...
File Source/core/dom/ContextFeatures.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/ContextF...
Source/core/dom/ContextFeatures.h:39: class ContextFeatures FINAL : public
RefCountedGarbageCollected<ContextFeatures>, public HeapSupplement<Page> {

Does this need to be RefCountedGarbageCollected? Can this be
GarbageCollectedFinalized?

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/EventHan...
File Source/core/dom/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/EventHan...
Source/core/dom/EventHandlerRegistry.cpp:207: if (m_document.isActive())

Nit: This is OK but I'd move the m_document.isActive() check and the comment
into clearWeakMembers. trace() should just trace things and we don't want to do
something non-trivial into trace().

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/Fullscre...
File Source/core/dom/FullscreenElementStack.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/Fullscre...
Source/core/dom/FullscreenElementStack.h:115: RenderFullScreen*
m_fullScreenRenderer;

This raw pointer looks safe since it's explicitly cleared.

https://codereview.chromium.org/255983003/diff/40001/Source/core/html/imports...
File Source/core/html/imports/HTMLImportsController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/core/html/imports...
Source/core/html/imports/HTMLImportsController.h:54: WTF_MAKE_FAST_ALLOCATED;

Shouldn't this be WTF_MAKE_FAST_ALLOCATED_WILL_BE_REMOVED? I'm curious why this
CL compiles without adding _WILL_BE_REMOVED.

https://codereview.chromium.org/255983003/diff/40001/Source/core/testing/Inte...
File Source/core/testing/InternalSettings.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/core/testing/Inte...
Source/core/testing/InternalSettings.h:85: #endif

Just I'm curious but why do we need to clear m_page in non-oilpan builds? Given
that the InternalSettings is a supplement object of a Page, the InternalSettings
and the Page should die together in non-oilpan. If so, we won't need to clear
m_page in non-oilpan as well.

https://codereview.chromium.org/255983003/diff/40001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeysController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeysController.h:33: MediaKeysClient*
m_client;

Are you sure that this raw pointer is safe?

https://codereview.chromium.org/255983003/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/GeolocationController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeolocationController.h:84: GeolocationClient*
m_client;

Ditto. I wonder how it's guaranteed that the client does not outlive the
GeolocationController.

https://codereview.chromium.org/255983003/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeolocationController.h:93:
GeolocationInspectorAgent* m_inspectorAgent;

This raw pointer looks safe since Page owns the InspectorController that owns
GeolocationInspectorAgent.

https://codereview.chromium.org/255983003/diff/40001/Source/modules/push_mess...
File Source/modules/push_messaging/PushController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/push_mess...
Source/modules/push_messaging/PushController.h:37: blink::WebPushClient*
m_client;

Ditto. Is this raw pointer safe?

https://codereview.chromium.org/255983003/diff/40001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:18:
ScreenOrientationDispatcher::instance().removeController(this);

I think you need to change ScreenOrientationDispatcher::m_controllers to a hash
set of weak members.

It's not a good thing to let some object hold a raw pointer to GarbageCollected
objects. Let's confirm that for all GarbageCollected classes in this CL, no
object is holding a raw pointer to the GarbageCollected objects.

https://codereview.chromium.org/255983003/diff/40001/Source/modules/webdataba...
File Source/modules/webdatabase/DatabaseClient.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/webdataba...
Source/modules/webdatabase/DatabaseClient.h:63: InspectorDatabaseAgent*
m_inspectorAgent;

This raw pointer looks safe because the Page owns InspectorController that owns
the InspectorDatabaseAgent.

https://codereview.chromium.org/255983003/diff/40001/Source/web/DatabaseClien...
File Source/web/DatabaseClientImpl.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/web/DatabaseClien...
Source/web/DatabaseClientImpl.h:39: class DatabaseClientImpl FINAL : public
NoBaseWillBeGarbageCollectedFinalized<DatabaseClientImpl>, public
WebCore::DatabaseClient {

How about making DatabaseClient NoBaseWillBeGarbageCollectedFinalized, instead
of making DatabaseClientImpl NoBaseWillBeGarbageCollectedFinalized?

Your CL will work, but it would look nicer if DatabaseClient inherits from both
NoBaseWillBeGarbageCollectedFinalized and HeapSupplement.

https://codereview.chromium.org/255983003/diff/40001/Source/web/PrerendererCl...
File Source/web/PrerendererClientImpl.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/web/PrerendererCl...
Source/web/PrerendererClientImpl.h:47: class PrerendererClientImpl FINAL :
public NoBaseWillBeGarbageCollected<PrerendererClientImpl>, public
WebCore::PrerendererClient {

Ditto. You can make PrerendererClient NoBaseWillBeGarbageCollected.

https://codereview.chromium.org/255983003/diff/40001/Source/web/PrerendererCl...
Source/web/PrerendererClientImpl.h:58: WebPrerendererClient* m_client;

Are you sure that this raw pointer is safe?

Some Impl objects have an OwnPtr to the client (e.g., ServiceWorkerClientImpl).
Some Impl objects have a raw pointer to the client (e.g.,
PrerendererClientImpl). I don't quite understand the difference and why the raw
pointer is safe.

https://codereview.chromium.org/255983003/diff/40001/Source/web/ServiceWorker...
File Source/web/ServiceWorkerGlobalScopeClientImpl.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/web/ServiceWorker...
Source/web/ServiceWorkerGlobalScopeClientImpl.h:43: class
ServiceWorkerGlobalScopeClientImpl FINAL : public
NoBaseWillBeGarbageCollectedFinalized<ServiceWorkerGlobalScopeClientImpl>,
public WebCore::ServiceWorkerGlobalScopeClient {

Ditto. You can make ServiceWorkerClient NoBaseWillBeGarbageCollectedFinalized.

https://codereview.chromium.org/255983003/diff/40001/Source/web/StorageQuotaC...
File Source/web/StorageQuotaClientImpl.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/web/StorageQuotaC...
Source/web/StorageQuotaClientImpl.h:39: class StorageQuotaClientImpl : public
NoBaseWillBeGarbageCollectedFinalized<StorageQuotaClientImpl>, public
WebCore::StorageQuotaClient {

Ditto. You can make StorageQuotaClient NoBaseWillBeGarbageCollectedFinalized.

https://codereview.chromium.org/255983003/diff/40001/Source/web/WorkerGlobalS...
File Source/web/WorkerGlobalScopeProxyProviderImpl.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/web/WorkerGlobalS...
Source/web/WorkerGlobalScopeProxyProviderImpl.h:45: class
WorkerGlobalScopeProxyProviderImpl FINAL : public
NoBaseWillBeGarbageCollectedFinalized<WorkerGlobalScopeProxyProviderImpl>,
public WebCore::WorkerGlobalScopeProxyProvider {

Ditto.

https://codereview.chromium.org/255983003/diff/40001/Source/web/WorkerPermiss...
File Source/web/WorkerPermissionClient.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/web/WorkerPermiss...
Source/web/WorkerPermissionClient.h:48: class WorkerPermissionClient FINAL :
public NoBaseWillBeGarbageCollectedFinalized<WorkerPermissionClient>, public
WillBeHeapSupplement<WebCore::WorkerClients> {

This client looks nice -- it inherits from both
NoBaseWillBeGarbageCollectedFinalized and HeapSupplement. It would be consistent
if all clients follow the rule :)

[zerny-chromium, 2014-04-30 08:29:33]

Thanks for the review Haraken. I agree that it would be nice to keep CLs small,
but I don't see a nice way to avoid moving the
Supplement<{Page,Document,WorkerClients}> at the same time. This is the smallest
CL I can think of that moves them all.

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/CSSSelec...
File Source/core/dom/CSSSelectorWatch.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/CSSSelec...
Source/core/dom/CSSSelectorWatch.h:61: CSSSelectorWatch(Document&);
On 2014/04/30 02:41:22, haraken wrote:
> 
> Add explicit.

Done.

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/ContextF...
File Source/core/dom/ContextFeatures.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/ContextF...
Source/core/dom/ContextFeatures.h:39: class ContextFeatures FINAL : public
RefCountedGarbageCollected<ContextFeatures>, public HeapSupplement<Page> {
On 2014/04/30 02:41:22, haraken wrote:
> 
> Does this need to be RefCountedGarbageCollected? Can this be
> GarbageCollectedFinalized?

It is ref-counted in non-oilpan builds so I'm assuming that someone holds a
RefPtr to it. Removing the RefPtr's is a task for another CL.

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/EventHan...
File Source/core/dom/EventHandlerRegistry.cpp (right):

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/EventHan...
Source/core/dom/EventHandlerRegistry.cpp:207: if (m_document.isActive())
On 2014/04/30 02:41:22, haraken wrote:
> 
> Nit: This is OK but I'd move the m_document.isActive() check and the comment
> into clearWeakMembers. trace() should just trace things and we don't want to
do
> something non-trivial into trace().
> 

Done.

https://codereview.chromium.org/255983003/diff/40001/Source/core/html/imports...
File Source/core/html/imports/HTMLImportsController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/core/html/imports...
Source/core/html/imports/HTMLImportsController.h:54: WTF_MAKE_FAST_ALLOCATED;
On 2014/04/30 02:41:22, haraken wrote:
> 
> Shouldn't this be WTF_MAKE_FAST_ALLOCATED_WILL_BE_REMOVED? I'm curious why
this
> CL compiles without adding _WILL_BE_REMOVED.

Thanks! Nothing at compile-time disallows defining new/delete for a GC'ed class.
I'll look into checking against redeclaration in the plugin. (I actually thought
that was added already, but that was on the branch plugin).

https://codereview.chromium.org/255983003/diff/40001/Source/core/testing/Inte...
File Source/core/testing/InternalSettings.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/core/testing/Inte...
Source/core/testing/InternalSettings.h:85: #endif
On 2014/04/30 02:41:22, haraken wrote:
> 
> Just I'm curious but why do we need to clear m_page in non-oilpan builds?
Given
> that the InternalSettings is a supplement object of a Page, the
InternalSettings
> and the Page should die together in non-oilpan. If so, we won't need to clear
> m_page in non-oilpan as well.

InternalSettings is RefCounted and does not keep the page alive, so it can
outlive the page. Thus m_page needs to be a WeakMember in the oilpan build.
Thanks for noticing the discrepancy!

https://codereview.chromium.org/255983003/diff/40001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeysController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeysController.h:33: MediaKeysClient*
m_client;
On 2014/04/30 02:41:22, haraken wrote:
> 
> Are you sure that this raw pointer is safe?

I'm not sure I follow. MediaKeysClient is off-heap and the management of its
instances is unaffected by this CL as far as I can see.

https://codereview.chromium.org/255983003/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/GeolocationController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeolocationController.h:84: GeolocationClient*
m_client;
On 2014/04/30 02:41:22, haraken wrote:
> 
> Ditto. I wonder how it's guaranteed that the client does not outlive the
> GeolocationController.

The opposite needs to hold. The client should always outlive the controller
since the controller has a raw pointer to the client. The client appears to be a
proxy (eg GeolocationClientProxy) which points to a 'web' client (eg
WebGeolocationClient). I'm not sure about the lifetime of those two, but they
are unaffected by this CL.

https://codereview.chromium.org/255983003/diff/40001/Source/modules/push_mess...
File Source/modules/push_messaging/PushController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/push_mess...
Source/modules/push_messaging/PushController.h:37: blink::WebPushClient*
m_client;
On 2014/04/30 02:41:22, haraken wrote:
> 
> Ditto. Is this raw pointer safe?

It should be as safe/unsafe as it was prior to this CL.

https://codereview.chromium.org/255983003/diff/40001/Source/modules/screen_or...
File Source/modules/screen_orientation/ScreenOrientationController.cpp (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/screen_or...
Source/modules/screen_orientation/ScreenOrientationController.cpp:18:
ScreenOrientationDispatcher::instance().removeController(this);
On 2014/04/30 02:41:22, haraken wrote:
> 
> I think you need to change ScreenOrientationDispatcher::m_controllers to a
hash
> set of weak members.
> 
> It's not a good thing to let some object hold a raw pointer to
GarbageCollected
> objects. Let's confirm that for all GarbageCollected classes in this CL, no
> object is holding a raw pointer to the GarbageCollected objects.

In the transition we can't avoid some cases of raw pointers to GC'ed objects,
which is why the plugin does not report such errors yet.

In the present case, m_controllers can't immediately be made weak because the
ScreenOrientationDispatcher is off heap. Moving it and updating the logic is not
completely trivial, so I'd like to postpone that to a followup CL. (On Oilpan we
have different assumptions regarding when removeController will be called (ie,
it is never called under dispatching)).

The current code should be safe since the dispatcher does not access any of its
controllers during removal and is guarantied to outlive the controllers.

https://codereview.chromium.org/255983003/diff/40001/Source/web/DatabaseClien...
File Source/web/DatabaseClientImpl.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/web/DatabaseClien...
Source/web/DatabaseClientImpl.h:39: class DatabaseClientImpl FINAL : public
NoBaseWillBeGarbageCollectedFinalized<DatabaseClientImpl>, public
WebCore::DatabaseClient {
On 2014/04/30 02:41:22, haraken wrote:
> 
> How about making DatabaseClient NoBaseWillBeGarbageCollectedFinalized, instead
> of making DatabaseClientImpl NoBaseWillBeGarbageCollectedFinalized?
> 
> Your CL will work, but it would look nicer if DatabaseClient inherits from
both
> NoBaseWillBeGarbageCollectedFinalized and HeapSupplement.

We could do that, but I've placed the GC bases as the left-most leaf when
possible since that is the class that can actually be instantiated (eg, it has
the create method which connects with the base). Also, it is the most flexible
setup (eg, DatabaseClient can be used as a mixin to other GC'ed classes).

https://codereview.chromium.org/255983003/diff/40001/Source/web/PrerendererCl...
File Source/web/PrerendererClientImpl.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/web/PrerendererCl...
Source/web/PrerendererClientImpl.h:58: WebPrerendererClient* m_client;
On 2014/04/30 02:41:22, haraken wrote:
> 
> Are you sure that this raw pointer is safe?
> 
> Some Impl objects have an OwnPtr to the client (e.g.,
ServiceWorkerClientImpl).
> Some Impl objects have a raw pointer to the client (e.g.,
> PrerendererClientImpl). I don't quite understand the difference and why the
raw
> pointer is safe.

I'm not sure either, but that is orthogonal to this CL as far as I can tell.

https://codereview.chromium.org/255983003/diff/40001/Source/web/WorkerPermiss...
File Source/web/WorkerPermissionClient.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/web/WorkerPermiss...
Source/web/WorkerPermissionClient.h:48: class WorkerPermissionClient FINAL :
public NoBaseWillBeGarbageCollectedFinalized<WorkerPermissionClient>, public
WillBeHeapSupplement<WebCore::WorkerClients> {
On 2014/04/30 02:41:22, haraken wrote:
> 
> This client looks nice -- it inherits from both
> NoBaseWillBeGarbageCollectedFinalized and HeapSupplement. It would be
consistent
> if all clients follow the rule :)

I've followed the alternative rule: place the GC base at the left-most outermost
position that is actually allocatable. The intermediates are really interfaces
and can remain mixins.

[haraken, 2014-04-30 11:18:07]

Thanks for the update. This is the final round of comments.

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/ContextF...
File Source/core/dom/ContextFeatures.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/core/dom/ContextF...
Source/core/dom/ContextFeatures.h:39: class ContextFeatures FINAL : public
RefCountedGarbageCollected<ContextFeatures>, public HeapSupplement<Page> {
On 2014/04/30 08:29:34, zerny-chromium wrote:
> On 2014/04/30 02:41:22, haraken wrote:
> > 
> > Does this need to be RefCountedGarbageCollected? Can this be
> > GarbageCollectedFinalized?
> 
> It is ref-counted in non-oilpan builds so I'm assuming that someone holds a
> RefPtr to it. Removing the RefPtr's is a task for another CL.

There are a few places that hold RefPtrs to ContextFeatures. I think you can
just replace them with RefPtrWillBeMembers. A follow-up CL is fine.

https://codereview.chromium.org/255983003/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/GeolocationController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeolocationController.h:84: GeolocationClient*
m_client;
On 2014/04/30 08:29:34, zerny-chromium wrote:
> On 2014/04/30 02:41:22, haraken wrote:
> > 
> > Ditto. I wonder how it's guaranteed that the client does not outlive the
> > GeolocationController.
> 
> The opposite needs to hold. The client should always outlive the controller
> since the controller has a raw pointer to the client. The client appears to be
a
> proxy (eg GeolocationClientProxy) which points to a 'web' client (eg
> WebGeolocationClient). I'm not sure about the lifetime of those two, but they
> are unaffected by this CL.

This raw pointer happens to be safe because WebViewImpl owns both Page and
GeolocationClient, and thus they die together.

However, "they are unaffected by this CL" is not true, because this CL changes
the lifetime of the Page. Previously the Page was dead promptly when the OwnPtr
to the Page is gone. After this CL, the Page does not die until the next GC is
triggered. Thus it's possible that the client can outlive the Page.

In general, we need to call m_client->willBeDestroyed() when
Page::willBeDestroyed() is called and clear out the m_client. See what
Page::willBeDestroyed() is doing for clients of the Page. (Of course, if there
is nothing to do in m_client->willBeDestroyed(), we don't need to call it.)

I think Mads is familiar with this issue since he addressed the client issues
when moving Page to the heap.

https://codereview.chromium.org/255983003/diff/60001/Source/core/dom/ContextF...
File Source/core/dom/ContextFeatures.h (right):

https://codereview.chromium.org/255983003/diff/60001/Source/core/dom/ContextF...
Source/core/dom/ContextFeatures.h:70: #if ENABLE(OILPAN)

Sometimes you're adding ENABLE(OILPAN) to trace(Visitor*) and sometimes not.
What's the difference?

https://codereview.chromium.org/255983003/diff/60001/Source/core/loader/Prere...
File Source/core/loader/PrerendererClient.h (left):

https://codereview.chromium.org/255983003/diff/60001/Source/core/loader/Prere...
Source/core/loader/PrerendererClient.h:45: virtual ~PrerendererClient() { }

You can use DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED.

https://codereview.chromium.org/255983003/diff/60001/Source/core/speech/Speec...
File Source/core/speech/SpeechInput.cpp (right):

https://codereview.chromium.org/255983003/diff/60001/Source/core/speech/Speec...
Source/core/speech/SpeechInput.cpp:63: for (HashMap<int,
SpeechInputListener*>::iterator it = m_listeners.begin(); it !=
m_listeners.end(); ++it)

This could be WillBeHeapHashMap<int, RawPtrWillBeWeakMember<SpeechInputListener>
>.

https://codereview.chromium.org/255983003/diff/60001/Source/core/testing/Inte...
File Source/core/testing/InternalSettings.h (right):

https://codereview.chromium.org/255983003/diff/60001/Source/core/testing/Inte...
Source/core/testing/InternalSettings.h:136: RawPtrWillBeWeakMember<Page> m_page;

hmm, I'm not quite sure about this pointer.

- In oilpan, InternalSettings is a HeapSupplement<Page>, so the InternalSettings
and the Page should die together. Thus this should be a strong pointer. So I
think this pointer should be RawPtrWillBeMember.

- In non-oilpan, weak processing for m_page is necessary because we have
InternalSettingsWrapper (for some reason). Do you know why we need
InternalSettingsWrapper? Can't we just make InternalSettings derive
InternalSettingsGenerated and Supplement<Page>? In other words, I wonder why we
cannot write like this:

class InternalSettings : public InternalSettingsGenerated, public
WillBeHeapSupplement<Page> { ... };

https://codereview.chromium.org/255983003/diff/60001/Source/core/xml/Document...
File Source/core/xml/DocumentXPathEvaluator.h (left):

https://codereview.chromium.org/255983003/diff/60001/Source/core/xml/Document...
Source/core/xml/DocumentXPathEvaluator.h:41: virtual ~DocumentXPathEvaluator();

You can use DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED.

https://codereview.chromium.org/255983003/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeysController.h (left):

https://codereview.chromium.org/255983003/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeysController.h:22: virtual
~MediaKeysController();

You can use DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED.

https://codereview.chromium.org/255983003/diff/60001/Source/modules/push_mess...
File Source/modules/push_messaging/PushController.h (left):

https://codereview.chromium.org/255983003/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushController.h:24: virtual ~PushController();

You can use DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED.

[haraken, 2014-04-30 11:19:57]

https://codereview.chromium.org/255983003/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/GeolocationController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeolocationController.h:84: GeolocationClient*
m_client;
On 2014/04/30 11:18:08, haraken wrote:
> On 2014/04/30 08:29:34, zerny-chromium wrote:
> > On 2014/04/30 02:41:22, haraken wrote:
> > > 
> > > Ditto. I wonder how it's guaranteed that the client does not outlive the
> > > GeolocationController.
> > 
> > The opposite needs to hold. The client should always outlive the controller
> > since the controller has a raw pointer to the client. The client appears to
be
> a
> > proxy (eg GeolocationClientProxy) which points to a 'web' client (eg
> > WebGeolocationClient). I'm not sure about the lifetime of those two, but
they
> > are unaffected by this CL.
> 
> This raw pointer happens to be safe because WebViewImpl owns both Page and
> GeolocationClient, and thus they die together.
> 
> However, "they are unaffected by this CL" is not true, because this CL changes
> the lifetime of the Page. Previously the Page was dead promptly when the
OwnPtr
> to the Page is gone. After this CL, the Page does not die until the next GC is
> triggered. Thus it's possible that the client can outlive the Page.
> 
> In general, we need to call m_client->willBeDestroyed() when
> Page::willBeDestroyed() is called and clear out the m_client. See what
> Page::willBeDestroyed() is doing for clients of the Page. (Of course, if there
> is nothing to do in m_client->willBeDestroyed(), we don't need to call it.)
> 
> I think Mads is familiar with this issue since he addressed the client issues
> when moving Page to the heap.

Note: I haven't yet checked if other m_client pointers in this CL are safe.

[zerny-chromium, 2014-04-30 11:46:52]

Thanks for your careful eye Haraken. I'll have a look at the remaining client
pointers, but I still don't see how this CL changes their lifetime (non of the
clients are owned/held alive by supplements, which are the only objects I'm
moving to the heap + WorkerClients).

https://codereview.chromium.org/255983003/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/GeolocationController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeolocationController.h:84: GeolocationClient*
m_client;
On 2014/04/30 11:19:58, haraken wrote:
> On 2014/04/30 11:18:08, haraken wrote:
> > On 2014/04/30 08:29:34, zerny-chromium wrote:
> > > On 2014/04/30 02:41:22, haraken wrote:
> > > > 
> > > > Ditto. I wonder how it's guaranteed that the client does not outlive the
> > > > GeolocationController.
> > > 
> > > The opposite needs to hold. The client should always outlive the
controller
> > > since the controller has a raw pointer to the client. The client appears
to
> be
> > a
> > > proxy (eg GeolocationClientProxy) which points to a 'web' client (eg
> > > WebGeolocationClient). I'm not sure about the lifetime of those two, but
> they
> > > are unaffected by this CL.
> > 
> > This raw pointer happens to be safe because WebViewImpl owns both Page and
> > GeolocationClient, and thus they die together.
> > 
> > However, "they are unaffected by this CL" is not true, because this CL
changes
> > the lifetime of the Page.

Page is already moved. This CL does not change its lifetime.

https://codereview.chromium.org/255983003/diff/60001/Source/core/dom/ContextF...
File Source/core/dom/ContextFeatures.h (right):

https://codereview.chromium.org/255983003/diff/60001/Source/core/dom/ContextF...
Source/core/dom/ContextFeatures.h:70: #if ENABLE(OILPAN)
On 2014/04/30 11:18:08, haraken wrote:
> 
> Sometimes you're adding ENABLE(OILPAN) to trace(Visitor*) and sometimes not.
> What's the difference?

It is needed in this particular case because in the non-oilpan build
ContextFeatures is not itself a Supplement (RefCountedSupplement defines a
wrapper which delegates to ContextFeatures).

https://codereview.chromium.org/255983003/diff/60001/Source/core/loader/Prere...
File Source/core/loader/PrerendererClient.h (left):

https://codereview.chromium.org/255983003/diff/60001/Source/core/loader/Prere...
Source/core/loader/PrerendererClient.h:45: virtual ~PrerendererClient() { }
On 2014/04/30 11:18:08, haraken wrote:
> 
> You can use DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED.

Should we not rather remove it since it is not needed in either build?

https://codereview.chromium.org/255983003/diff/60001/Source/core/speech/Speec...
File Source/core/speech/SpeechInput.cpp (right):

https://codereview.chromium.org/255983003/diff/60001/Source/core/speech/Speec...
Source/core/speech/SpeechInput.cpp:63: for (HashMap<int,
SpeechInputListener*>::iterator it = m_listeners.begin(); it !=
m_listeners.end(); ++it)
On 2014/04/30 11:18:08, haraken wrote:
> 
> This could be WillBeHeapHashMap<int,
RawPtrWillBeWeakMember<SpeechInputListener>
> >.

And it should be. Thanks for spotting it.

https://codereview.chromium.org/255983003/diff/60001/Source/core/testing/Inte...
File Source/core/testing/InternalSettings.h (right):

https://codereview.chromium.org/255983003/diff/60001/Source/core/testing/Inte...
Source/core/testing/InternalSettings.h:136: RawPtrWillBeWeakMember<Page> m_page;
On 2014/04/30 11:18:08, haraken wrote:
> 
> hmm, I'm not quite sure about this pointer.
> 
> - In oilpan, InternalSettings is a HeapSupplement<Page>, so the
InternalSettings
> and the Page should die together. Thus this should be a strong pointer. So I
> think this pointer should be RawPtrWillBeMember.
> 
> - In non-oilpan, weak processing for m_page is necessary because we have
> InternalSettingsWrapper (for some reason). Do you know why we need
> InternalSettingsWrapper? Can't we just make InternalSettings derive
> InternalSettingsGenerated and Supplement<Page>? In other words, I wonder why
we
> cannot write like this:
> 
> class InternalSettings : public InternalSettingsGenerated, public
> WillBeHeapSupplement<Page> { ... };

That is because InternalSettingsGenerated is RefCounted and a Supplement can't
be RefCounted. Since the base is already RefCounted we can't use
RefCountedSupplement, so the wrapper class is explicitly defined for the
non-oilpan build. For the oilpan-build the issue goes away. In both cases we are
not ensured that InternalSettings lives and dies with Page because others can
have RefPtrs/Members to it.

In general, a HeapSupplement can outlive the object it is a supplement to.

https://codereview.chromium.org/255983003/diff/60001/Source/core/xml/Document...
File Source/core/xml/DocumentXPathEvaluator.h (left):

https://codereview.chromium.org/255983003/diff/60001/Source/core/xml/Document...
Source/core/xml/DocumentXPathEvaluator.h:41: virtual ~DocumentXPathEvaluator();
On 2014/04/30 11:18:08, haraken wrote:
> 
> You can use DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED.

Again, is it not best to simply remove it now?

https://codereview.chromium.org/255983003/diff/60001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeysController.h (left):

https://codereview.chromium.org/255983003/diff/60001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeysController.h:22: virtual
~MediaKeysController();
On 2014/04/30 11:18:08, haraken wrote:
> 
> You can use DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED.

ditto?

https://codereview.chromium.org/255983003/diff/60001/Source/modules/push_mess...
File Source/modules/push_messaging/PushController.h (left):

https://codereview.chromium.org/255983003/diff/60001/Source/modules/push_mess...
Source/modules/push_messaging/PushController.h:24: virtual ~PushController();
On 2014/04/30 11:18:08, haraken wrote:
> 
> You can use DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED.

ditto?

[haraken, 2014-04-30 12:02:07]

LGTM.

https://codereview.chromium.org/255983003/diff/40001/Source/modules/geolocati...
File Source/modules/geolocation/GeolocationController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/geolocati...
Source/modules/geolocation/GeolocationController.h:84: GeolocationClient*
m_client;
On 2014/04/30 11:46:53, zerny-chromium wrote:
> On 2014/04/30 11:19:58, haraken wrote:
> > On 2014/04/30 11:18:08, haraken wrote:
> > > On 2014/04/30 08:29:34, zerny-chromium wrote:
> > > > On 2014/04/30 02:41:22, haraken wrote:
> > > > > 
> > > > > Ditto. I wonder how it's guaranteed that the client does not outlive
the
> > > > > GeolocationController.
> > > > 
> > > > The opposite needs to hold. The client should always outlive the
> controller
> > > > since the controller has a raw pointer to the client. The client appears
> to
> > be
> > > a
> > > > proxy (eg GeolocationClientProxy) which points to a 'web' client (eg
> > > > WebGeolocationClient). I'm not sure about the lifetime of those two, but
> > they
> > > > are unaffected by this CL.
> > > 
> > > This raw pointer happens to be safe because WebViewImpl owns both Page and
> > > GeolocationClient, and thus they die together.
> > > 
> > > However, "they are unaffected by this CL" is not true, because this CL
> changes
> > > the lifetime of the Page.
> 
> Page is already moved. This CL does not change its lifetime.

Sorry, I was misunderstanding! The supplementable objects are already moved to
the heap, so this CL doesn't change any lifetime. Thus the CL doesn't produce
any new issues.

https://codereview.chromium.org/255983003/diff/60001/Source/core/loader/Prere...
File Source/core/loader/PrerendererClient.h (left):

https://codereview.chromium.org/255983003/diff/60001/Source/core/loader/Prere...
Source/core/loader/PrerendererClient.h:45: virtual ~PrerendererClient() { }
On 2014/04/30 11:46:53, zerny-chromium wrote:
> On 2014/04/30 11:18:08, haraken wrote:
> > 
> > You can use DECLARE_EMPTY_DESTRUCTOR_WILL_BE_REMOVED.
> 
> Should we not rather remove it since it is not needed in either build?

If you omit an empty destructor, the destructor is implicitly generated in
header files, which can cause #include cycles. Also we sometimes need to define
an empty, virtual destructor. For these situations, we introduced
DECLARE_EMPTY_VIRTUAL_DESTRUCTOR_WILL_BE_REMOVED.

Either way this wouldn't be a big deal. You can just remove it if it doesn't
matter :)

https://codereview.chromium.org/255983003/diff/60001/Source/core/testing/Inte...
File Source/core/testing/InternalSettings.h (right):

https://codereview.chromium.org/255983003/diff/60001/Source/core/testing/Inte...
Source/core/testing/InternalSettings.h:136: RawPtrWillBeWeakMember<Page> m_page;
On 2014/04/30 11:46:53, zerny-chromium wrote:
> On 2014/04/30 11:18:08, haraken wrote:
> > 
> > hmm, I'm not quite sure about this pointer.
> > 
> > - In oilpan, InternalSettings is a HeapSupplement<Page>, so the
> InternalSettings
> > and the Page should die together. Thus this should be a strong pointer. So I
> > think this pointer should be RawPtrWillBeMember.
> > 
> > - In non-oilpan, weak processing for m_page is necessary because we have
> > InternalSettingsWrapper (for some reason). Do you know why we need
> > InternalSettingsWrapper? Can't we just make InternalSettings derive
> > InternalSettingsGenerated and Supplement<Page>? In other words, I wonder why
> we
> > cannot write like this:
> > 
> > class InternalSettings : public InternalSettingsGenerated, public
> > WillBeHeapSupplement<Page> { ... };
> 
> That is because InternalSettingsGenerated is RefCounted and a Supplement can't
> be RefCounted. Since the base is already RefCounted we can't use
> RefCountedSupplement, so the wrapper class is explicitly defined for the
> non-oilpan build. For the oilpan-build the issue goes away. In both cases we
are
> not ensured that InternalSettings lives and dies with Page because others can
> have RefPtrs/Members to it.

Makes sense.


> In general, a HeapSupplement can outlive the object it is a supplement to.

In what situation do we want to let a supplement object outlive the
supplementable object?

If it outlives, we also need to care about a raw pointer in HeapSupplement that
points to the supplementable object.

class X : public HeapSupplement<Document> {
  Document& m_document;  // I was assuming that this raw pointer is safe because
the Document and X die together. We have this kind of raw pointer in this CL.
};

[zerny-chromium, 2014-04-30 12:11:25]

https://codereview.chromium.org/255983003/diff/40001/Source/modules/encrypted...
File Source/modules/encryptedmedia/MediaKeysController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/encrypted...
Source/modules/encryptedmedia/MediaKeysController.h:33: MediaKeysClient*
m_client;
On 2014/04/30 02:41:22, haraken wrote:
> 
> Are you sure that this raw pointer is safe?

This is a part object of WebViewImpl which owns Page.

https://codereview.chromium.org/255983003/diff/40001/Source/modules/push_mess...
File Source/modules/push_messaging/PushController.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/modules/push_mess...
Source/modules/push_messaging/PushController.h:37: blink::WebPushClient*
m_client;
On 2014/04/30 02:41:22, haraken wrote:
> 
> Ditto. Is this raw pointer safe?

I can't find an owner of this pointer. WebPushClient is abstract and there are
no derived classes in WebKit/

The only access to a WebPushClient is WebViewClient::webPushClient()  and so it
is not apparent what lifetime requirements there are for a WebViewClient
implementation.

https://codereview.chromium.org/255983003/diff/40001/Source/web/PrerendererCl...
File Source/web/PrerendererClientImpl.h (right):

https://codereview.chromium.org/255983003/diff/40001/Source/web/PrerendererCl...
Source/web/PrerendererClientImpl.h:58: WebPrerendererClient* m_client;
On 2014/04/30 02:41:22, haraken wrote:
> 
> Are you sure that this raw pointer is safe?
> 
> Some Impl objects have an OwnPtr to the client (e.g.,
ServiceWorkerClientImpl).
> Some Impl objects have a raw pointer to the client (e.g.,
> PrerendererClientImpl). I don't quite understand the difference and why the
raw
> pointer is safe.

Again, here, it is not clear what lifetime guaranties we have for
WebPrerendererClient. It is provided as an input to
WebView::setPrerendererClient(...) which create and attaches the supplement to
the page. The lifetime appears to be external.

[zerny-chromium, 2014-04-30 12:23:22]

> > In general, a HeapSupplement can outlive the object it is a supplement to.
> 
> In what situation do we want to let a supplement object outlive the
> supplementable object?

All of the RefCountedSupplement cases and the InternalSettings case are examples
of a Supplement that can outlive its host, thus the wrapper hack which is owned
by the Supplementable and has a RefPtr to the object that is "supplemented".

In oilpan the wrapper becomes unnecessary since it is completely OK to hold on
to a supplement after its host is gone.


> If it outlives, we also need to care about a raw pointer in HeapSupplement
that
> points to the supplementable object.
> 
> class X : public HeapSupplement<Document> {
>   Document& m_document;  // I was assuming that this raw pointer is safe
because
> the Document and X die together. We have this kind of raw pointer in this CL.
> };

You are completely right and that is why the use of Document& is really not very
nice in oilpan. It should rather just be a strongly traced Member<Document> and
then it would be immediately clear that it the two live and die together.

[haraken, 2014-04-30 12:29:52]

On 2014/04/30 12:23:22, zerny-chromium wrote:
> > > In general, a HeapSupplement can outlive the object it is a supplement to.
> > 
> > In what situation do we want to let a supplement object outlive the
> > supplementable object?
> 
> All of the RefCountedSupplement cases and the InternalSettings case are
examples
> of a Supplement that can outlive its host, thus the wrapper hack which is
owned
> by the Supplementable and has a RefPtr to the object that is "supplemented".
> 
> In oilpan the wrapper becomes unnecessary since it is completely OK to hold on
> to a supplement after its host is gone.
> 
> 
> > If it outlives, we also need to care about a raw pointer in HeapSupplement
> that
> > points to the supplementable object.
> > 
> > class X : public HeapSupplement<Document> {
> >   Document& m_document;  // I was assuming that this raw pointer is safe
> because
> > the Document and X die together. We have this kind of raw pointer in this
CL.
> > };
> 
> You are completely right and that is why the use of Document& is really not
very
> nice in oilpan. It should rather just be a strongly traced Member<Document>
and
> then it would be immediately clear that it the two live and die together.

Thanks for the clarification! Changing it to a Member<Document> in the near
future sounds reasonable to me.

[zerny-chromium, 2014-04-30 12:38:02]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-04-30 12:38:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zerny@chromium.org/255983003/100001

[commit-bot: I haz the power, 2014-04-30 13:42:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-30 13:42:55]

Try jobs failed on following builders:
  win_blink_compile_dbg on tryserver.blink

[zerny-chromium, 2014-04-30 14:10:08]

The CQ bit was checked by zerny@chromium.org

[commit-bot: I haz the power, 2014-04-30 14:10:30]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zerny@chromium.org/255983003/120001

[commit-bot: I haz the power, 2014-04-30 15:14:56]

Change committed as 172990