Camera - need to handle OnNavigation events to prevent stale callback invocations

RESOLVED FIXED

Status

Firefox OS
General
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mikeh, Assigned: mikeh)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-basecamp:+)

Details

(Whiteboard: [LOE:S])

Attachments

(1 attachment, 11 obsolete attachments)

55.02 KB, patch
Details | Diff | Splinter Review
DOMCameraManager.cpp:48:  // TODO: implement -- see getUserMedia() implementation
DOMCameraManager.h:70:    // TODO: window management stuff
Summary: [b2g] Camera - need to handle OnNavigation events to prevent stale callback invocations → Camera - need to handle OnNavigation events to prevent stale callback invocations
blocking-basecamp: --- → ?
Whiteboard: [LOE:S]
Status: NEW → ASSIGNED
Sounds pretty bad.
blocking-basecamp: ? → +
Created attachment 661394 [details] [diff] [review]
WIP - handle navigation events to clean up object references
Created attachment 661396 [details] [diff] [review]
WIP - handle navigation events to clean up object references

The right patch this time.
Attachment #661394 - Attachment is obsolete: true
Created attachment 662321 [details] [diff] [review]
handle navigation events to clean up object references
Attachment #661396 - Attachment is obsolete: true
Comment on attachment 662321 [details] [diff] [review]
handle navigation events to clean up object references

Hi :jst, any chance you have some time to review this patch?
Attachment #662321 - Flags: review?(jst)
Sorry for not getting to this sooner, I was out on vacation for a while there... My one question from reading through this patch the first time (still need to look a bit more) is what the rationale behind the window id handling changes. Seems the right thing to do here is to pass in the window id like we used to do in Navigator::GetMozCameras(), and use that window id from then on, instead of relying on being able to get a window id from the running code. That may or may not be possible, especially when dealing with JS running in a JS component or some such, i.e. not in a DOM window's scope.
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #6)
>
> Sorry for not getting to this sooner, I was out on vacation for a while
> there... My one question from reading through this patch the first time
> (still need to look a bit more) is what the rationale behind the window id
> handling changes. Seems the right thing to do here is to pass in the window
> id like we used to do in Navigator::GetMozCameras(), and use that window id
> from then on, instead of relying on being able to get a window id from the
> running code.

I did this to conserve resources, to create only one DOMCameraManager (DCM) for the system instead of one per Navigator instance.  In this case, the DCM is not tied to a specific window or window ID.

> That may or may not be possible, especially when dealing with
> JS running in a JS component or some such, i.e. not in a DOM window's scope.

Hmm, Navigator::GetMozCameras() goes to some lengths to prevent general camera access to stale windows or windows that don't refer to each other as expected:

    nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
    NS_ENSURE_TRUE(win, NS_ERROR_FAILURE);

    if (!win->GetOuterWindow() || win->GetOuterWindow()->GetCurrentInnerWindow() != win) {
      return NS_ERROR_NOT_AVAILABLE;
    }

Is this sufficient to guard against your concern?  If not, I'll need you (or someone else) to bring me up to speed on JS componentry and how to handle that.  This code is based heavily on what's already done in the getUserMedia() implementation, and this is how that code handles stale windows.
(In reply to Mike Habicher [:mikeh] from comment #7)
> Hmm, Navigator::GetMozCameras() goes to some lengths to prevent general
> camera access to stale windows or windows that don't refer to each other as
> expected:
> 
>     nsCOMPtr<nsPIDOMWindow> win = do_QueryReferent(mWindow);
>     NS_ENSURE_TRUE(win, NS_ERROR_FAILURE);
> 
>     if (!win->GetOuterWindow() ||
> win->GetOuterWindow()->GetCurrentInnerWindow() != win) {
>       return NS_ERROR_NOT_AVAILABLE;
>     }
> 
> Is this sufficient to guard against your concern?  If not, I'll need you (or
> someone else) to bring me up to speed on JS componentry and how to handle
> that.  This code is based heavily on what's already done in the
> getUserMedia() implementation, and this is how that code handles stale
> windows.

I'm afraid not, or when dealing with stale windows, yes, but finding the window id for the correct window by calling nsJSUtils::GetCurrentlyRunningCodeInnerWindowID() may or may not get you what you want. Consider the case where a JS component, and not a webpage or B2G app, finds a reference to a window and calls into your code, at that point nsJSUtils::GetCurrentlyRunningCodeInnerWindowID() will not find a window at all, since the running code is not running in a window at all. You'll get 0 back from that call and you won't find a window to associate with here. Or consider the case where JS in window A calls into another window, B, and gets camera stuff off of that window. The code in this patch will think that the camera call happened for window A, when it really should be associated with window B.

