1073017 - (camera-parent) [Camera][Gecko] Move camera manager into the b2g parent process

Status: RESOLVED WONTFIX

(Firefox OS Graveyard :: Gaia::Camera, defect)

Description

[Mike Habicher [:mikeh] (high bugzilla latency)]

This would allow for central management of access to the camera hardware, both from a security perspective, and from an arbitration one.

Comment 1

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: WIP - implement camera service in parent process, v0.1

This builds, but is mostly boilerplate and TODOs. Completely untested, probably doesn't do anything, may recalibrate your freezer's coolness setting so all your ice cream melts.

Comment 3

[Mike Habicher [:mikeh] (high bugzilla latency)]

I've finally got the skeleton for this working, and the initial timing results are not promising: the round-trip time for a request/response seems to be ~720 to 850 ms, at least based on logcat timings.

This is likely to be too significant a delay to introduce into the Camera app start-up latency.

Comment 4

[Mike Habicher [:mikeh] (high bugzilla latency)]

Moved the IPDL from PContent to PBackground, which keeps the runnables off of the Main Thread.

It looks like this doesn't make a big difference: the initial "ping" has a RTT of ~700ms. Subsequent messages have an RTT of ~1.1ms, however, which is much better.

Investigating whether or not the RTT can be subsumed by the initial app start-up time.

Comment 5

[Mike Habicher [:mikeh] (high bugzilla latency)]

Almost all of the setup time can be subsumed into the initial app start-up time, assuming the app meets the criteria for pre-opening the camera hardware.

With all of these changes, the Camera app takes ~130ms longer to start with the additional RTT through the b2g parent.

Comment 6

[Mike Habicher [:mikeh] (high bugzilla latency)]

It looks like the "IPDL Background" thread is idle when the GetCamera() call comes in:

http://people.mozilla.org/~bgirard/cleopatra/#report=ce308887f1989e96c4374ad5d4016b32cb7c8a6c&filter=%5B%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A1944,%22end%22%3A2100%7D,%7B%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A1978,%22end%22%3A1983%7D%5D&selection=0,1,2,3,4,5,6

Not having as much success profiling the Camera app, yet.

Comment 7

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Register CameraThread so the profiler can find it, v1

It's probably worth landing this anyway so the profiler can be used on this thread.

Comment 8

[Mike Habicher [:mikeh] (high bugzilla latency)]

New profiler results:

http://people.mozilla.org/~bgirard/cleopatra/#report=e85336412f34658dc2e8f03d4a94f01b203aea99&filter=[{%22type%22%3A%22RangeSampleFilter%22,%22start%22%3A4573,%22end%22%3A5666}]&selection=0,33,1,2,3,4,5,6

All of the initial CameraService stuff happens at 4723 (+0ms), the callback into the camera stack occurs at 4983 (+260ms), and the camera hardware is connected at 5347 (+624ms).

In the parent process' Background IPDL thread, the message is received and replied to at 4925 (+202ms). While this is happening, both threads are quiescent, so it's not obvious why the RTT is asymmetric.

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8573544 [details] [diff] [review]
Register CameraThread so the profiler can find it, v1

Review of attachment 8573544 [details] [diff] [review]:
-----------------------------------------------------------------

Shouldn't this be wrapped in #ifdef MOZ_PROFILING?

Comment 10

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8573544 [details] [diff] [review]
Register CameraThread so the profiler can find it, v1

Moved to bug 1141267. (And as discussed offline, it looks like profiler_register_thread() is called from a number of places, sometimes wrapped in conditionals, but usually not.)

Comment 11

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Implement camera service in parent process, v1

https://treeherder.mozilla.org/#/jobs?repo=try&revision=04b825225c1e

Comment 12

[Mike Habicher [:mikeh] (high bugzilla latency)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d98a8b31aee0

Comment 13

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Implement camera broker service in parent process, v2

try-push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f38c3bb66a9

Comment 14

[GitHub Autolander Bot]

Attachment: [gaia] mikeaich:bug1073017 > mozilla-b2g:master

Comment 15

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8588754 [details] [review]
[gaia] mikeaich:bug1073017 > mozilla-b2g:master

DO NOT LAND (yet)

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8586121 [details] [diff] [review]
Implement camera broker service in parent process, v2

Review of attachment 8586121 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good although I may need to study the parent/child signaling a little more to make sure I actually understand it. r+ with below addressed.

::: dom/camera/CameraCommon.h
@@ +23,2 @@
>  #else
> +#define DOM_CAMERA_LOG( type, ... ) printf_stderr(__VA_ARGS__) /* mikeh */

Note to remove before checking in.

::: dom/camera/CameraService.cpp
@@ +94,5 @@
> +
> +      nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> +      if (NS_WARN_IF(!obs)) {
> +        obs->RemoveObserver(aService, NS_XPCOM_SHUTDOWN_OBSERVER_ID);
> +      }

The observer service doesn't seem to like one removing observers on shutdown (at least there are warnings about it). Are we sure this the correct thing to do?

@@ +124,5 @@
> +
> +#if defined(MOZ_B2G_CAMERA)
> +  if (!XRE_IsParentProcess()) {
> +    return CameraServiceChildProcess::Create();
> +  }

I wonder if running the camera tests via B2G desktop are broken by this. Did you try running them?

@@ +130,5 @@
> +  return new CameraService();
> +#endif
> +
> +  NS_WARNING("No platform support for cameras!");
> +  return nullptr;

Shouldn't this be part of an #else? I could see a compiler complaining about unreachable code.

@@ +181,5 @@
> +    NS_WARNING("Can't request camera access during shutdown");
> +    return NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
> +  }
> +
> +  ReentrantMonitorAutoEnter monitor(mClientLock);

Elsewhere we are careful not to hold the static mutex at the same time as the monitor. I suppose we must hold both here to ensure that ::HandleShutdownOnBackgroundThread doesn't happen between us hypothetically unlocking the static mutex and locking the monitor? I think another potential solution is to store sInShutdown in the object (i.e. mInShutdown) and protected it via the monitor instead. That may be more trouble than it is worth though :).

