Last Comment Bug 773847 - Selecting to capture a picture for getUserMedia test page fails with an exception thrown on startMedia line 139
: Selecting to capture a picture for getUserMedia test page fails with an excep...
Status: VERIFIED FIXED
[getUserMedia], [blocking-gum+], [qa!]
: regression
Product: Core
Classification: Components
Component: WebRTC: Audio/Video (show other bugs)
: Trunk
: ARM Android
: -- blocker (vote)
: mozilla17
Assigned To: [:jesup] on pto until 2016/8/1 Randell Jesup
: Jason Smith [:jsmith]
Mentors:
Depends on:
Blocks: 738528
  Show dependency treegraph
 
Reported: 2012-07-13 16:04 PDT by Jason Smith [:jsmith]
Modified: 2012-11-25 16:59 PST (History)
8 users (show)
jsmith: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
unaffected
affected
verified


Attachments
Picture Fails on Galaxy Nexus (105.05 KB, image/png)
2012-07-13 16:04 PDT, Jason Smith [:jsmith]
no flags Details
some LOGs to help catch the bug (3.41 KB, patch)
2012-08-23 23:20 PDT, [:jesup] on pto until 2016/8/1 Randell Jesup
no flags Details | Diff | Splinter Review
revise popup logic (control state comparison) to match nsHTMLInputElement.cpp (2.39 KB, patch)
2012-08-24 15:12 PDT, [:jesup] on pto until 2016/8/1 Randell Jesup
mounir: review+
Details | Diff | Splinter Review
Fix popupcontrol logic and take snapshots from main thread (3.44 KB, patch)
2012-08-25 14:02 PDT, [:jesup] on pto until 2016/8/1 Randell Jesup
no flags Details | Diff | Splinter Review
Modified patch to allow ShowFilePickerForMimeType from any thread; flips pref to default available (for Android only) (3.83 KB, patch)
2012-08-25 15:05 PDT, [:jesup] on pto until 2016/8/1 Randell Jesup
no flags Details | Diff | Splinter Review
Fix popupcontrol logic and take snapshots from main thread, enable Snapshots on Android by default (4.18 KB, patch)
2012-08-25 16:01 PDT, [:jesup] on pto until 2016/8/1 Randell Jesup
blassey.bugs: review+
Details | Diff | Splinter Review

Description Jason Smith [:jsmith] 2012-07-13 16:04:35 PDT
Created attachment 642122 [details]
Picture Fails on Galaxy Nexus

Steps:

1. Launch the latest nightly build of FF Android
2. Go to https://people.mozilla.com/~anarayanan/gum_test.html
3. Go to picture

Expected:

A picture should be shot from my phone's camera.

Actual:

See screenshot. An exception is thrown. No picture is taken.

