Bug 56507 – DeviceOrientationClient leak

• Status: NEW
• Component: WebKit Misc.

[Joseph Pecoraro]

On chromium the lifetime of a DeviceOrientationClient seems to be
managed by the WebViewImpl, because its proxy is an OwnPtr.

On platform/mac it looks like the lifetime of a DeviceOrientationClient is
a little crazier:

  1. Allocated somewhere in client code.
  2. Setup deviceOrientationClient in PageClient and pass to Page constructor
    => controller uses it during the Page's lifetime
  3. Page is destroyed, Controller is destroyed, Controller calls
      deviceOrientationControllerDestroyed on the Client
  4. Deallocate when receiving deviceOrientationControllerDestroyed.

The controller is necessary to call deviceOrientationControllerDestroyed
and destroy the DeviceOrientationClient. However, the controller
might not even be created in Page's constructor:

  , m_deviceOrientationController(RuntimeEnabledFeatures::deviceOrientationEnabled() ? new DeviceOrientationController(this, pageClients.deviceOrientationClient) : 0)

I'm just eyeballing this but I think there is a leak if this feature is
disabled at runtime. The DeviceOrientationClient would be created:

  #if ENABLE(DEVICE_ORIENTATION)
      pageClients.deviceOrientationClient = new WebDeviceOrientationClient(self);
  #endif
      _private->page = new Page(pageClients); 

But no Controller would adopt it, and therefore no-one ever tells
this DeviceOrientationClient instance to delete itself.

The EmptyClient implementation also doesn't delete itself. That
looks expected since its a static local though.

This seems messy. It would be much nicer to RefCount the DeviceOrientationClient
like other PageClients. Chromium might complicate this because they already
use OwnPtr for their proxy.

[Joseph Pecoraro]

We could probably adopt an approach similar to chromium, and have
WebKit mac client code manage the lifetime, and pass just a point
down to the Page to use. This would make sense if the
WebDeviceOrientationClient was a singleton and we could get
away with just one. However, it seems to want to request a
WebDeviceOrientationProvider from a WebView.

Can these objects be made into a singleton?

[Steve Block]

> I'm just eyeballing this but I think there is a leak if this feature is
> disabled at runtime. The DeviceOrientationClient would be created:
> 
>   #if ENABLE(DEVICE_ORIENTATION)
>       pageClients.deviceOrientationClient = new WebDeviceOrientationClient(self);
>   #endif
>       _private->page = new Page(pageClients); 
> 
> But no Controller would adopt it, and therefore no-one ever tells
> this DeviceOrientationClient instance to delete itself.
I think you're right that there's a potential for a leak given the Mac client ownership model. However, does Mac use RuntimeEnabledFeatures?

> This seems messy. It would be much nicer to RefCount the DeviceOrientationClient
> like other PageClients.
Is this correct? I don't think that other clients are ref-counted, at least not by the Page or controller. My understanding is that clients are owned and managed by the emebedder and that the controller holds only a weak pointer.

> However, it seems to want to request a
> WebDeviceOrientationProvider from a WebView.
> 
> Can these objects be made into a singleton?
I think it would be possible to use a single client for all pages, and handle the multiplexing manually, but this doesn't match how clients are normally used.