How much resources are we talking about saving here? If it's a trivial amount, maybe we can just keep creating a manager per window where camera stuff is done. If it's more, maybe we can simply set up something where we create a light weight object per window that can carry the window id here, and that way keep things working no matter what JS code calls into this stuff.
(In reply to Johnny Stenback (:jst, jst@mozilla.com) from comment #8)
> 
> How much resources are we talking about saving here? If it's a trivial
> amount, maybe we can just keep creating a manager per window where camera
> stuff is done. If it's more, maybe we can simply set up something where we
> create a light weight object per window that can carry the window id here,
> and that way keep things working no matter what JS code calls into this
> stuff.

Not a huge amount of resources.  And it does simplify my patch.

Okay, thanks for the feedback.  I'll get a new patch up this afternoon to address your concerns.
Created attachment 665727 [details] [diff] [review]
handle navigation events to clean up object references

As discussed.
Attachment #662321 - Attachment is obsolete: true
Attachment #662321 - Flags: review?(jst)
Attachment #665727 - Flags: review?(jst)
Comment on attachment 665727 [details] [diff] [review]
handle navigation events to clean up object references

This looks good! But I did find one issue you should fix before this lands...

 nsDOMCameraManager::nsDOMCameraManager(uint64_t aWindowId)
   : mWindowId(aWindowId)
+  , mCameraThread(nullptr)
 {
   /* member initializers and constructor code */
   DOM_CAMERA_LOGT("%s:%d : this=%p, windowId=%llx\n", __func__, __LINE__, this, mWindowId);
+  nsCOMPtr<nsIObserverService> obs = services::GetObserverService();
+  obs->AddObserver(this, "xpcom-shutdown", false);

Two things here. One, you're passing |this| to an XPCOM method before anything else holds |this| alive. In this case, given the current implementation of AddObserver(), you likely get away with that, but it violates XPCOM rules, and the problem is that if AddObserver() ever changes to call AddRef() and then Release() on its argument (which would be perfectly valid), |this| will be deleted from within its constructor.

The second thing is that this code as written means that this object is held alive by the observer service until shutdown happens (i.e. you'll "leak" in the sense that any page that ever creates an nsDOMCameraManager will cause it to stay in memory until shutdown). The only place where you call RemoveObserver() is when you get the XPCOM shutdown notification, or in the destructor of this object, except your destructor will never be called because the observer holds this object alive.

There are two general approaches around those problems, one is to make nsDOMCameraManager implement nsIWeakReference and pass true as the last argument to AddObserver(). But that only fixes the second problem, and might actually cause the first issue to bite as well.

The other option, which is probably what you want here, is to have another class that is your nsIObserver. That has several benefits in that it lets you easily avoid the first problem above, makes the second problem a non-issue, and it also lets you only have one observer instead of one per nsDOMCameraManager. It will will save you on resources in cases where you have more than one app using the camera (which is probably unlikely on b2g any time soon, but still), and it won't cost you noticeably in the case where there's only one.

I.e. something like this:

class nsDOMCameraManagerObserver : public nsIObserver
{
  NS_DECL_...
  ...

  Observe(...)
  {
    nsDOMCameraManager::sActiveWindows.Clear();
  }
}

class nsDOMCameraManager : ...
{
  static nsDOMCameraManagerObserver *sObserver;
  static uint32_t sCameraManagerCount;

  nsDOMCameraManager()
  {
    if (!sCameraManagerCount++) {
      sCameraManager = new nsDOMCameraManagerObserver();
      NS_ADDREF(sCameraManager);

      ...

      obs->AddObserver(sCameraManager, "xpcom-shutdown", false);
    }
  }

  ~nsDOMCameraManager()
  {
    if (!--sCameraManagerCount) {
      ...

      obs->RemoveObserver(sCameraManager, "xpcom-shutdown");
      NS_RELEASE(sCameraManager);
    }
  }
}

- In nsDOMCameraManager::Observe(nsISupports* aSubject, const char* aTopic, const PRUnichar* aData)

+  if (strcmp(aTopic, "xpcom-shutdown") == 0) {
+    return XpComShutdown();

The caller of this method, the observer service, doesn't care about the return code. So you could make XpComShutdown() a void method.

r=jst with something along those lines. Happy to look over the observer stuff once more once that's fixed up, but the rest looks really good!
Attachment #665727 - Flags: review?(jst) → review+
Created attachment 666021 [details] [diff] [review]
handle navigation events to clean up object references (20120928-1528 EDT)

:jst, thanks for the feedback.  This patch incorporates your suggestions by:
1. moving the call to AddObserver() outside of the DOMCameraManager ctor and into the static Create()
2. making DOMCameraManager support weak references, and setting AddObserver()'s third parameter to 'true'
3. XpComShutdown() now returns void.
Attachment #665727 - Attachment is obsolete: true
Attachment #666021 - Flags: review?(jst)
Comment on attachment 666021 [details] [diff] [review]
handle navigation events to clean up object references (20120928-1528 EDT)

Looks good, r=jst
Attachment #666021 - Flags: review?(jst) → review+
Created attachment 666075 [details] [diff] [review]
handle navigation events to clean up object references (20120928-1810 EDT)

try in progress: https://tbpl.mozilla.org/?tree=Try&rev=99118a653acd
Attachment #666021 - Attachment is obsolete: true
Created attachment 666082 [details] [diff] [review]
handle navigation events to clean up object references (20120928-1823 EDT)
Attachment #666075 - Attachment is obsolete: true
Created attachment 666083 [details] [diff] [review]
handle navigation events to clean up object references (20120928-1823 EDT)

try in progress: https://tbpl.mozilla.org/?tree=Try&rev=dce67fad0a23
Attachment #666082 - Attachment is obsolete: true
Keywords: checkin-needed
This doesn't apply cleanly to inbound. Please rebase.

patching file dom/camera/CameraControlImpl.h
Hunk #1 FAILED at 2
Hunk #2 FAILED at 35
Hunk #3 succeeded at 73 with fuzz 1 (offset 4 lines).
Hunk #5 FAILED at 113
Hunk #10 FAILED at 355
4 out of 10 hunks FAILED -- saving rejects to file dom/camera/CameraControlImpl.h.rej
patching file dom/camera/DOMCameraControl.cpp
Hunk #1 FAILED at 202
Hunk #2 FAILED at 313
2 out of 3 hunks FAILED -- saving rejects to file dom/camera/DOMCameraControl.cpp.rej
patching file dom/camera/GonkCameraHwMgr.cpp
Hunk #2 succeeded at 194 with fuzz 1 (offset 28 lines).
patching file dom/camera/ICameraControl.h
Hunk #1 succeeded at 33 with fuzz 1 (offset 2 lines).
patching file dom/camera/nsIDOMCameraManager.idl
Hunk #1 FAILED at 171
1 out of 2 hunks FAILED -- saving rejects to file dom/camera/nsIDOMCameraManager.idl.rej
patch failed, unable to continue (try -v)
patch failed, rejects left in working dir
errors during apply, please fix and refresh 779145
Keywords: checkin-needed
Created attachment 666213 [details] [diff] [review]
handle navigation events to clean up object references (20120929-1138 EDT)

Rebased as requested.
Attachment #666083 - Attachment is obsolete: true
Keywords: checkin-needed
This still doesn't apply cleanly. Are you rebasing against inbound?
Keywords: checkin-needed
(In reply to Ryan VanderMeulen from comment #19)
> This still doesn't apply cleanly. Are you rebasing against inbound?

No, against m-c, which is the git repo the B2G build pulls.
Created attachment 666247 [details] [diff] [review]
handle navigation events to clean up object references (20120929-1700 EDT)

Rebased to m-i.
Attachment #666213 - Attachment is obsolete: true
Keywords: checkin-needed
Much better, thanks! (Though it was missing the commit information :P)

https://hg.mozilla.org/integration/mozilla-inbound/rev/c1fff87fd8e8
Keywords: checkin-needed
Of course, compiling successfully in addition to applying cleanly is a nice trait for a patch to have. Backed out for build bustage.
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dde5624fc81

https://tbpl.mozilla.org/php/getParsedLog.php?id=15674129&tree=Mozilla-Inbound

In file included from ../../../../dom/camera/CameraControlImpl.cpp:7:
../../../../dom/camera/CameraControlImpl.h:424:42: error: no matching constructor for initialization of 'mozilla::StartRecordingResult'
        rv = NS_DispatchToMainThread(new StartRecordingResult(mOnSuccessCb));
                                         ^                    ~~~~~~~~~~~~
../../../../dom/camera/CameraControlImpl.h:372:7: note: candidate constructor (the implicit copy constructor) not viable: no known conversion from 'nsCOMPtr<nsICameraStartRecordingCallback>' to 'const mozilla::StartRecordingResult' for 1st argument
class StartRecordingResult : public nsRunnable
      ^
../../../../dom/camera/CameraControlImpl.h:375:3: note: candidate constructor not viable: requires 2 arguments, but 1 was provided
  StartRecordingResult(nsICameraStartRecordingCallback* onSuccess, uint64_t aWindowId)
  ^
1 error generated.
make[7]: *** [CameraControlImpl.o] Error 1
Created attachment 666343 [details] [diff] [review]
handle navigation events to clean up object references (20120930-1315 EDT)

try server progress: https://tbpl.mozilla.org/?tree=Try&rev=b04acaa3b38e
Attachment #666247 - Attachment is obsolete: true
Created attachment 666380 [details] [diff] [review]
handle navigation events to clean up object references (20120928-1726 EDT)

Rebased, rebuilt.

try server progress: https://tbpl.mozilla.org/?tree=Try&rev=d8039d98b52e
Attachment #666343 - Attachment is obsolete: true
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/5245844b645d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Duplicate of this bug: 779138
Keywords: dev-doc-needed
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.