@@ +204,5 @@
> +    for (ClientIndex i = 0; i < mCurrentClients.Length(); ++i) {
> +      CameraServiceClient* client = mCurrentClients[i];
> +      MOZ_ASSERT(client);
> +      client->CameraReleaseRequired();
> +      mDyingClients.AppendElement(client);

I see that the reference to client in mCurrentClients won't get prematurely released indirectly through the CameraReleaseRequired callback, because that callback posts back to the current thread, but I think I would feel better if mDyingClients.AppendElement(client) happened before the callback to guarantee that won't happen. Unless there is some reason for the current ordering I am missing.

@@ +463,5 @@
> +    }
> +  }
> +
> +  rv = timer->Cancel();
> +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(rv));

Do we need to cancel if timeExceeded is true? And does the cancel succeed if so?

::: dom/camera/GonkCameraHwMgr.cpp
@@ +336,5 @@
>      return nullptr;
>    }
>  
> +  nsRefPtr<ProxyClient> proxy = new ProxyClient(client);
> +  client->SetProxyClient(proxy);

I believe we should store it as a nsWeakPtr instead of a raw pointer. In the normal case where we get the camera then we know the reference won't be destroyed until we explicitly release it. If we are denied the camera because multiple clients requested it and we aren't the last one, I believe the only strong reference in cs->mPendingClients is removed while we still hold a now dangling pointer here which we will (at least) use in ::Close via ~GonkCameraClient...

::: dom/camera/TestGonkCameraHardware.cpp
@@ +86,5 @@
>      if (!NS_WARN_IF(!event)) {
>        nsString state;
>  
>        event->GetNewState(state);
> +      printf_stderr("[mikeh] state = '%s'\n", NS_ConvertUTF16toUTF8(state).get());

Remove.

::: dom/camera/ipc/CameraServiceChildProcess.cpp
@@ +147,5 @@
> +      }
> +      // If we don't yet have a valid 'sCameraChild' because we're waiting for
> +      // the PBackground thread callback, pending requests will be handled in
> +      // CameraServiceChildProcess::CameraActorCreated().
> +      break;

The initial state being kAccessDenied is sort of confusing at first glance. We haven't been denied...yet :).

@@ +156,5 @@
> +      break;
> +
> +    case kAccessRequested:
> +      // wait for the response from the parent process
> +      break;

We don't appear to ever set kAccessRequested?

Comment 17

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Andrew Osmond [:aosmond] from comment #16)

> Looks good although I may need to study the parent/child signalling a little
> more to make sure I actually understand it. r+ with below addressed.
> 
> ::: dom/camera/CameraService.cpp
> @@ +94,5 @@
> > +
> > +      nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
> > +      if (NS_WARN_IF(!obs)) {
> > +        obs->RemoveObserver(aService, NS_XPCOM_SHUTDOWN_OBSERVER_ID);
> > +      }
> 
> The observer service doesn't seem to like one removing observers on shutdown
> (at least there are warnings about it). Are we sure this the correct thing
> to do?

The only evidence I have that it is:

https://dxr.mozilla.org/mozilla-central/search?q=%22RemoveObserver%28this%2C+NS_XPCOM_SHUTDOWN_OBSERVER_ID%29%3B%22&case=true

> @@ +124,5 @@
> > +
> > +#if defined(MOZ_B2G_CAMERA)
> > +  if (!XRE_IsParentProcess()) {
> > +    return CameraServiceChildProcess::Create();
> > +  }
> 
> I wonder if running the camera tests via B2G desktop are broken by this. Did
> you try running them?

Are the B2G desktop tests run as part of CI? If so, the CI build passed the last time I ran it (which was, admittedly, a while ago -- I'll re-run it when I update the patch).

> @@ +181,5 @@
> > +    NS_WARNING("Can't request camera access during shutdown");
> > +    return NS_ERROR_ILLEGAL_DURING_SHUTDOWN;
> > +  }
> > +
> > +  ReentrantMonitorAutoEnter monitor(mClientLock);
> 
> Elsewhere we are careful not to hold the static mutex at the same time as
> the monitor. I suppose we must hold both here to ensure that
> ::HandleShutdownOnBackgroundThread doesn't happen between us hypothetically
> unlocking the static mutex and locking the monitor? I think another
> potential solution is to store sInShutdown in the object (i.e. mInShutdown)
> and protected it via the monitor instead. That may be more trouble than it
> is worth though :).

'sInShutdown' can't be part of the object because it has to exist even if the singleton has been destroyed. 

> @@ +204,5 @@
> > +    for (ClientIndex i = 0; i < mCurrentClients.Length(); ++i) {
> > +      CameraServiceClient* client = mCurrentClients[i];
> > +      MOZ_ASSERT(client);
> > +      client->CameraReleaseRequired();
> > +      mDyingClients.AppendElement(client);
> 
> I see that the reference to client in mCurrentClients won't get prematurely
> released indirectly through the CameraReleaseRequired callback, because that
> callback posts back to the current thread, but I think I would feel better
> if mDyingClients.AppendElement(client) happened before the callback to
> guarantee that won't happen. Unless there is some reason for the current
> ordering I am missing.

I'm going to tweak this. I'll poke you for a re-review.

> @@ +463,5 @@
> > +    }
> > +  }
> > +
> > +  rv = timer->Cancel();
> > +  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(rv));
> 
> Do we need to cancel if timeExceeded is true? And does the cancel succeed if
> so?

MOZ_ALWAYS_TRUE() is only defined for DEBUG builds, and this should only fail if something goes horribly wrong during shutdown.

