Closed Bug 773847 Opened 12 years ago Closed 12 years ago

Selecting to capture a picture for getUserMedia test page fails with an exception thrown on startMedia line 139

Categories

(Core :: WebRTC: Audio/Video, defect)

ARM
Android
defect
Not set
blocker

Tracking

()

VERIFIED FIXED
mozilla17
Tracking Status
firefox15 --- unaffected
firefox16 --- affected
firefox17 --- verified

People

(Reporter: jsmith, Assigned: jesup)

References

Details

(Keywords: regression, Whiteboard: [getUserMedia], [blocking-gum+], [qa!])

Attachments

(3 files, 3 obsolete files)

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
It looks like this bug regressed and broke bug 738528, for android.  likely due to a gUM regression when this landed for desktop.
Blocks: 738528
Keywords: regression
Version: unspecified → Trunk
note: this bug is specifically for Fennec Android.   it does not affect Desktop Firefox.
Severity: normal → blocker
Whiteboard: [getUserMedia], [blocking-gum+]
QA Contact: jsmith
is it expect that this would work as expected in Fx15?
(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.)
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.
(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
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.
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).
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;
    }
    ...
(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).
(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|.
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
Attachment #654932 - Attachment is obsolete: true
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.
Attachment #655185 - Flags: review?(jst)
Attachment #655185 - Flags: feedback?(blassey.bugs)
Attachment #655185 - Flags: review?(mounir)
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
Attachment #655185 - Flags: review?(mounir)
Attachment #655185 - Flags: review?(jst)
Attachment #655185 - Flags: review+
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.
Try build running; will verify with that
Attachment #655185 - Attachment is obsolete: true
Attachment #655185 - Flags: feedback?(blassey.bugs)
Attachment #655352 - Flags: review?(mounir)
Attachment #655352 - Flags: review?(blassey.bugs)
Assignee: nobody → rjesup
Attachment #655352 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #655352 - Flags: review?(mounir)
Attachment #655352 - Flags: review?(blassey.bugs)
Attachment #655360 - Flags: review?(blassey.bugs)
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
Attachment #655360 - Flags: review?(blassey.bugs)
Attachment #655374 - Flags: review?(blassey.bugs)
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.
Attachment #655374 - Flags: review?(blassey.bugs) → review+
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.
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.
(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.
https://hg.mozilla.org/mozilla-central/rev/88755b34790a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [qa+]
Verified on Nightly.
Status: RESOLVED → VERIFIED
Whiteboard: [getUserMedia], [blocking-gum+], [qa+] → [getUserMedia], [blocking-gum+], [qa!]
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: