833797 - Gecko - runnables will leak if NS_DispatchToMainThread() fails

Status: RESOLVED WONTFIX

(Core :: XPCOM, defect)

Description

[satender]

There are quite a few places in gecko code where NS_DispatchToMainThread is instantiating a new object on the heap, without the nsRefPtr protection. This will cause memory leaks whenever NS_DispatchToMainThread will fail. 


The fix will involve wrapping each of the new'd object in nsRefPtr.

The areas of code affected are under gecko in WebSockets, radio, sctp, HAL, Radio Sensor, media, gps, volume control.
Based on the grep output here are the affected 53 files under ~/b2g/B2G/gecko directory:

./content/base/src/WebSocket.cpp:      NS_DispatchToMainThread(new CallDispatchConnectionCloseEvents(this),
./content/media/nsBuiltinDecoderStateMachine.cpp:    NS_DispatchToMainThread(new nsDecoderDisposeEvent(mDecoder.forget(),
./dom/bluetooth/linux/BluetoothDBusService.cpp:    NS_DispatchToMainThread(new PrepareProfileManagersRunnable());
./dom/bluetooth/linux/BluetoothDBusService.cpp:  NS_DispatchToMainThread(new ShutdownProfileManagersRunnable());
./dom/media/MediaManager.cpp:      NS_DispatchToMainThread(new ErrorCallbackRunnable(
./dom/media/MediaManager.cpp:      NS_DispatchToMainThread(new ErrorCallbackRunnable(
./dom/media/MediaManager.cpp:      NS_DispatchToMainThread(new ErrorCallbackRunnable(
./dom/media/MediaManager.cpp:        NS_DispatchToMainThread(new ErrorCallbackRunnable(
./dom/media/MediaManager.cpp:        NS_DispatchToMainThread(new ErrorCallbackRunnable(
./dom/media/MediaManager.cpp:      NS_DispatchToMainThread(new ErrorCallbackRunnable(
./dom/media/MediaManager.cpp:    NS_DispatchToMainThread(new GetUserMediaStreamRunnable(
./dom/media/MediaManager.cpp:      NS_DispatchToMainThread(new ErrorCallbackRunnable(
./dom/media/MediaManager.cpp:    NS_DispatchToMainThread(new SuccessCallbackRunnable(
./dom/media/MediaManager.cpp:    NS_DispatchToMainThread(new DeviceSuccessCallbackRunnable(
./dom/system/gonk/GonkGPSGeolocationProvider.cpp:  NS_DispatchToMainThread(new UpdateLocationEvent(somewhere));
./dom/system/gonk/GonkGPSGeolocationProvider.cpp:  NS_DispatchToMainThread(new UpdateCapabilitiesEvent(capabilities));
./dom/system/gonk/GonkGPSGeolocationProvider.cpp:  NS_DispatchToMainThread(new AGPSStatusEvent(status->status));
./dom/system/gonk/GonkGPSGeolocationProvider.cpp:  NS_DispatchToMainThread(new RequestSetIDEvent(flags));
./dom/system/gonk/nsVolumeService.cpp:  NS_DispatchToMainThread(new UpdateVolumeRunnable(this, aVolume));
./dom/system/gonk/VolumeServiceTest.cpp:  NS_DispatchToMainThread(new InitVolumeServiceTestIO());
./gfx/layers/opengl/LayerManagerOGL.cpp:    NS_DispatchToMainThread(new ReadDrawFPSPref());
./gfx/thebes/gfxReusableSurfaceWrapper.cpp:    NS_DispatchToMainThread(new DeleteImageOnMainThread(mSurface));
./hal/gonk/GonkFMRadio.cpp:    NS_DispatchToMainThread(new RadioUpdate(hal::FM_RADIO_OPERATION_ENABLE,
./hal/gonk/GonkFMRadio.cpp:        NS_DispatchToMainThread(new RadioUpdate(hal::FM_RADIO_OPERATION_ENABLE,                                                 hal::FM_RADIO_OPERATION_STATUS_SUCCESS));
./hal/gonk/GonkFMRadio.cpp:        NS_DispatchToMainThread(new RadioUpdate(hal::FM_RADIO_OPERATION_SEEK,
./hal/gonk/GonkHal.cpp:      NS_DispatchToMainThread(new AlarmFiredEvent(alarmData->mGeneration));
./hal/gonk/GonkSensor.cpp:      NS_DispatchToMainThread(new SensorRunnable(buffer[i], sensors, size));
./hal/gonk/GonkSwitch.cpp:      NS_DispatchToMainThread(new SwitchEventRunnable(info.mEvent));
./hal/gonk/GonkSwitch.cpp:      NS_DispatchToMainThread(new SwitchEventRunnable(info.mEvent));
./layout/base/nsDocumentViewer.cpp:  NS_DispatchToMainThread(new nsDocumentShownDispatcher(mDocument));
./netwerk/cache/nsCacheService.cpp:        NS_DispatchToMainThread(new nsSetSmartSizeEvent(size));
./netwerk/cache/nsCacheService.cpp:    NS_DispatchToMainThread(new nsDisableOldMaxSmartSizePrefEvent());
./netwerk/cache/nsDeleteDir.cpp:  NS_DispatchToMainThread(new nsDestroyThreadEvent(mThread));
./netwerk/protocol/websocket/WebSocketChannel.cpp:        NS_DispatchToMainThread(new CallOnMessageAvailable(this, utf8Data, -1));
./netwerk/protocol/websocket/WebSocketChannel.cpp:          NS_DispatchToMainThread(new CallOnServerClose(this, mServerCloseCode,
./netwerk/protocol/websocket/WebSocketChannel.cpp:        NS_DispatchToMainThread(new CallOnMessageAvailable(this, binaryData,
./netwerk/protocol/websocket/WebSocketChannel.cpp:    NS_DispatchToMainThread(new CallOnStop(this, reason));
./netwerk/protocol/websocket/WebSocketChannel.cpp:    return NS_DispatchToMainThread(new CallOnTransportAvailable(this,
./netwerk/protocol/websocket/WebSocketChannel.cpp:          NS_DispatchToMainThread(new CallAcknowledge(this,
./netwerk/sctp/datachannel/DataChannel.cpp:    NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
./netwerk/sctp/datachannel/DataChannel.cpp:  NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
./netwerk/sctp/datachannel/DataChannel.cpp:  NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
./netwerk/sctp/datachannel/DataChannel.cpp:  NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
./netwerk/sctp/datachannel/DataChannel.cpp:          NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
./netwerk/sctp/datachannel/DataChannel.cpp:      NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
./netwerk/sctp/datachannel/DataChannel.cpp:    NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
./netwerk/sctp/datachannel/DataChannel.cpp:    NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
./netwerk/sctp/datachannel/DataChannel.cpp:            NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
./netwerk/sctp/datachannel/DataChannel.cpp:            NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
./netwerk/sctp/datachannel/DataChannel.cpp:    NS_DispatchToMainThread(new DataChannelOnMessageAvailable(
./security/manager/ssl/src/nsPSMBackgroundThread.cpp:  return NS_DispatchToMainThread(new nsRunnable());
./toolkit/crashreporter/nsExceptionHandler.cpp:      NS_DispatchToMainThread(new ReportInjectedCrash(pid));
./xpcom/base/nsConsoleService.cpp:    NS_DispatchToMainThread(new AddConsoleEnabledPrefWatcher);

Comment 1

[satender]

removing dependency on 818518, The fix is very similar to one done in 818518 for the camera specific files, but doesnt really depend on it.

Comment 2

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Why do the runnables leak if DispatchToMainThread() fails?

Please be very careful with these callsites: there are numerous locations that depend on the Dispatch(new Runnable()) idiom for thread safety.

Comment 3

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Normal COM rules require in-params to a function to already have a reference, and it's assumed that if the function doesn't call AddRef again that the in-param will eventually die once its refcount goes to 0. Doing this:

  NS_DispatchToMainThread(new nsRunnable())

doesn't manage the lifetime of the runnable at all. If the function doesn't take ownership of the runnable then it simply leaks.

This pattern can cause further problems if the function takes some temporary reference, like this:

  void Foo(nsIRunnable* aRunnable)
  {
    {
      nsCOMPtr<nsIFoo> foo = do_QueryInterface(aRunnable);
    }
    // Do something with the now-deleted aRunnable.
  }

So, we should fix this kind of thing wherever we see it.

That said, it looks like the only ways that NS_DispatchToMainThread can fail are:
  1. ThreadManager has already killed its nsThread representing the main thread (e.g. deep in shutdown)
  2. Some kind of GetService failure (probably only in deep shutdown)
  3. The thread has stopped accepting new events (mEventsAreDoomed == true, set once Shutdown() is called on the thread)
  4. OOM in the event queue.

So it's unlikely that this is a big source of leaks. Still, we should fix.

Comment 4

[Mounir Lamouri (:mounir)]

Attachment: Why not this?

I'm not sure why we need to change all callers. Can't we simply keep the pattern we are currently using and just make sure the method doesn't leak the runnable? It seems less code to modify and would easily allow us to stop doing the strong reference if it becomes useless in the future. However, I'm afraid I'm missing something big, I didn't finish my coffee yet ;)

Comment 5

[satender]

the fix certainly looks much neater in terms of limiting the change to single place. 

While maybe not directly related, one point I am not clear on is the difference between nsRefPtr and nsCOMPtr. Can they be used interchangeably while passing the same underlying object (nsRunnable in this case) around. 
I am assuming nsRefPtr is very similar to boost::shared_ptr

Definitely eager to hear the opinion of the experts on this thread.

Comment 6

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 705804 [details] [diff] [review]
Why not this?

This causes a double (threadsafe) AddRef for callers that are doing this correctly... I hope that's most callers, so this could have perf impact.

Also, only fixing NS_DispatchToMainThread() leaves NS_DispatchToCurrentThread() and nsIThread::Dispatch() vulnerable to the same problem.

Comment 7

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 705804 [details] [diff] [review]
Why not this?

Oops, I'm also not an XPCOM peer so I can't really make the decision to minus this.

Comment 8

[Benjamin Smedberg]

See also bug 803174. At this point the only cases where it *can* fail are shutdown-related, so I think we should WONTFIX this bug (shutdown leaks aren't a big deal).