> ::: dom/camera/GonkCameraHwMgr.cpp
> @@ +336,5 @@
> >      return nullptr;
> >    }
> >  
> > +  nsRefPtr<ProxyClient> proxy = new ProxyClient(client);
> > +  client->SetProxyClient(proxy);
> 
> I believe we should store it as a nsWeakPtr instead of a raw pointer. In the
> normal case where we get the camera then we know the reference won't be
> destroyed until we explicitly release it. If we are denied the camera
> because multiple clients requested it and we aren't the last one, I believe
> the only strong reference in cs->mPendingClients is removed while we still
> hold a now dangling pointer here which we will (at least) use in ::Close via
> ~GonkCameraClient...

I decided not to make this an nsWeakPtr for simplicity. The GonkCameraClient never derefs the pointer, and only uses it as a handle for deregistration. Let me think about this a bit more.

> ::: dom/camera/ipc/CameraServiceChildProcess.cpp
> @@ +147,5 @@
> > +      }
> > +      // If we don't yet have a valid 'sCameraChild' because we're waiting for
> > +      // the PBackground thread callback, pending requests will be handled in
> > +      // CameraServiceChildProcess::CameraActorCreated().
> > +      break;
> 
> The initial state being kAccessDenied is sort of confusing at first glance.
> We haven't been denied...yet :).

Well, it's the default state.

> @@ +156,5 @@
> > +      break;
> > +
> > +    case kAccessRequested:
> > +      // wait for the response from the parent process
> > +      break;
> 
> We don't appear to ever set kAccessRequested?

Huh. Good catch.

Comment 18

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Implement camera broker service in parent process, v2.1

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=91f33f213009

Comment 19

[Mike Habicher [:mikeh] (high bugzilla latency)]

try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5cd1dae1062

Comment 20

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Implement camera broker service in parent process, v2.2

tries: https://treeherder.mozilla.org/#/jobs?repo=try&revision=e5cd1dae1062
       https://treeherder.mozilla.org/#/jobs?repo=try&revision=a303a3e1bf93

Comment 21

[Mike Habicher [:mikeh] (high bugzilla latency)]

Comment on attachment 8590500 [details] [diff] [review]
Implement camera broker service in parent process, v2.2

Re-r? incorporating most of your feedback. I'll remove the debugging prints before landing, and rename GonkCameraHwMgr.* to GonkCameraClient.* in a subsequent, rename-only, patch.

Comment 22

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8590500 [details] [diff] [review]
Implement camera broker service in parent process, v2.2

Review of attachment 8590500 [details] [diff] [review]:
-----------------------------------------------------------------

Compared the delta, LGTM.

Comment 23

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: 1073017_ipc.patch

Comment 24

[Gian-Carlo Pascutto [:gcp]]

It kind of sucks B2G isn't using WebRTC for it's camera needs: https://bugzilla.mozilla.org/show_bug.cgi?id=1104616 :-/

Comment 25

[Mike Habicher [:mikeh] (high bugzilla latency)]

(In reply to Gian-Carlo Pascutto [:gcp] from comment #24)

> It kind of sucks B2G isn't using WebRTC for it's camera needs:
> https://bugzilla.mozilla.org/show_bug.cgi?id=1104616 :-/

It likely will be in the future, with a healthy extension of ImageCapture, but for now we needed a way to mediate between different processes trying to open the camera. (I've been thinking that before my patch lands, it should probably be s/Camera*/GonkCamera*/, since it's very Gonk-specific.)

Does the patch in bug 1104616 work with b2g/Gonk? If so, we should examine merging (at least) the broker functionality.

Comment 26

[Gian-Carlo Pascutto [:gcp]]

>Does the patch in bug 1104616 work with b2g/Gonk? If so, we should examine merging (at least) the broker 
>functionality.

Probably not. Gonk has it's own MediaEngineGonkVideoSource whereas everything else is using the generic MediaEngineWebRTCVideoSource. They seem to be totally different. Bug 1104616 proxies what the latter needs.

Comment 27

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

Mike, can you give me a little overview of the architecture here? I see lots of locks so I assume you want the parent-side objects accessible from multiple threads? Why? And what's the threading like on the child-side?

Comment 28

[Mike Habicher [:mikeh] (high bugzilla latency)]

Ben, the Parent/Child classes are set up in such a way as to expose the CameraService singleton identically whether or not the the camera access request occurs in a child process or the parent process: GonkCameraClient is a subclass of CameraServiceClient, as are the Parent sides of the IPC channels. Furthermore, the instances are organized in a (very shallow) tree, with IPC channels between then:

                                                             /----GonkCameraClient
                     /-------[Child] CameraService(ChildProcess) 
  CameraService--[Parent]                                    \----GonkCameraClient
         |     \-[Parent]
         |           \-------[Child] CameraService(ChildProcess)
         |                                                   \----GonkCameraClient
         +-------GonkCameraClient

With this, if two GonkCameraClients in the child process compete for access to the hardware, the decision can be made locally without a round-trip through the parent process' CameraService. Conversely, if another process requests camera access, the parent process' CameraService only needs to send one message to each child process, and the local CameraService instance will notify any clients of the need to release the hardware.

The threading model is a complicated because the AOSP/CAF camera library uses multiple threads. Our part in Gecko mostly dispatches calls to the camera library from a single worker, but the library spins up a number of internal threads on which different callbacks, including notification of the loss of connection to the mediaserver (the process that actually holds the camera hardware instance), can run.

Comment 29

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

Comment on attachment 8591843 [details] [diff] [review]
1073017_ipc.patch

Review of attachment 8591843 [details] [diff] [review]:
-----------------------------------------------------------------

The IPC stuff looks mostly fine to me, though I have a few problems/suggestions below.

::: dom/camera/ipc/CameraParent.cpp
@@ +36,5 @@
> +CameraParent::CameraParent()
> +  : mShutdownState(Running)
> +{
> +  DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);
> +  MOZ_COUNT_CTOR(CameraParent);

You don't need CTOR/DTOR logging for refcounted classes (as long as you use our refcount macros)

@@ +79,5 @@
> +
> +  if (mService) {
> +    mService->DeregisterClient(this);
> +#ifdef DEBUG
> +    mService = nullptr;

Hm, why only do this in DEBUG builds? It means you'll hold a ref to the service longer in opt builds, and that seems undesirable.

@@ +93,5 @@
> +CameraParent::RecvRequestCameraAccess()
> +{
> +  DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);
> +  MOZ_ASSERT(mService);
> +  mService->RequestCameraAccess(this);

Shouldn't you prevent this call if a misbehaving child sends this message multiple times? And what about the child sends this message partway through one of your shutdown sequences? Same goes for basically all the Recv* functions below. If you can detect anything out of the ordinary then you should return false and kill the child.

::: dom/camera/ipc/CameraChild.cpp
@@ +12,5 @@
> +
> +using namespace mozilla;
> +using namespace mozilla::camera;
> +
> +StaticRefPtr<CameraServiceChildProcess> sCameraService;

Nit: "s" prefix is for static members. Use "g" for global here.

@@ +22,5 @@
> +{
> +  DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);
> +  MOZ_COUNT_CTOR(CameraChild);
> +  MOZ_ASSERT(!sCameraService);
> +  MOZ_ASSERT(aCameraService);

Ok, so there's only one CameraChild at a time in each process, right? So why do you also need a global for the CameraServiceChildProcess? Couldn't you just make CameraChild hold a nsRefPtr<CameraServiceChildProcess> member? Then you wouldn't need the ClearOnShutdown bit below.

@@ +38,5 @@
> +    }
> +  };
> +
> +  sCameraService = aCameraService;
> +  NS_DispatchToMainThread(new Delegate());

