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)
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)
105.05 KB,
image/png
|
Details | |
3.83 KB,
patch
|
Details | Diff | Splinter Review | |
4.18 KB,
patch
|
blassey
:
review+
|
Details | Diff | Splinter Review |
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•12 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•12 years ago
|
Version: unspecified → Trunk
Comment 2•12 years ago
|
||
note: this bug is specifically for Fennec Android. it does not affect Desktop Firefox.
Reporter | ||
Updated•12 years ago
|
Severity: normal → blocker
Reporter | ||
Updated•12 years ago
|
status-firefox15:
--- → unaffected
status-firefox16:
--- → affected
status-firefox17:
--- → affected
Reporter | ||
Updated•12 years ago
|
Whiteboard: [getUserMedia], [blocking-gum+]
Reporter | ||
Updated•12 years ago
|
QA Contact: jsmith
Comment 3•12 years ago
|
||
is it expect that this would work as expected in Fx15?
Comment 4•12 years ago
|
||
(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•12 years ago
|
||
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•12 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•12 years ago
|
||
Comment 8•12 years ago
|
||
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•12 years ago
|
||
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•12 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; } ...
Comment 11•12 years ago
|
||
(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•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
Attachment #654932 -
Attachment is obsolete: true
Assignee | ||
Comment 14•12 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•12 years ago
|
Attachment #655185 -
Flags: review?(mounir)
Comment 15•12 years ago
|
||
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•12 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•12 years ago
|
||
Try build running; will verify with that
Assignee | ||
Updated•12 years ago
|
Attachment #655185 -
Attachment is obsolete: true
Attachment #655185 -
Flags: feedback?(blassey.bugs)
Assignee | ||
Updated•12 years ago
|
Attachment #655352 -
Flags: review?(mounir)
Attachment #655352 -
Flags: review?(blassey.bugs)
Assignee | ||
Comment 18•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
Attachment #655360 -
Flags: review?(blassey.bugs)
Assignee | ||
Updated•12 years ago
|
Attachment #655374 -
Flags: review?(blassey.bugs)
Comment 20•12 years ago
|
||
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.
Updated•12 years ago
|
Attachment #655374 -
Flags: review?(blassey.bugs) → review+
Assignee | ||
Comment 21•12 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•12 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•12 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/88755b34790a
Comment 24•12 years ago
|
||
(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•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/88755b34790a
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla17
Reporter | ||
Updated•12 years ago
|
Reporter | ||
Updated•12 years ago
|
Whiteboard: [getUserMedia], [blocking-gum+] → [getUserMedia], [blocking-gum+], [qa+]
Reporter | ||
Comment 26•12 years ago
|
||
Verified on Nightly.
Status: RESOLVED → VERIFIED
Whiteboard: [getUserMedia], [blocking-gum+], [qa+] → [getUserMedia], [blocking-gum+], [qa!]
Reporter | ||
Updated•12 years ago
|
Flags: in-testsuite-
You need to log in
before you can comment on or make changes to this bug.
Description
•