The default bug view has changed. See this FAQ.

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

VERIFIED FIXED in Firefox 17

Status

()

Core
WebRTC: Audio/Video
--
blocker
VERIFIED FIXED
5 years ago
4 years ago

People

(Reporter: jsmith, Assigned: jesup)

Tracking

({regression})

Trunk
mozilla17
ARM
Android
regression
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox15 unaffected, firefox16 affected, firefox17 verified)

Details

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

Attachments

(3 attachments, 3 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 years ago
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
(Reporter)

Updated

5 years ago
Version: unspecified → Trunk

Comment 2

5 years ago
note: this bug is specifically for Fennec Android.   it does not affect Desktop Firefox.
(Reporter)

Updated

5 years ago
Severity: normal → blocker
(Reporter)

Updated

5 years ago
status-firefox15: --- → unaffected
status-firefox16: --- → affected
status-firefox17: --- → affected
(Reporter)

Updated

5 years ago
Whiteboard: [getUserMedia], [blocking-gum+]
(Reporter)

Updated

5 years ago
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.
(Assignee)

Comment 6

5 years ago
(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
(Assignee)

Comment 7

5 years ago
Created attachment 654932 [details] [diff] [review]
some LOGs to help catch the bug
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).
(Assignee)

Comment 10

5 years ago
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|.
(Assignee)

Comment 13

5 years ago
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
(Assignee)

Updated

5 years ago
Attachment #654932 - Attachment is obsolete: true
(Assignee)

Comment 14

5 years ago
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)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 16

5 years ago
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.
(Assignee)

Comment 17

5 years ago
Created attachment 655352 [details] [diff] [review]
Fix popupcontrol logic and take snapshots from main thread

Try build running; will verify with that
(Assignee)

Updated

5 years ago
Attachment #655185 - Attachment is obsolete: true
Attachment #655185 - Flags: feedback?(blassey.bugs)
(Assignee)

Updated

5 years ago
Attachment #655352 - Flags: review?(mounir)
Attachment #655352 - Flags: review?(blassey.bugs)
(Assignee)

Comment 18

5 years ago
Created attachment 655360 [details] [diff] [review]
Modified patch to allow ShowFilePickerForMimeType from any thread; flips pref to default available (for Android only)
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)
(Assignee)

Comment 19

5 years ago
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
(Assignee)

Updated

5 years ago
Attachment #655360 - Flags: review?(blassey.bugs)
(Assignee)

Updated

5 years ago
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+
(Assignee)

Comment 21

5 years ago
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.
(Assignee)

Comment 22

5 years ago
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.
(Assignee)

Comment 23

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/88755b34790a
(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
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
(Reporter)

Updated

5 years ago
status-firefox17: affected → fixed
(Reporter)

Updated

5 years ago
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [qa+]
(Reporter)

Comment 26

5 years ago
Verified on Nightly.
Status: RESOLVED → VERIFIED
status-firefox17: fixed → verified
Whiteboard: [getUserMedia], [blocking-gum+], [qa+] → [getUserMedia], [blocking-gum+], [qa!]
(Reporter)

Updated

4 years ago
Flags: in-testsuite-
You need to log in before you can comment on or make changes to this bug.