Hrm, this is a bit weird. Wouldn't it make more sense to do this kind of thing in CameraServiceChildProcess itself? And just use a raw pointer for your global?

::: dom/camera/ipc/CameraChild.h
@@ +12,5 @@
> +namespace mozilla {
> +namespace camera {
> +
> +class CameraChildCreateCallback;
> +class CameraServiceChildProcess;

Nit: Make these nested classes of CameraChild? Then you wouldn't need the friend decls either.

@@ +14,5 @@
> +
> +class CameraChildCreateCallback;
> +class CameraServiceChildProcess;
> +
> +class CameraChild : public PCameraChild

Nit: Mark final, then s/protected/private/ below.

::: ipc/glue/BackgroundParentImpl.h
@@ +122,5 @@
>    DeallocPCacheStreamControlParent(dom::cache::PCacheStreamControlParent* aActor) override;
> +
> +  virtual PCameraParent*
> +  AllocPCameraParent() override;
> +  

Nit: extra whitespace here.

::: ipc/glue/BackgroundParentImpl.cpp
@@ +439,5 @@
> +{
> +  AssertIsInMainProcess();
> +  AssertIsOnBackgroundThread();
> +
> +  return mozilla::camera::CameraParent::Create();

Hrm, you want to ensure that there's only ever one of these per-process, right ? I think you should use BackgroundParent::GetRawContentParentForComparison() in a hash set here to track that you only have one per ContentParent.

@@ +454,5 @@
> +  return true;
> +}
> +
> +bool
> +BackgroundParentImpl::RecvPCameraConstructor(camera::PCameraParent* aActor)

You could do the CameraService check in Alloc and then pass that service pointer in to the CameraParent constructor. Then you wouldn't need to override RecvPCameraConstructor at all.

::: dom/camera/ipc/PCamera.ipdl
@@ +8,5 @@
> +
> +namespace mozilla {
> +namespace camera {
> +
> +protocol PCamera

The descriptions of each message make sense but it would be good to comment on the sequences that you expect. BeginShutdown->__delete__? DisableNotificationsForShutdown->NotificationsDisabledForShutdown->__delete__?

::: dom/camera/ipc/CameraParent.h
@@ +19,5 @@
> +
> +namespace camera {
> +
> +class CameraParent : public PCameraParent
> +                   , public CameraServiceClient

Nit: Mark as 'final', and s/protected/private below.

@@ +39,5 @@
> +
> +  nsRefPtr<CameraService> mService;
> +  ShutdownState mShutdownState;
> +
> +  static CameraParent* Create();

Make sure you indicate here that you're returning a pointer that carries a reference. Usually we do this by making it return already_AddRefed<>, and then you do the refcount gymnastics in BackgroundParentImpl.

Comment 30

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: Implement camera broker service in parent process, v2.3, r=aosmond

Carrying forward r+ from aosmond; bent, I'll post a breakout of the IPC stuff shortly. This should incorporate all of your suggested changes.

I can't seem to push to try anymore. :)

Comment 31

[Mike Habicher [:mikeh] (high bugzilla latency)]

Attachment: 1073017_ipc.patch

IPC breakout -- includes review feedback.

Comment 32

[Mike Habicher [:mikeh] (high bugzilla latency)]

Andrew, would your mind pushing attachment 8595603 [details] [diff] [review] to try for me?

Comment 33

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2e65c2a391b

Comment 34

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Implement camera broker service in parent process, v2.4, r=aosmond

Refresh against tree, fix constructor which should be explicit.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cb3da7e38e7

Comment 35

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1073017_ipc_2015-06-02.patch

Refresh against tree.

Comment 36

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

Comment on attachment 8614462 [details] [diff] [review]
bug1073017_ipc_2015-06-02.patch

Review of attachment 8614462 [details] [diff] [review]:
-----------------------------------------------------------------

Class structure is a bit odd, and looks like some remnants of previous versions are still laying around...

I think what's happening is that CameraServiceChildProcess and CameraChild are hard to separate, so why not ditch CameraServiceChildProcess entirely and make CameraChild inherit PCameraChild *and* CameraService (and maybe also CameraServiceClient)? Wouldn't that simplify lots of things?

::: dom/camera/ipc/CameraChild.cpp
@@ +12,5 @@
> +
> +using namespace mozilla;
> +using namespace mozilla::camera;
> +
> +// ********** CameraChild

Nit: Not really useful in my opinion

@@ +30,5 @@
> +  MOZ_COUNT_DTOR(CameraChild);
> +  MOZ_ASSERT(mCameraService);
> +  MOZ_ASSERT(mShutdownState == Dead);
> +
> +  mCameraService = nullptr;

This isn't needed, you're about to do it automatically

@@ +65,5 @@
> +{
> +  DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);
> +  MOZ_ASSERT(mCameraService);
> +
> +  if (mCameraService) {

This was just asserted, no need to check here. Same pattern repeats in several other methods below.

@@ +84,5 @@
> +  return true;
> +}
> +
> +bool
> +CameraChild::RecvCameraReleaseRequired()

No DOM_CAMERA_LOGT here?

::: dom/camera/ipc/CameraChild.h
@@ +14,5 @@
> +
> +class CameraChild final : public PCameraChild
> +{
> +public:
> +  class CameraServiceChildProcess;

Does this need to be public?

@@ +24,5 @@
> +  {
> +    Running = 0,
> +    SentStopNotifying,
> +    ReceivedNotificationsStoppedForShutdown,
> +    Dead

Make |Dead| be DEBUG-only? It's only touched in assertions and DEBUG-only code.

@@ +27,5 @@
> +    ReceivedNotificationsStoppedForShutdown,
> +    Dead
> +  };
> +
> +  ShutdownState mShutdownState;

Nit: members shouldn't be mixed in among methods, they should all be together.

@@ +45,5 @@
> +
> +  virtual bool RecvCameraReleaseRequired() override;
> +
> +  nsRefPtr<CameraServiceChildProcess> mCameraService;
> +  nsCOMPtr<nsIThread> mCameraThread;

This appears to be unused.

::: dom/camera/ipc/CameraParent.cpp
@@ +10,5 @@
> +
> +using namespace mozilla;
> +using namespace mozilla::camera;
> +
> +// ********** CameraParent

Nit: Not useful in my opinion

@@ +16,5 @@
> +/* static */ CameraParent*
> +CameraParent::Create(CameraService* aService)
> +{
> +  // We AddRef the new parent here because the IPC framework only holds
> +  // a raw pointer to its actors, but expects the instance to exist until

The normal way to do this is to let the caller worry about refcounting (just like most XPCOM stuff).

  PCameraParent*
  BackgroundParentImpl::AllocPCameraParent(...)
  {
    nsRefPtr<CameraParent> actor = CameraParent::Create(...);
    // ...
    return actor.forget().take();
  }

  bool
  BackgroundParentImpl::DeallocPCameraParent(PCameraParent* aActor)
  {
    nsRefPtr<CameraParent> actor =
      dont_AddRef(static_cast<CameraParent*>(aActor));
    // ...
    return true;
  }

Then this function doesn't need a huge comment explaining ownership. It would be even better to also change the signature here to match XPCOM more:

  // static
  already_AddRefed<CameraParent>
  CameraParent::Create(CameraService* aService)

That way it's clear that any pointer returned carries a reference.

@@ +32,5 @@
> +/* static */ void
> +CameraParent::Destroy(CameraParent* aParent)
> +{
> +  // Explicitly Release the parent when the IPC framework tells us to.
> +  // This *should* destroy the parent instance.

This isn't true, it just means IPC is done with the actor. When it actually gets destroyed is determined by its refcount.

@@ +42,5 @@
> +  : mService(aService)
> +  , mShutdownState(Running)
> +  , mAccessStatus(kAccessDenied)
> +{
> +  DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);

MOZ_ASSERT(mService) too?

@@ +72,5 @@
> +
> +  if (mService) {
> +    mService->DeregisterClient(this);
> +#ifdef DEBUG
> +    mService = nullptr;

I don't think this should be DEBUG-only. This is a refcounted object so there's no guarantee that ActorDestroy will soon be followed by actual destruction, so you'll hold the service alive longer in non-DEBUG builds than in DEBUG builds. That could cause funny bugs or cycles that you'd miss in testing.

::: dom/camera/ipc/CameraParent.h
@@ +13,5 @@
> +
> +namespace mozilla {
> +
> +namespace ipc {
> +  class BackgroundParentImpl;

Nit: no indent needed

@@ +19,5 @@
> +
> +namespace camera {
> +
> +class CameraParent final : public PCameraParent
> +                         , public CameraServiceClient

CameraServiceClient has a threadsafe refcount (which is maybe needed, I don't know) so you don't get any of the builtin threadsafety checks. IPDL actors, though, are not threadsafe, and must never be used on any but the thread on which they were created. I think it would be very smart to add a NS_DECL_OWNINGTHREAD here, and make sure every method that touches the CameraParentactor has a NS_ASSERT_OWNINGTHREAD call in it. Otherwise you risk chaos!

Nit: style guide for classes doesn't agree with this (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style#Classes)

@@ +27,5 @@
> +public:
> +  void BeginShutdown();
> +
> +private:
> +  nsRefPtr<CameraService> mService;

Nit: members before methods is unusual...

@@ +35,5 @@
> +    Running = 0,
> +    SentBeginShutdown,
> +    ReceivedStopNotifying,
> +    SentNotificationsStoppedForShutdown,
> +    Dead

Make |Dead| be DEBUG-only? It's only touched in assertions and DEBUG-only code.

@@ +39,5 @@
> +    Dead
> +  };
> +  ShutdownState mShutdownState;
> +
> +  enum AccessStatus {

You already inherit this from CameraServiceClient, don't redefine it.

::: dom/camera/ipc/CameraServiceChildProcess.cpp
@@ +16,5 @@
> +using namespace mozilla::dom;
> +using namespace mozilla::ipc;
> +using namespace mozilla::camera;
> +
> +static CameraChild* sCameraChild;

Nit: "s" prefix is for static members... I'd either make this a real static member of CameraServiceChildProcess or rename to use "g" for global

@@ +24,5 @@
> +
> +class CameraChild::CameraChildCreateCallback final
> +  : public nsIIPCBackgroundChildCreateCallback
> +{
> +  NS_DECL_THREADSAFE_ISUPPORTS

All PBackground stuff is guaranteed to only be used on one thread, so this doesn't need to be threadsafe.

@@ +63,5 @@
> +    DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__);
> +    MOZ_CRASH("Failed to create CameraChild actor!");
> +  }
> +  
> +  nsRefPtr<CameraServiceChildProcess> mChildProcess;

Nit: Seems like this should be called "mCameraService"

@@ +76,5 @@
> +  DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__);
> +  MOZ_ASSERT(!XRE_IsParentProcess());
> +  MOZ_ASSERT(!sCameraChild);
> +
> +  CameraServiceChildProcess* service = new CameraServiceChildProcess();

Hm, what guards against this being called multiple times (and then creating multiple instances)?

@@ +99,5 @@
> +  DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);
> +  MOZ_ASSERT(!XRE_IsParentProcess());
> +  MOZ_ASSERT(!sCameraChild);
> +
> +  sCameraChild = aCameraChild;

It's weird that this is set when the CameraChild is created, but then it gets unset in CameraServiceChildProcess's destructor...

@@ +137,5 @@
> +  DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);
> +  MOZ_ASSERT(!XRE_IsParentProcess());
> +  PROFILER_MARKER(__func__);
> +
> +  ReentrantMonitorAutoEnter lock(mClientLock);

Why are you locking here? We're not touching any of the members that are supposed to be protected by it, and you're calling out to IPDL code with it held...

Are you thinking that mAccessStatus should be protected by the lock? I don't think it is possible for it to be written or read on any thread other than the single thread where sCameraChild is valid.

@@ +141,5 @@
> +  ReentrantMonitorAutoEnter lock(mClientLock);
> +  switch (mAccessStatus) {
> +    case kAccessDenied:
> +      if (sCameraChild) {
> +        if (sCameraChild->SendRequestCameraAccess()) {

This is infallible, no need to check the return value.

@@ +161,5 @@
> +      break;
> +
> +    default:
> +      NS_WARNING("Unhandled mAccessStatus value!");
> +      MOZ_ASSERT(false, "Unhandled mAccessStatus value!");

These two should just be combined into MOZ_CRASH.

@@ +172,5 @@
> +{
> +  DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);
> +  MOZ_ASSERT(!XRE_IsParentProcess());
> +
> +  ReentrantMonitorAutoEnter lock(mClientLock);

Why?

@@ +183,5 @@
> +{
> +  DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);
> +  MOZ_ASSERT(!XRE_IsParentProcess());
> +
> +  ReentrantMonitorAutoEnter lock(mClientLock);

Why?

@@ +194,5 @@
> +{
> +  DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);
> +  MOZ_ASSERT(!XRE_IsParentProcess());
> +
> +  ReentrantMonitorAutoEnter lock(mClientLock);

Why?

@@ +200,5 @@
> +  CloseCurrentClients();
> +  RejectPendingClients();
> +
> +  // We need to call this now in the case where we have no current or
> +  // pending clients.

You just closed current clients and rejected pending clients, so "in the case where" doesn't make any sense.

@@ +214,5 @@
> +
> +  CameraService::DeregisterClient(aClient);
> +
> +  // Only deregister with the parent process if our process' current access
> +  // is denied. Otherwise 

Otherwise... what?

@@ +234,5 @@
> +  // Only deregister with the parent process when we have no more clients.
> +  if (mDyingClients.IsEmpty() &&
> +      mCurrentClients.IsEmpty() &&
> +      mPendingClients.IsEmpty()) {
> +    unused << sCameraChild->SendDeregisterClient();

Holding a lock while calling out to other code that you don't control is a bad idea. Can you drop the lock before making this call?

::: dom/camera/ipc/CameraServiceChildProcess.h
@@ +12,5 @@
> +
> +namespace mozilla {
> +namespace camera {
> +
> +class CameraChild;

This doesn't need to be forward-declared, at least not if CameraServiceChildProcess remains an inner class.

@@ +13,5 @@
> +namespace mozilla {
> +namespace camera {
> +
> +class CameraChild;
> +class CameraChildCreateCallback;

This class doesn't exist. CameraChild::CameraChildCreateCallback does, but that doesn't need to be forward-declared here.

@@ +16,5 @@
> +class CameraChild;
> +class CameraChildCreateCallback;
> +class CameraReplyRunnable;
> +
> +class CameraChild::CameraServiceChildProcess

Weird, why is this an inner class of CameraChild?

@@ +17,5 @@
> +class CameraChildCreateCallback;
> +class CameraReplyRunnable;
> +
> +class CameraChild::CameraServiceChildProcess
> +  : public CameraService

CameraService has a threadsafe refcount (which is maybe needed, I don't know) so you don't get any of the builtin threadsafety checks. IPDL actors, though, are not threadsafe, and must never be used on any but the thread on which they were created. I think it would be very smart to add a NS_DECL_OWNINGTHREAD here, and make sure every method that touches the sCameraChild actor has a NS_ASSERT_OWNINGTHREAD call in it. Otherwise you risk chaos!

@@ +20,5 @@
> +class CameraChild::CameraServiceChildProcess
> +  : public CameraService
> +{
> +  friend class mozilla::camera::CameraChild;
> +  friend class mozilla::camera::CameraChildCreateCallback;

This class doesn't exist

@@ +35,5 @@
> +  virtual void ResolvePendingClients() override;
> +
> +  virtual void CameraServiceAccessAllowed();
> +  virtual void CameraServiceAccessDenied();
> +  virtual void CameraServiceReleaseRequired();

Why are these virtual? Maybe you should make this inherit CameraServiceClient since this service is both a service and a client.

@@ +42,5 @@
> +
> +  enum AccessStatus {
> +    kAccessDenied,
> +    kAccessRequested,
> +    kAccessAllowed

Duplicating this from CameraServiceClient seems wrong...

@@ +44,5 @@
> +    kAccessDenied,
> +    kAccessRequested,
> +    kAccessAllowed
> +  };
> +  AccessStatus mAccessStatus;

Nit: Members shouldn't be mixed in with methods.

::: dom/camera/ipc/PCamera.ipdl
@@ +36,5 @@
> +
> +  /**
> +   * Sent to inform the child process that it must close any open
> +   * camera hardware instances, remove their listeners, and respond
> +   * with a call to StopNotifying() when all listeners are closed.

Nit: "StopNotifying" doesn't exist, this should be "DeregisterClient" I think (see below for better name maybe)

@@ +42,5 @@
> +  CameraReleaseRequired();
> +
> +  /**
> +   * Sent to inform the child process that it has been granted access
> +   * to a camera hardware instance.

Nit: "in response to a RequestCameraAccess() call"

@@ +69,5 @@
> +   * Sent when the child no longer needs to receive any messages
> +   * from the parent. This implies that no one in the child process
> +   * is holding an instances of the camera hardware open.
> +   */
> +  DeregisterClient();

Nit: What about making this "CameraReleased" to fit better with "CameraReleaseRequired"?

::: ipc/glue/BackgroundParentImpl.cpp
@@ +488,5 @@
> +  AssertIsInMainProcess();
> +  AssertIsOnBackgroundThread();
> +
> +  const void* self =
> +    reinterpret_cast<const void*>(BackgroundParent::GetRawContentParentForComparison(this));

Changing the key type of |mCameraActorParents| should make this not need a cast.

@@ +520,5 @@
> +  MOZ_ASSERT(self);
> +
> +  if (!mCameraActorParents.GetEntry(self)) {
> +    MOZ_ASSERT(false, "Content process missing in camera actors table!");
> +    return false;

I don't think this is ok... First, if you return early here you'll kill the child process but leak memory in the parent (bad!). Second, you should be able to assert that |GetEntry| will always return true.

So just:

  MOZ_ASSERT(mCameraActorParents.GetEntry(self));
  mCameraActorParents.RemoveEntry(self);

  camera::CameraParent::Destroy(static_cast<camera::CameraParent*>(aActor));
  return true;

::: ipc/glue/BackgroundParentImpl.h
@@ +134,5 @@
> +
> +  // Used to ensure that CameraParents are limited to one per child process.
> +  // void pointers should be able to hold intptr_t values, and we don't
> +  // really care about the stored values -- just whether or not they exist.
> +  nsTHashtable<nsVoidPtrHashKey> mCameraActorParents;

I'd use nsUint64HashKey so you don't have to do casting anywhere.

Nit: members shouldn't be mixed in among methods. Put at the top of the class in the implicit 'private' section?

Comment 37

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Implement camera broker service in parent process, v3, r=aosmond

Rebase, incorporate review feedback, change slightly to seize camera first to reduce app startup time.

Comment 38

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1073017_refactor_2015-07-01.patch

Delta for review feedback after rebasing.

Comment 39

[Andrew Osmond [:aosmond] (he/him)]

Attachment: bug1073017_ipc_2015-07-01.patch

Breakout for IPC related changes after rebasing and incorporating review feedback. See also attachment 8628238 [details] [diff] [review].

> @@ +24,5 @@
> > +
> > +class CameraChild::CameraChildCreateCallback final
> > +  : public nsIIPCBackgroundChildCreateCallback
> > +{
> > +  NS_DECL_THREADSAFE_ISUPPORTS
> 
> All PBackground stuff is guaranteed to only be used on one thread, so this
> doesn't need to be threadsafe.

I was commenting to justify why I did not do this (there is some main thread activity in ChildImpl::GetOrCreateForCurrentThread) but it looks like it doesn't touch that pointer off the current thread in the end. I will switch that as part of a conditional r+ if there are no other major issues.

Comment 40

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

Comment on attachment 8628238 [details] [diff] [review]
bug1073017_refactor_2015-07-01.patch

Review of attachment 8628238 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, this is almost right, but a few more issues to fix:

::: dom/camera/GonkCameraHwMgr.cpp
@@ +270,5 @@
>    DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);
>    PROFILER_MARKER("GonkCameraClient::OnCameraAccessAllowed():ENTER");
>  
> +  // Already have the camera
> +  if (mCamera.get()) {

Nit: This |.get()| (and the other below) shouldn't be needed.

::: dom/camera/ipc/CameraChild.cpp
@@ +25,5 @@
>  
> +class CameraChild::CameraChildCreateCallback final
> +  : public nsIIPCBackgroundChildCreateCallback
> +{
> +  NS_DECL_THREADSAFE_ISUPPORTS

Nit: Move to public section, and doesn't need to be threadsafe

@@ +62,5 @@
> +  {
> +    DOM_CAMERA_LOGT("%s:%d\n", __func__, __LINE__);
> +    MOZ_CRASH("Failed to create CameraChild actor!");
> +  }
> +  

Nit: whitespace

@@ +86,5 @@
> +  if (backgroundChild) {
> +    CameraChildCreateCallback::CreateCameraActor(backgroundChild, this);
> +  } else {
> +    nsRefPtr<nsIIPCBackgroundChildCreateCallback> callback =
> +      new CameraChildCreateCallback(this);

It's a little sketchy to pass 'this' from within the constructor here because your refcount is invalid at this point. If the callee does an AddRef/Release pair (like for a temporary QueryInterface call) then your object will be deleted before this call returns. And I know that this isn't an issue with your current code, but there's no guarantee that someone won't change things later and expose a security issue.

Usually we split this into two steps to make sure it's always safe:

  CameraChild::CameraChild()
  {
    // Stuff that doesn't pass 'this' around.
  }

  static already_AddRefed<CameraChild>
  CameraChild::Create()
  {
    nsRefPtr<CameraChild> camera = new CameraChild();
    // Using 'camera' is safe now because it has a stable refcount
    return camera.forget();
  }

@@ +106,5 @@
>  CameraChild::ActorDestroy(ActorDestroyReason aWhy)
>  {
>    DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);
> +  MOZ_ASSERT(!XRE_IsParentProcess());
> +  AssertIsOnBackgroundThread();

You really want NS_DECL_OWNINGTHREAD in the CameraChild declaration and NS_ASSERT_OWNINGTHREAD here (and elsewhere in this file). You're definitely not on "the PBackground thread" here, so the name is horribly confusing.

@@ +118,5 @@
>  bool
>  CameraChild::RecvCameraAccessAllowed()
>  {
>    DOM_CAMERA_LOGT("%s:%d : this=%p\n", __func__, __LINE__, this);
> +  MOZ_ASSERT(!XRE_IsParentProcess());

It's not really worth asserting this over and over here. Asserting it once in CameraChild's constructor should be sufficient.

(Thread assertions are another matter of course so doing those assertions everywhere is good)

::: dom/camera/ipc/CameraParent.h
@@ +21,5 @@
>  
>  class CameraParent final : public PCameraParent
>                           , public CameraServiceClient
>  {
> +  NS_DECL_OWNINGTHREAD

Nit: Put this with other members

::: ipc/glue/BackgroundParentImpl.cpp
@@ +617,4 @@
>    if (parent) {
>      mCameraActorParents.PutEntry(self);
>    }
> +  return parent.forget().downcast<PCameraParent>().take();

Hopefully the downcast isn't needed?

::: ipc/glue/BackgroundParentImpl.h
@@ +21,5 @@
>  class BackgroundParentImpl : public PBackgroundParent
>  {
> +  // Used to ensure that CameraParents are limited to one per child process.
> +  // void pointers should be able to hold intptr_t values, and we don't
> +  // really care about the stored values -- just whether or not they exist.

Nit: this comment is talking about void pointers still

@@ +22,5 @@
>  {
> +  // Used to ensure that CameraParents are limited to one per child process.
> +  // void pointers should be able to hold intptr_t values, and we don't
> +  // really care about the stored values -- just whether or not they exist.
> +  nsTHashtable<nsUint64HashKey> mCameraActorParents;

Oh, sorry I missed this before. You can't have the hashset be a member of BackgroundParentImpl.

The goal is to make sure that there is only ever one PCameraParent per PContentParent. But there can be many PBackgroundParent per PContentParent. So you need this hash set to be a global somehow. It can't be a member of BackgroundParentImpl.

The only trick is that you have to make sure to clean it up when the last PBackgroundParent is cleaned up. I'd suggest keeping a global refcount+hashset:

  uint32_t gInstanceCount = 0;
  StaticAutoPtr<nsTHashtable<nsUint64HashKey>> gUniqueCameraSet;

  // ...

  BackgroundParentImpl::BackgroundParentImpl()
  {
    // ...
    if (!gInstanceCount) {
      MOZ_ASSERT(!gUniqueCameraSet);
      gUniqueCameraSet = new nsTHashtable<nsUint64HashKey>();
    }
    gInstanceCount++;
  }

  BackgroundParentImpl::~BackgroundParentImpl()
  {
    // ...
    MOZ_ASSERT(gInstanceCount);
    gInstanceCount--;
    if (!gInstanceCount) {
      MOZ_ASSERT(gUniqueCameraSet);
      MOZ_ASSERT(!gUniqueCameraSet->Count());
      gUniqueCameraSet = nullptr;
    }
  }

Does that make sense?

Comment 41

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Implement camera broker service in parent process, v4, r=aosmond

Refresh against tree.

Comment 42

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Refactor based on review feedback.

Incorporate review feedback.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c3e7b6297e8

(In reply to Ben Turner (not reading bugmail, use the needinfo flag!) from comment #40)
> Comment on attachment 8628238 [details] [diff] [review]
> bug1073017_refactor_2015-07-01.patch
> 
> Review of attachment 8628238 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Oh, sorry I missed this before. You can't have the hashset be a member of
> BackgroundParentImpl.
> 
> The goal is to make sure that there is only ever one PCameraParent per
> PContentParent. But there can be many PBackgroundParent per PContentParent.
> So you need this hash set to be a global somehow. It can't be a member of
> BackgroundParentImpl.
> 
> The only trick is that you have to make sure to clean it up when the last
> PBackgroundParent is cleaned up. I'd suggest keeping a global
> refcount+hashset:
> 
>   uint32_t gInstanceCount = 0;
>   StaticAutoPtr<nsTHashtable<nsUint64HashKey>> gUniqueCameraSet;
> 
>   // ...
> 
>   BackgroundParentImpl::BackgroundParentImpl()
>   {
>     // ...
>     if (!gInstanceCount) {
>       MOZ_ASSERT(!gUniqueCameraSet);
>       gUniqueCameraSet = new nsTHashtable<nsUint64HashKey>();
>     }
>     gInstanceCount++;
>   }
> 
>   BackgroundParentImpl::~BackgroundParentImpl()
>   {
>     // ...
>     MOZ_ASSERT(gInstanceCount);
>     gInstanceCount--;
>     if (!gInstanceCount) {
>       MOZ_ASSERT(gUniqueCameraSet);
>       MOZ_ASSERT(!gUniqueCameraSet->Count());
>       gUniqueCameraSet = nullptr;
>     }
>   }
> 
> Does that make sense?

Yes but in the end I removed the assert entirely. It is tricky because the actor can be destroyed before PCameraChild is, such as when the child process dies (either by a crash, or by updating the camera gaia application via "make install-gaia APP=camera"). Since the actor is already gone, GetRawContentParentForComparison asserts as it has no underlying pointer to use for comparison purposes, and even if it did not, we would assert due to not finding the entry in our hashtable. That is very annoying when doing app development which uses the camera on a debug build for example.

Comment 43

[Andrew Osmond [:aosmond] (he/him)]

Comment on attachment 8636145 [details] [diff] [review]
Part 2. Refactor based on review feedback.

I hope you have one more quick review in you :). I incorporated all of your feedback save what I explained in comment 42.

Comment 44

[Gian-Carlo Pascutto [:gcp]]

What's the status of this work? I'm looking at bug 1194699 and I'll need to extend bug 1104616 with some things that these patches were also trying to address.

Comment 45

[BMO Automation]

Firefox OS is not being worked on