Desktop Web Runtime UI implementation for mozGetUserMedia (Camera API)

RESOLVED FIXED in Firefox 27

Status

Firefox Graveyard
Webapp Runtime
P1
normal
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: jsmith, Assigned: marco)

Tracking

({uiwanted})

16 Branch
Firefox 27
uiwanted
Bug Flags:
in-testsuite +

Details

(Whiteboard: DesktopWebRT2)

Attachments

(3 attachments, 7 obsolete attachments)

(Reporter)

Description

6 years ago
We need to add support for a user interface for mozGetUserMedia (Camera API) in the desktop web runtime. Areas include, but are not limited to:

- Permissions management for accessing the camera/audio
- Picture management upon taking a picture
- Other areas that product can provide more details on

Needs product and UX input to break down the specific requirements to having a UI for desktop web runtime for mozGetUserMedia. This bug is a meta bug for tracking the implementation of anything that's required to enable mozGetUserMedia in the desktop web runtime.
(Reporter)

Updated

6 years ago
Keywords: meta
(Reporter)

Updated

6 years ago
Keywords: productwanted, uiwanted
(Reporter)

Updated

6 years ago
Keywords: meta
Summary: [meta] Desktop Web Runtime UI implementation for mozGetUserMedia (Camera API) → Desktop Web Runtime UI implementation for mozGetUserMedia (Camera API)
(Reporter)

Updated

6 years ago
Keywords: productwanted
Priority: -- → P1
(Reporter)

Comment 1

6 years ago
Assigning to Bryan (per Rags) to find someone to work on the UX for this.
Assignee: nobody → clarkbw
(Reporter)

Updated

6 years ago
status-firefox16: --- → wontfix
Assignee: clarkbw → nobody
(Assignee)

Updated

5 years ago
Depends on: 892837
(Assignee)

Updated

5 years ago
Assignee: nobody → mcastelluccio
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 781875 [details] [diff] [review]
Patch

With this patch we show two separate prompts (two dialog windows) to the user to ask for permissions (one prompt for video, one prompt for audio).
Attachment #781875 - Flags: ui-review?(madhava)
(Assignee)

Comment 3

5 years ago
Created attachment 781876 [details]
Screenshot from 2013-07-26 12:46:04.png

The title of the dialog window is "%AppName% - Share Camera" or "%AppName% - Share Microphone".
(Assignee)

Comment 4

5 years ago
No indicator is shown when the camera/microphone is currently in use. I think that's not necessary because the user is aware that the application is using the camera/microphone.
(Reporter)

Comment 5

