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

Source/modules/geolocation/GeolocationController.h

 /*
  * Copyright (C) 2009 Apple Inc. All rights reserved.
  *
  * Redistribution and use in source and binary forms, with or without
  * modification, are permitted provided that the following conditions
  * are met:
  * 1. Redistributions of source code must retain the above copyright
  *    notice, this list of conditions and the following disclaimer.
  * 2. Redistributions in binary form must reproduce the above copyright
  *    notice, this list of conditions and the following disclaimer in the
@@ skipping 24 matching lines @@
 #include "wtf/RefPtr.h"
 
 namespace WebCore {
 
 class GeolocationInspectorAgent;
 class GeolocationClient;
 class GeolocationError;
 class GeolocationPosition;
 class Page;
 
-class GeolocationController FINAL : public Supplement<Page>, public PageLifecycleObserver {
+class GeolocationController FINAL : public NoBaseWillBeGarbageCollectedFinalized<GeolocationController>, public WillBeHeapSupplement<Page>, public PageLifecycleObserver {
+    WILL_BE_USING_GARBAGE_COLLECTED_MIXIN(GeolocationController);
     WTF_MAKE_NONCOPYABLE(GeolocationController);
 public:
     virtual ~GeolocationController();
 
-    static PassOwnPtr<GeolocationController> create(Page&, GeolocationClient*);
+    static PassOwnPtrWillBeRawPtr<GeolocationController> create(Page&, GeolocationClient*);
 
     void addObserver(Geolocation*, bool enableHighAccuracy);
     void removeObserver(Geolocation*);
 
     void requestPermission(Geolocation*);
     void cancelPermissionRequest(Geolocation*);
 
     void positionChanged(GeolocationPosition*);
     void errorOccurred(GeolocationError*);
 
     GeolocationPosition* lastPosition();
 
     void setClientForTest(GeolocationClient*);
     bool hasClientForTest() { return m_hasClientForTest; }
     GeolocationClient* client() { return m_client; }
 
     // Inherited from PageLifecycleObserver.
     virtual void pageVisibilityChanged() OVERRIDE;
 
     static const char* supplementName();
-    static GeolocationController* from(Page* page) { return static_cast<GeolocationController*>(Supplement<Page>::from(page, supplementName())); }
+    static GeolocationController* from(Page* page) { return static_cast<GeolocationController*>(WillBeHeapSupplement<Page>::from(page, supplementName())); }
 
-    virtual void trace(Visitor*) OVERRIDE { };
+    virtual void trace(Visitor*) OVERRIDE;
 
     virtual void willBeDestroyed() OVERRIDE;
 
 private:
     GeolocationController(Page&, GeolocationClient*);
 
     void startUpdatingIfNeeded();
     void stopUpdatingIfNeeded();
 
     GeolocationClient* m_client;

[haraken, 2014/04/30 02:41:22]

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

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

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.

[haraken, 2014/04/30 11:18:08]

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.

[haraken, 2014/04/30 11:19:58]

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

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

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

[haraken, 2014/04/30 12:02:08]

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.

     bool m_hasClientForTest;
 
-    RefPtrWillBePersistent<GeolocationPosition> m_lastPosition;
-    typedef WillBePersistentHeapHashSet<RefPtrWillBeMember<Geolocation> > ObserversSet;
+    RefPtrWillBeMember<GeolocationPosition> m_lastPosition;
+    typedef WillBeHeapHashSet<RefPtrWillBeMember<Geolocation> > ObserversSet;
     // All observers; both those requesting high accuracy and those not.
     ObserversSet m_observers;
     ObserversSet m_highAccuracyObservers;
     bool m_isClientUpdating;
     GeolocationInspectorAgent* m_inspectorAgent;

[haraken, 2014/04/30 02:41:22]

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

 };
 
 } // namespace WebCore
 
 #endif // GeolocationController_h