Build: FF Android Nightly
Phone: Samsung Galaxy Nexus
OS: Android 4.04
Comment 1 Tony Chung [:tchung] 2012-08-01 10:09:09 PDT
It looks like this bug regressed and broke bug 738528, for android.  likely due to a gUM regression when this landed for desktop.
Comment 2 Tony Chung [:tchung] 2012-08-01 11:46:30 PDT
note: this bug is specifically for Fennec Android.   it does not affect Desktop Firefox.
Comment 3 Brad Lassey [:blassey] (use needinfo?) 2012-08-21 11:08:06 PDT
is it expect that this would work as expected in Fx15?
Comment 4 Maire Reavy (On PTO July 23rd to 31st) 2012-08-21 11:21:45 PDT
(In reply to Brad Lassey [:blassey] from comment #3)
> is it expect that this would work as expected in Fx15?

Yes. This bug was introduced after Fx15 uplifted.  Fx16+ is affected.  (Fx15 works as expected.)
Comment 5 Brad Lassey [:blassey] (use needinfo?) 2012-08-21 11:47:45 PDT
The reason I ask is I couldn't arrive at the expected results by the STR in Fx15. Are there any prefs I need to flip here?

I click the picture button, and all that happens is I get a button labeled stop.
Comment 6 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-08-21 12:24:47 PDT
(summarizing from IRC) The pref is media.navigator.enabled.  It's not in all.js in FF15, you need to create it.  See the end of Bug 738528
Comment 7 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-08-23 23:20:55 PDT
Created attachment 654932 [details] [diff] [review]
some LOGs to help catch the bug
Comment 8 Brad Lassey [:blassey] (use needinfo?) 2012-08-24 08:04:43 PDT
this is where this is failing. https://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#378

I don't know why we're using the pop up blocker here or what the expected state is.
Comment 9 Anant Narayanan [:anant] 2012-08-24 09:26:56 PDT
Hmm, odd because it's the same code that was in FF 15 but the check was passing. The other possibility is that code was never executed because of bad #ifdef's!

Popup blocking was added per Jonas' and Mounir's comments (bug 738528, comments 45 and 80). The PopupControlState should be > openControlled, which should be set when a user action is taken (such as tapping on a button, as is the case in the demo page).
Comment 10 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-08-24 09:53:37 PDT
Here's the code from FF15 (current Beta).  Note the code is in Run(), dispatched to the main thread by getUserMedia() via the GetUserMediaRunnable::Run() method.  In Head/Aurora, it's directly in getUserMedia.  One difference I see: Beta getUserMedia() does this before launching the runnables:


  // Store the WindowID in a hash table and mark as active. The entry is removed
  // when this window is closed or navigated away from.
  PRUint64 windowID = aWindow->WindowID();
  StreamListeners* listeners = mActiveWindows.Get(windowID);
  if (!listeners) {
    listeners = new StreamListeners;
    mActiveWindows.Put(windowID, listeners);
  }

Could that affect the popup state?  I don't know enough to be sure, but it's a difference.

  GetUserMediaSnapshotCallbackRunable::Run()
  {
    mWindowID = mWindow->WindowID();

    // Before getting a snapshot, check if page is allowed to open a popup.
    // We do this because {picture:true} on all platforms will open a new
    // "window" to let the user preview or select an image.

    if (mWindow->GetPopupControlState() <= openControlled) {
      return NS_OK;
    }
    
    nsCOMPtr<nsIPopupWindowManager> pm =
      do_GetService(NS_POPUPWINDOWMANAGER_CONTRACTID);
    if (!pm) {
      return NS_OK;
    }

    PRUint32 permission;
    nsCOMPtr<nsIDocument> doc = mWindow->GetExtantDoc();
    pm->TestPermission(doc->GetDocumentURI(), &permission);
    if (permission == nsIPopupWindowManager::DENY_POPUP) {
      nsCOMPtr<nsIDOMDocument> domDoc = mWindow->GetExtantDocument();
      nsGlobalWindow::FirePopupBlockedEvent(
        domDoc, mWindow, nsnull, EmptyString(), EmptyString()
      );
      return NS_OK;
    }
    ...
Comment 11 Anant Narayanan [:anant] 2012-08-24 10:02:16 PDT
(In reply to Randell Jesup [:jesup] from comment #10)
> Here's the code from FF15 (current Beta).  Note the code is in Run(),
> dispatched to the main thread by getUserMedia() via the
> GetUserMediaRunnable::Run() method.  In Head/Aurora, it's directly in
> getUserMedia.  One difference I see: Beta getUserMedia() does this before
> launching the runnables:

As long as both the old & new versions are on the main thread, we should be OK.

>   // Store the WindowID in a hash table and mark as active. The entry is
> removed
>   // when this window is closed or navigated away from.
>   PRUint64 windowID = aWindow->WindowID();
>   StreamListeners* listeners = mActiveWindows.Get(windowID);
>   if (!listeners) {
>     listeners = new StreamListeners;
>     mActiveWindows.Put(windowID, listeners);
>   }
> 
> Could that affect the popup state?  I don't know enough to be sure, but it's
> a difference.

Nope, we store in the windowID in a hashtable purely for MediaManager's own use (specifically, to check if a window is still open before calling any callbacks on it).
Comment 12 Mounir Lamouri (:mounir) 2012-08-24 12:31:57 PDT
(In reply to Brad Lassey [:blassey] from comment #8)
> this is where this is failing.
> https://mxr.mozilla.org/mozilla-central/source/dom/media/MediaManager.cpp#378
> 
> I don't know why we're using the pop up blocker here or what the expected
> state is.

The error is exactly in that line. Look at:
https://mxr.mozilla.org/mozilla-central/source/dom/base/nsPIDOMWindow.h#33

As you can see, failing in case of |<= openControlled| will fail if the call is allowed. You should have the exact opposite test: |> openControlled|.
Comment 13 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-08-24 15:12:00 PDT
Created attachment 655185 [details] [diff] [review]
revise popup logic (control state comparison) to match nsHTMLInputElement.cpp

The test for PopupControlState was backwards; revised it to match nsHTMLInputElement.cpp (and bring the rest of the popup code into the if(){}).  Less-than and greater-than on enums are overloaded concepts and should be avoided!  Unable to test as I don't have an android dev environment
Comment 14 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-08-24 23:24:47 PDT
Comment on attachment 655185 [details] [diff] [review]
revise popup logic (control state comparison) to match nsHTMLInputElement.cpp

Brad - can you get someone to do an Android build of this as a test and check if it works as intended?  Thanks.
Comment 15 Mounir Lamouri (:mounir) 2012-08-25 06:44:19 PDT
Comment on attachment 655185 [details] [diff] [review]
revise popup logic (control state comparison) to match nsHTMLInputElement.cpp

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

Don't return NS_ERROR_FAILURE is a call is blocked by the popup blocker. That would break the script by throwing an exception. That should only happen in case of real failures.

::: dom/media/MediaManager.cpp
@@ +378,5 @@
> +    if (aWindow->GetPopupControlState() > openControlled) {
> +      nsCOMPtr<nsIPopupWindowManager> pm =
> +        do_GetService(NS_POPUPWINDOWMANAGER_CONTRACTID);
> +      if (!pm)
> +        return NS_ERROR_FAILURE;

NS_OK

@@ +383,5 @@
>  
> +      uint32_t permission;
> +      nsCOMPtr<nsIDocument> doc = aWindow->GetExtantDoc();
> +      pm->TestPermission(doc->NodePrincipal(), &permission);
> +      if (aWindow && (permission == nsIPopupWindowManager::DENY_POPUP)) {

Remove the check for aWindow. If aWindow is null, you already had two other crashy call points.

@@ +388,5 @@
> +        nsCOMPtr<nsIDOMDocument> domDoc = aWindow->GetExtantDocument();
> +        nsGlobalWindow::FirePopupBlockedEvent(
> +          domDoc, aWindow, nullptr, EmptyString(), EmptyString()
> +                                              );
> +        return NS_ERROR_FAILURE;

NS_OK
Comment 16 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-08-25 13:44:07 PDT
Ok, the other problem is that with this fixed, it simply says "Success" without popping the Intent (to select the camera and take a picture).  

The cause is that on Android, ShowFilePickerForMimeType() must be run on the Main Thread (on sBridge->mThread).  One of the thing done to unblock the main thread for GetUserMedia as part of the WebRTC landing was to move the media capture calls to another thread; but this breaks Snapshot() on Android.

Trying a patch to put Snapshot() calls on the main thread.
Comment 17 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-08-25 14:02:11 PDT
Created attachment 655352 [details] [diff] [review]
Fix popupcontrol logic and take snapshots from main thread

Try build running; will verify with that
Comment 18 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-08-25 15:05:06 PDT
Created attachment 655360 [details] [diff] [review]
Modified patch to allow ShowFilePickerForMimeType from any thread; flips pref to default available (for Android only)
Comment 19 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-08-25 16:01:41 PDT
Created attachment 655374 [details] [diff] [review]
Fix popupcontrol logic and take snapshots from main thread, enable Snapshots on Android by default

GetJNIForThread() patch was unstable; do something like that for a future fix.  This is the previous patch, with enable for Android only turned on by default
Comment 20 Anant Narayanan [:anant] 2012-08-25 17:59:30 PDT
It would be better to DispatchToMainThread in MediaEngineDefault.cpp instead of MediaManager.cpp. The contract between MediaManager and a MediaEngine is that Snapshot is expected to be called *off* the main thread, as it can block (see MediaEngineWebRTCVideo.cpp).

Since on Android it's OK to block on the main thread because of the intent popup, a DispatchToMainThread in MediaEngineDefault in the #ifdef block for Android is not harmful.
Comment 21 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-08-25 20:36:23 PDT
Agreed it would be correct to have the main-thread requirement hidden, and initially blassey indicated the same thing - however, implementation-wise, since Snapshot() is a synchronous call, we would have to both DispatchToMainThread(), and wait for the Runnable on the main thread to complete.  Brad and I thought about it and decided the best solution was to try to make Android's ShowFilePickerForMimeType() not insistent on being run from Main Thread - but that ran into the stability problem in the other patch here.

So, given the preferred solution needs more work (and is riskier), and since Desktop doesn't yet support Snapshot (and is preffed off even if it does), dispatching to Main Thread were I did seemed a reasonable solution for now.  We'll want to either implement your solution or the solution in the other patch here for FF18.

When we land this patch, we'll mark this as leave open.
Comment 22 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-08-25 20:39:51 PDT
Actually, to make things cleaner, let's close this bug when it lands and we'll open a new bug to clean up the implementation.
Comment 23 [:jesup] on pto until 2016/8/1 Randell Jesup 2012-08-25 22:34:30 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/88755b34790a
Comment 24 Anant Narayanan [:anant] 2012-08-25 23:57:41 PDT
(In reply to Randell Jesup [:jesup] from comment #21)
> Agreed it would be correct to have the main-thread requirement hidden, and
> initially blassey indicated the same thing - however, implementation-wise,
> since Snapshot() is a synchronous call, we would have to both
> DispatchToMainThread(), and wait for the Runnable on the main thread to
> complete.

Well, if we dispatch to the main thread with DISPATCH_SYNC, we won't have to wait.

> So, given the preferred solution needs more work (and is riskier), and since
> Desktop doesn't yet support Snapshot (and is preffed off even if it does),
> dispatching to Main Thread were I did seemed a reasonable solution for now. 
> We'll want to either implement your solution or the solution in the other
> patch here for FF18.

Ok, sounds good.
Comment 25 Ryan VanderMeulen [:RyanVM] 2012-08-26 13:05:45 PDT
https://hg.mozilla.org/mozilla-central/rev/88755b34790a
Comment 26 Jason Smith [:jsmith] 2012-08-28 15:14:53 PDT
Verified on Nightly.

Note You need to log in before you can comment on or make changes to this bug.