5 years ago
(In reply to Marco Castelluccio [:marco] from comment #2)
> Created attachment 781875 [details] [diff] [review]
> Patch
> 
> With this patch we show two separate prompts (two dialog windows) to the
> user to ask for permissions (one prompt for video, one prompt for audio).

Note - we should really use a single prompt, not double prompts. Double prompts will annoy the user unnecessarily.
(Assignee)

Comment 6

5 years ago
Created attachment 786502 [details] [diff] [review]
Patch v2

I think this is a good beginning, then we can make UX improvements in the future.

There's a single prompt, similar to the popup notification in Firefox.
Attachment #781875 - Attachment is obsolete: true
Attachment #781876 - Attachment is obsolete: true
Attachment #781875 - Flags: ui-review?(madhava)
Attachment #786502 - Flags: review?(myk)
Comment on attachment 786502 [details] [diff] [review]
Patch v2

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

This patch looks reasonable, modulo minor issues.  I haven't tested it yet, though, as it doesn't apply, even if I apply the patches from bug 892837 first.

::: browser/installer/package-manifest.in
@@ +763,5 @@
>  @BINPATH@/webapprt/defaults/preferences/prefs.js
>  @BINPATH@/webapprt/modules/Startup.jsm
>  @BINPATH@/webapprt/modules/WebappRT.jsm
>  @BINPATH@/webapprt/modules/WebappsHandler.jsm
> +@BINPATH@/webapprt/modules/WebrtcHandler.jsm

Nit: the convention for runtime filenames is to capitalize acronyms like the "RTC" in a word like "WebRTC" (f.e. WebappRT.jsm, although I wish I had named that file simply Runtime.jsm), so this should be WebRTCHandler.jsm.

::: webapprt/WebrtcHandler.jsm
@@ +13,5 @@
> +Cu.import("resource://gre/modules/Services.jsm");
> +Cu.import("resource://gre/modules/XPCOMUtils.jsm");
> +
> +function handleRequest(aSubject, aTopic, aData) {
> +  let { windowID: windowID, callID: callID } = JSON.parse(aData);

Nit: if you aren't going to name the variables differently from the properties to which you assign them in a destructuring assignment, then you don't need to specify the variable names, as destructuring assignment will assign the property values to variables with the same names as the properties by default, i.e. the following has the same effect:

  let { windowID, callID } = JSON.parse(aData); // assigns to windowID, callID

If you are going to name them differently, however, or you're doing deep destructuring, then you must specify their names, i.e.:

  let { foo: bar, baz: { qux: bazqux } } = metasyntactics; // assigns to bar, bazqux

@@ +40,5 @@
> +        break;
> +
> +      case "video":
> +        if (aVideoRequested)
> +          videoDevices.push(device);

Nit: surround conditional statements with braces here and above.

@@ +53,5 @@
> +
> +  let params = {
> +                 inn: { videoDevices: videoDevices, audioDevices: audioDevices },
> +                 out: null
> +               };

Nit: since there are only two "in" params and a single "out" param, it seems simpler to pass the "in" params directly:

let params = {
               videoDevices: videoDevices,
               audioDevices: audioDevices,
               out: null,
             };

@@ +63,5 @@
> +    return;
> +  }
> +
> +  let allowedDevices = Cc["@mozilla.org/supports-array;1"]
> +                         .createInstance(Ci.nsISupportsArray);

Nit: in browser and runtime code (unlike some platform code, like Webapps.jsm), such calls are generally structured like this:

  let allowedDevices = Cc["@mozilla.org/supports-array;1"].
                       createInstance(Ci.nsISupportsArray);

@@ +87,5 @@
> +function denyRequest(aCallID, aError) {
> +  let msg = null;
> +  if (aError) {
> +    msg = CC["@mozilla.org/supports-string;1"]
> +            .createInstance(Ci.nsISupportsString);

CC -> Cc

::: webapprt/content/gum-ask.xul
@@ +16,5 @@
> +        buttons="accept,cancel"
> +        buttonlabelaccept="&gumDialog.buttonlabelaccept;"
> +        buttonaccesskeyaccept="&gumDialog.buttonaccesskeyaccept;"
> +        onload="onLoad()"
> +        ondialogaccept="return onOK();"

Nit: trailing semi-colon typically absent in this context.

::: webapprt/jar.mn
@@ +6,5 @@
>  % content webapprt %content/
>  * content/webapp.js                     (content/webapp.js)
>  * content/webapp.xul                    (content/webapp.xul)
> +  content/gum-ask.xul                   (content/gum-ask.xul)
> +  content/gum-ask.js                    (content/gum-ask.js)

Nit: the convention for runtime filenames is to use intercaps to distinguish words, so these should be gumAsk.*. 

However, "gum" is a fairly unusual shortening of getUserMedia, used only in a couple places in our codebase; and dialogs like these typically have names that end with "dialog" in our codebase, like cookieAcceptDialog.*; so I would call this getUserMediaDialog.*.

::: webapprt/locales/jar.mn
@@ +6,5 @@
>  @AB_CD@.jar:
>  % locale webapprt @AB_CD@ %locale/webapprt/
>      locale/webapprt/webapp.dtd                     (%webapprt/webapp.dtd)
>      locale/webapprt/webapp.properties              (%webapprt/webapp.properties)
> +    locale/webapprt/gum-ask.dtd                    (%webapprt/gum-ask.dtd)

This file is missing from the patch.
Attachment #786502 - Flags: review?(myk) → review-
(Assignee)

Comment 8

5 years ago
Created attachment 789248 [details] [diff] [review]
Patch v2

(In reply to Myk Melez [:myk] [@mykmelez] from comment #7)
> This patch looks reasonable, modulo minor issues.  I haven't tested it yet,
> though, as it doesn't apply, even if I apply the patches from bug 892837
> first.

Strange, I can apply the patch on top of the patches from bug 892837.
Attachment #786502 - Attachment is obsolete: true
Attachment #789248 - Flags: review?(myk)
(In reply to Marco Castelluccio [:marco] from comment #8)
> Strange, I can apply the patch on top of the patches from bug 892837.

Hmm, I can too now.  However, my test hosted app doesn't trigger the dialog in the runtime (although it does in the browser).  Can you add an automated test to confirm that the dialog gets triggered?
(Assignee)

Comment 10

5 years ago
Created attachment 790534 [details] [diff] [review]
Test

There's still a problem to fix here:

> TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_getUserMedia.js
> leaked until shutdown [nsGlobalWindow #24 http://test/webapprtChrome/webapprt/test/chrome/getUserMedia.html]
(Reporter)

Comment 11

5 years ago
(In reply to Marco Castelluccio [:marco] from comment #10)
> Created attachment 790534 [details] [diff] [review]
> Test
> 
> There's still a problem to fix here:
> 
> > TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_getUserMedia.js
> > leaked until shutdown [nsGlobalWindow #24 http://test/webapprtChrome/webapprt/test/chrome/getUserMedia.html]

That's because you can't run automated tests on buildbot that make use of real devices. Calling gum with audio/video true is trying to look for a real camera/microphone, which is not possible in buildbot.

We do have fake stream support by applying an additional parameter of fake: true. But fake: true isn't going to prompt.

So this test isn't going to probably work in buildbot.
(Assignee)

Comment 12

5 years ago
(In reply to Myk Melez [:myk] [@mykmelez] from comment #9)
> Hmm, I can too now.  However, my test hosted app doesn't trigger the dialog
> in the runtime (although it does in the browser).  Can you add an automated
> test to confirm that the dialog gets triggered?

I've just tested also on Mac, it is working for me (make sure you don't rebuild only the webapprt directory).
(In reply to Marco Castelluccio [:marco] from comment #12)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #9)
> > Hmm, I can too now.  However, my test hosted app doesn't trigger the dialog
> > in the runtime (although it does in the browser).  Can you add an automated
> > test to confirm that the dialog gets triggered?
> 
> I've just tested also on Mac, it is working for me (make sure you don't
> rebuild only the webapprt directory).

I built clobber the first time, and I've done so again, but my test app still doesn't trigger the dialog.  What app are you using to test?
Comment on attachment 789248 [details] [diff] [review]
Patch v2

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

After another clobber build and some tweaks to the test app code (although I don't see how they would actually help), I was able to get the dialog to appear.  And the only issues are nits.

Note that this no longer applies, although the conflict is trivial. r=myk, but I'd much rather have a single patch combining the implementation and its test than two separate patches, which complicates review and landing of the change.  So please combine these patches before landing them!

::: webapprt/content/getUserMediaDialog.js
@@ +5,5 @@
> +function onOK() {
> +  window.arguments[0].out = {
> +                             video: document.getElementById("video").selectedItem.value,
> +                             audio: document.getElementById("audio").selectedItem.value
> +                            };

Nit: these lines exceed 80 columns and should be shortened, f.e. via this format:

  window.arguments[0].out = {
    video: document.getElementById("video").selectedItem.value,
    audio: document.getElementById("audio").selectedItem.value
  };

::: webapprt/content/getUserMediaDialog.xul
@@ +20,5 @@
> +        ondialogaccept="return onOK()"
> +        buttonlabelcancel="&getUserMediaDialog.buttonlabelcancel;"
> +        buttonaccesskeycancel="&getUserMediaDialog.buttonaccesskeycancel;">
> +
> +<script type="application/javascript" src="chrome://webapprt/content/getUserMediaDialog.js"/>

Nit: this line exceeds 80 columns and should be wrapped, f.e. between the attributes.

@@ +38,5 @@
> +    <menupopup>
> +      <menuitem label="&getUserMediaDialog.audio.noAudio;" value="-1"/>
> +    </menupopup>
> +  </menulist>
> +</groupbox>

Nit: the stuff between the <dialog> tags should be indented two spaces.
Attachment #789248 - Flags: review?(myk) → review+
Comment on attachment 790534 [details] [diff] [review]
Test

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

Looks good, just one minor issue.  Also note that the patch no longer applies, but the conflict is trivial.  As mentioned in my review of the other patch, please combine these patches for the final review and commission!

::: webapprt/test/chrome/browser_getUserMedia.js
@@ +28,5 @@
> +      if (msg.textContent == "PERMISSION_DENIED") {
> +        ok(true, "Error: PERMISSION_DENIED");
> +      } else {
> +        ok(false, "Error: PERMISSION_DENIED");
> +      }

Nit: the test assertion message "Error: PERMISSION_DENIED" is not very informative.  Such messages should explain what the test expects in a human-friendly way, f.e. "getUserMedia permission denied".

Also, this test should be written as:

  ok(msg.textContent == "PERMISSION_DENIED", "…");
Attachment #790534 - Flags: review-
Erm, there is that one minor issue plus this issue!

(In reply to Jason Smith [:jsmith] from comment #11)
> (In reply to Marco Castelluccio [:marco] from comment #10)
> > Created attachment 790534 [details] [diff] [review]
> > Test
> > 
> > There's still a problem to fix here:
> > 
> > > TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_getUserMedia.js
> > > leaked until shutdown [nsGlobalWindow #24 http://test/webapprtChrome/webapprt/test/chrome/getUserMedia.html]
> 
> That's because you can't run automated tests on buildbot that make use of
> real devices. Calling gum with audio/video true is trying to look for a real
> camera/microphone, which is not possible in buildbot.
> 
> We do have fake stream support by applying an additional parameter of fake:
> true. But fake: true isn't going to prompt.
> 
> So this test isn't going to probably work in buildbot.
Erm, one more nit:

/Users/myk/test_getusermedia_webapprt:100: space before tab in indent.
         	  document.getElementById("msg").textContent = err;
(Assignee)

Comment 18

5 years ago
Created attachment 792510 [details] [diff] [review]
Patch

The tests are enabled only if MOZ_WEBRTC_LEAKING_TESTS isn't defined.
Attachment #789248 - Attachment is obsolete: true
Attachment #790534 - Attachment is obsolete: true
Attachment #792510 - Flags: review?(myk)
(Reporter)

Comment 19

5 years ago
Comment on attachment 792510 [details] [diff] [review]
Patch

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

::: webapprt/test/chrome/Makefile.in
@@ +31,5 @@
>    $(NULL)
>  
> +# The following tests are leaking and cannot be run by default yet
> +# because we haven't a fake device that still prompts
> +ifndef MOZ_WEBRTC_LEAKING_TESTS

See my above comment - you cannot run tests in buildbot that make use of real devices here. So none of these tests can be checked in cleanly to the mainline tree as it stands right now.
(Assignee)

Comment 20

5 years ago
(In reply to Jason Smith [:jsmith] from comment #19)
> See my above comment - you cannot run tests in buildbot that make use of
> real devices here. So none of these tests can be checked in cleanly to the
> mainline tree as it stands right now.

The tests would be executed only if MOZ_WEBRTC_LEAKING_TESTS wasn't defined.
(Reporter)

Comment 21

5 years ago
(In reply to Marco Castelluccio [:marco] from comment #20)
> (In reply to Jason Smith [:jsmith] from comment #19)
> > See my above comment - you cannot run tests in buildbot that make use of
> > real devices here. So none of these tests can be checked in cleanly to the
> > mainline tree as it stands right now.
> 
> The tests would be executed only if MOZ_WEBRTC_LEAKING_TESTS wasn't defined.

By default that's not defined in the main tree, so that will fail if this lands on inbound. But anyways, these tests aren't valid for mozilla central until we land support for fake devices. I'd rather pull these tests out of the patch entirely and reconsider getting tests here when we land fake devices support.
(Assignee)

Comment 22

5 years ago
Created attachment 792909 [details] [diff] [review]
Patch

> By default that's not defined in the main tree, so that will fail if this
> lands on inbound.

Fixed.

> But anyways, these tests aren't valid for mozilla central
> until we land support for fake devices. I'd rather pull these tests out of
> the patch entirely and reconsider getting tests here when we land fake
> devices support.

The tests are useful for us to avoid regressions, I think we're good as long as they don't run on test servers.
Attachment #792510 - Attachment is obsolete: true
Attachment #792510 - Flags: review?(myk)
Attachment #792909 - Flags: review?(myk)
Comment on attachment 792510 [details] [diff] [review]
Patch

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

(In reply to Jason Smith [:jsmith] from comment #21)
> (In reply to Marco Castelluccio [:marco] from comment #20)
> > The tests would be executed only if MOZ_WEBRTC_LEAKING_TESTS wasn't defined.
> 
> By default that's not defined in the main tree, so that will fail if this
> lands on inbound.

Right.  The logic is reversed here, as this test script should be disabled by default, so it doesn't run in automation, but developers can run it manually.


> But anyways, these tests aren't valid for mozilla central
> until we land support for fake devices. I'd rather pull these tests out of
> the patch entirely and reconsider getting tests here when we land fake
> devices support.

The tests will work on developer machines with real devices, and it seems better to at least enable developers to run them manually than to not have them at all.

But I want to double check our reuse of the MOZ_WEBRTC_LEAKING_TESTS flag, which Henrik landed in bug 817709, but which isn't being used anymore by the core WebRTC mochitests.

Henrik: is this a reasonable reuse of that flag?

(I also wonder why the test is leaking in the first place, since it seems like it shouldn't, even on developer machines with real devices.)

::: webapprt/test/chrome/getUserMedia.html
@@ +16,5 @@
> +          if (err == "PERMISSION_DENIED") {
> +            document.getElementById("msg").textContent = "PERMISSION_DENIED";
> +          } else {
> +            document.getElementById("msg").textContent = err;
> +          }

Nit: this conditional is unnecessary, as you get the same result if you simply set textContent to err.
Attachment #792510 - Flags: review-
Attachment #792510 - Flags: feedback?(hskupin)
(Reporter)

Comment 24

5 years ago
(In reply to Myk Melez [:myk] [@mykmelez] from comment #23)
> (I also wonder why the test is leaking in the first place, since it seems
> like it shouldn't, even on developer machines with real devices.)

Where is the leak occurring btw? Is it webrt code or gUM code?

We've had a lot of leaks in webrtc code during development, so I wouldn't be surprised if this is a leak in that code again.
(In reply to Jason Smith [:jsmith] from comment #24)
> (In reply to Myk Melez [:myk] [@mykmelez] from comment #23)
> > (I also wonder why the test is leaking in the first place, since it seems
> > like it shouldn't, even on developer machines with real devices.)
> 
> Where is the leak occurring btw? Is it webrt code or gUM code?

It isn't clear, but the thing that leaks is an nsGlobalWindow for a content test page loaded in the runtime:

TEST-UNEXPECTED-FAIL | chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/browser_getUserMedia.js | leaked until shutdown [nsGlobalWindow #16 http://test/webapprtChrome/webapprt/test/chrome/getUserMedia.html]

The page does virtually nothing besides calling navigator.mozGetUserMedia with "audio" and "video" both set to true.  The chrome test script does register both a "load" listener and a mutation observer on the page, but it appears to clean them up correctly.
(Assignee)

Comment 26

5 years ago
(In reply to Myk Melez [:myk] [@mykmelez] from comment #25)
> (In reply to Jason Smith [:jsmith] from comment #24)
> > (In reply to Myk Melez [:myk] [@mykmelez] from comment #23)
> > > (I also wonder why the test is leaking in the first place, since it seems
> > > like it shouldn't, even on developer machines with real devices.)
> > 
> > Where is the leak occurring btw? Is it webrt code or gUM code?
> 
> It isn't clear, but the thing that leaks is an nsGlobalWindow for a content
> test page loaded in the runtime:
> 
> TEST-UNEXPECTED-FAIL |
> chrome://mochitests/content/webapprtChrome/webapprt/test/chrome/
> browser_getUserMedia.js | leaked until shutdown [nsGlobalWindow #16
> http://test/webapprtChrome/webapprt/test/chrome/getUserMedia.html]
> 
> The page does virtually nothing besides calling navigator.mozGetUserMedia
> with "audio" and "video" both set to true.  The chrome test script does
> register both a "load" listener and a mutation observer on the page, but it
> appears to clean them up correctly.

The test is pretty similar to the geolocation tests, but they don't leak.
Comment on attachment 792510 [details] [diff] [review]
Patch

(Removing feedback request from this patch; will add it to the new one.)
Attachment #792510 - Flags: feedback?(hskupin)
(Reporter)

Comment 28

5 years ago
FWIW - The only known bug I know of that might be related to this leak we're seeing here is bug 826538 if it's in the webrtc codebase.
Comment on attachment 792909 [details] [diff] [review]
Patch

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

r=myk, but wait to land this until we hear from Henrik about my question in comment 23.

::: webapprt/test/chrome/browser_getUserMedia.js
@@ +10,5 @@
> +      win.addEventListener("load", function onLoadWindow() {
> +        win.removeEventListener("load", onLoadWindow, false);
> +        openedWindows++;
> +        if (openedWindows == 2) {
> +          ok(true, "Prompt shown.");

Doesn't this duplicate the other test of the number of openedWindows?

@@ +24,5 @@
> +
> +  loadWebapp("getUserMedia.webapp", undefined, function onLoad() {
> +    let msg = gAppBrowser.contentDocument.getElementById("msg");
> +    mutObserver = new MutationObserver(function(mutations) {
> +      ok(msg.textContent == "PERMISSION_DENIED", "getUserMedia permission denied.");

Nit: "is" is a better test assertion function in this case:

  is(msg.textContent, "PERMISSION_DENIED", "getUserMedia permission denied.");

@@ +28,5 @@
> +      ok(msg.textContent == "PERMISSION_DENIED", "getUserMedia permission denied.");
> +
> +      if (openedWindows != 2) {
> +        ok(false, "Prompt not shown.");
> +      }

Nit: whenever possible, test assertions should test the actual condition being asserted, so this would be better written as:

  ok(openedWindows == 2, "prompt shown");

Or even better:

  is(openedWindows, 2, "prompt shown");

::: webapprt/test/chrome/getUserMedia.html
@@ +16,5 @@
> +          if (err == "PERMISSION_DENIED") {
> +            document.getElementById("msg").textContent = "PERMISSION_DENIED";
> +          } else {
> +            document.getElementById("msg").textContent = err;
> +          }

Nit: as mentioned earlier, no need for conditional here.
Attachment #792909 - Flags: review?(myk)
Attachment #792909 - Flags: review+
Attachment #792909 - Flags: feedback?(hskupin)
(In reply to Myk Melez [:myk] [@mykmelez] from comment #23)
> The tests will work on developer machines with real devices, and it seems
> better to at least enable developers to run them manually than to not have
> them at all.
> 
> But I want to double check our reuse of the MOZ_WEBRTC_LEAKING_TESTS flag,
> which Henrik landed in bug 817709, but which isn't being used anymore by the
> core WebRTC mochitests.
> 
> Henrik: is this a reasonable reuse of that flag?
> 
> (I also wonder why the test is leaking in the first place, since it seems
> like it shouldn't, even on developer machines with real devices.)

Instead of using this flag I would propose that you work with Randell or Eric to get this investigated and fixed before the patch gets landed. It might be something simple. If not, flag could be used again I think. But best is to ask Randell what he thinks. We only used this flag during the initial development of the WebRTC code as Jason already mentioned. As of now we should not land new code and tests which leak.
Flags: needinfo?(rjesup)
Comment on attachment 792909 [details] [diff] [review]
Patch

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

I wonder if we simply execute the test too quickly and continue with the next one, which most likely reloads the dom window? What happens if we add an additional timeout before the call to finish()? Does it fix the leak?

::: webapprt/test/chrome/browser_getUserMedia.js
@@ +10,5 @@
> +      win.addEventListener("load", function onLoadWindow() {
> +        win.removeEventListener("load", onLoadWindow, false);
> +        openedWindows++;
> +        if (openedWindows == 2) {
> +          ok(true, "Prompt shown.");

Also this is a bit vague. How can you be sure that the right window (popup) has been opened? I do not see any check for that. Also what's the first window here? Is it the main window? Sorry, but I don't know that much about the app runtime.
Attachment #792909 - Flags: feedback?(hskupin) → feedback+
(Assignee)

Comment 32

5 years ago
(In reply to Henrik Skupin (:whimboo) from comment #31)
> I wonder if we simply execute the test too quickly and continue with the
> next one, which most likely reloads the dom window? What happens if we add
> an additional timeout before the call to finish()? Does it fix the leak?

I've tried to set a 5 seconds timeout, but the leak is still there.

> Also this is a bit vague. How can you be sure that the right window (popup)
> has been opened? I do not see any check for that.

Yes, I didn't know how to test the contents of the prompt dialog, but I think this kind of test should be enough.
Whiteboard: DesktopWebRT2

Updated

5 years ago
Blocks: 919287
(Reporter)

Updated

5 years ago
No longer blocks: 919287
(Reporter)

Updated

5 years ago
Duplicate of this bug: 919287
(Assignee)

Comment 34

5 years ago
Needinfo ping?
marco: can you get a debug build with mediamanager:5,getusermedia:5 logging of the leak?  (plus the leak stats themselves).  If it only happens on tbpl/try, you can modify the mochitest python to change the default NSPR_LOG_MODULES settings.

I'm ok with re-using the flag, since it's no longer needed for the original purpose, but would like to be sure we're not papering over a real bug (for example leaking windows if it doesn't find any devices, or if the request is denied).
Flags: needinfo?(rjesup)
(Assignee)

Comment 36

5 years ago
Created attachment 815182 [details]
leak.log
(Assignee)

Comment 37

5 years ago
Created attachment 815183 [details]
nonleak.log
(Assignee)

Comment 38

5 years ago
I've just noticed that Firefox leaks too if you deny the request.
(Assignee)

Comment 39

5 years ago
Filed bug 925208 for Firefox.
(Assignee)

Comment 40

5 years ago
Created attachment 815480 [details] [diff] [review]
Patch

I've had to modify the patch because it was bitrotten.
Instead of using the flag, I've just added |skip-if:true|, to manually execute the test we just need to remove that line.
Attachment #792909 - Attachment is obsolete: true
Attachment #815480 - Flags: checkin?(myk)
Attachment #815480 - Flags: checkin?(myk) → checkin+
https://hg.mozilla.org/mozilla-central/rev/562abc4f545e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 27
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.