1311048 - Detect Firefox windows in window and application capture code.

Status: RESOLVED FIXED

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

Description

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Split out from bug 1284910, so I can land patches there.

Comment 1

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1311048 - Pick out Firefox windows as scary by pid (only OSX atm).

Review commit: https://reviewboard.mozilla.org/r/90014/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90014/

Comment 2

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Verified with NSPR_LOG_MODULES=CamerasParent:4

> [IPDL Background]: D/CamerasParent Returning Pictures name 6680 id (pid = 327)
> [IPDL Background]: D/CamerasParent Returning Activity Monitor (Windowed Processes) name 39 id (pid = 316)
> [IPDL Background]: D/CamerasParent Returning gUM screenshare window - JSFiddle name 8357 id (pid = 50341) (scary)
> [IPDL Background]: D/CamerasParent Returning Mozilla Developer Network name 8422 id (pid = 50341) (scary)

Comment 3

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8806615 [details]
Bug 1311048 - Pick out Firefox windows as scary by pid (only OSX atm).

https://reviewboard.mozilla.org/r/90014/#review89640

the pid_t stuff should at least be consistent.  posix says pid_t, windows says int (but it's a posix or maybe ISO c++ function).  Note that getpid() supposedly returns a deprecation warning on windows and they want you to use \_getpid(), which you could do.  Lots of other places in our system use getpid().

Note that scaryWindow = true tests things, kind-of - but please either elaborate on why this is being done in that DEBUG code, or remove this odd use of scaryWindow in DEBUG.

::: dom/media/webrtc/MediaEngineWebRTC.cpp:223
(Diff revision 1)
>        // TODO: Detect firefox windows
>        //scaryWindow = true;
> +      uint32_t pid = getpid();
> +      if (pid) {
> +        scaryWindow = true;
> +      }

File a bug, note number here.
Why "if (pid)"?  Note this is all debug code...  What's the reasoning for making everything scary in DEBUG (except if getpid() returns 0.......???)

::: dom/media/webrtc/MediaEngineWebRTC.cpp:232
(Diff revision 1)
>        // TODO: Detect firefox application windows
>        //scaryWindow = true;
>      }

Ditto here - file a bug, note here

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.h:26
(Diff revision 1)
> +  void setPid(const int pid);
>  
>    ScreenId getScreenId();
>    const char *getDeviceName();
>    const char *getUniqueIdName();
> +  int getPid();

const?

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.h:34
(Diff revision 1)
>  
>  protected:
>    ScreenId screenId_;
>    char* deviceNameUTF8_;
>    char* deviceUniqueIdUTF8_;
> +  int pid_;

pid_t -- you may want to check Windows; they deprecated getpid() for _getpid() (I can't find any support for their assertion it should be deprecated), but both on windows return int.  I suspect given other uses in the tree that pid_t is typedefed to int on windows.

::: media/webrtc/trunk/webrtc/modules/desktop_capture/desktop_device_info.cc:83
(Diff revision 1)
> +int DesktopDisplayDevice::getPid() {
> +  return pid_;

Ditto pid_t

::: media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer.h:32
(Diff revision 1)
>   public:
>    typedef webrtc::WindowId WindowId;
>  
>    struct Window {
>      WindowId id;
> +    int pid;

ditto

::: media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_mac.mm:128
(Diff revision 1)
>        if (layer != 0)
>          continue;
>  
>        int id;
>        CFNumberGetValue(window_id, kCFNumberIntType, &id);
> +      int pid = 0;

ditto

::: media/webrtc/trunk/webrtc/modules/video_capture/mac/avfoundation/video_capture_avfoundation_info.mm:58
(Diff revision 1)
>  int32_t VideoCaptureMacAVFoundationInfo::GetDeviceName(
>      uint32_t deviceNumber, char* deviceNameUTF8,
>      uint32_t deviceNameLength, char* deviceUniqueIdUTF8,
>      uint32_t deviceUniqueIdUTF8Length, char* productUniqueIdUTF8,
> -    uint32_t productUniqueIdUTF8Length)
> +    uint32_t productUniqueIdUTF8Length,
> +    int32_t* pid)

somehow it morphed to int32_t here

::: media/webrtc/trunk/webrtc/video_engine/browser_capture_impl.h:23
(Diff revision 1)
>                                    uint32_t deviceNameLength,
>                                    char* deviceUniqueIdUTF8,
>                                    uint32_t deviceUniqueIdUTF8Length,
>                                    char* productUniqueIdUTF8 = NULL,
> -                                  uint32_t productUniqueIdUTF8Length = 0) {
> +                                  uint32_t productUniqueIdUTF8Length = 0,
> +                                  int32_t* pid = 0) {

and here

Comment 4

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8806615 [details]
Bug 1311048 - Pick out Firefox windows as scary by pid (only OSX atm).

https://reviewboard.mozilla.org/r/90014/#review89640

> File a bug, note number here.
> Why "if (pid)"?  Note this is all debug code...  What's the reasoning for making everything scary in DEBUG (except if getpid() returns 0.......???)

Sorry, left-over debug cruft. I forgot to wire things all the way up! The downside of using low-level logging for testing. Will fix.

> Ditto here - file a bug, note here

Will be added in another patch here soon.

Comment 5

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8806615 [details]
Bug 1311048 - Pick out Firefox windows as scary by pid (only OSX atm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90014/diff/1-2/

Comment 6

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8806615 [details]
Bug 1311048 - Pick out Firefox windows as scary by pid (only OSX atm).

PTAL at comment 5.

Comment 7

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8806615 [details]
Bug 1311048 - Pick out Firefox windows as scary by pid (only OSX atm).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90014/diff/2-3/

Comment 8

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1311048 - Pick out Firefox windows as scary by pid on Windows.

Review commit: https://reviewboard.mozilla.org/r/90518/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90518/

Comment 9

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8807345 [details]
Bug 1311048 - Pick out Firefox windows as scary by pid on Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90518/diff/1-2/

Comment 10

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8807345 [details]
Bug 1311048 - Pick out Firefox windows as scary by pid on Windows.

https://reviewboard.mozilla.org/r/90518/#review90286

::: media/webrtc/trunk/webrtc/modules/desktop_capture/window_capturer_win.cc:66
(Diff revision 2)
>    WindowCapturer::Window window;
>    window.id = reinterpret_cast<WindowCapturer::WindowId>(hwnd);
>  
> +  DWORD pid;
> +  GetWindowThreadProcessId(hwnd, &pid);
> +  window.pid = pid;

Note that DWORD is unsigned; pid_t is signed.  perhaps make it explicit with a cast to pid_t

Comment 11

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1311048 - Pick out Firefox windows as scary by pid on Linux.

Review commit: https://reviewboard.mozilla.org/r/90846/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90846/

Comment 12

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1311048 - Make it compile on android.

Review commit: https://reviewboard.mozilla.org/r/90848/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90848/

Comment 13

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Attachment: Bug 1311048 - Surface process id in application sharing for completeness (we never share own process).

Review commit: https://reviewboard.mozilla.org/r/90850/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/90850/

Comment 14

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Comment on attachment 8807345 [details]
Bug 1311048 - Pick out Firefox windows as scary by pid on Windows.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/90518/diff/2-3/

Comment 15

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Also note re-review on second patch, comment 6 (it won't show up in mozReview), thanks!

Comment 16

[Jan-Ivar Bruaroey [:jib] (needinfo? me)]

Sorry, first patch.

Comment 17

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8806615 [details]
Bug 1311048 - Pick out Firefox windows as scary by pid (only OSX atm).

https://reviewboard.mozilla.org/r/90014/#review90584

re-r+ if I haven't already

Comment 18

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8807821 [details]
Bug 1311048 - Pick out Firefox windows as scary by pid on Linux.

https://reviewboard.mozilla.org/r/90846/#review90586

Comment 19

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8807822 [details]
Bug 1311048 - Make it compile on android.

https://reviewboard.mozilla.org/r/90848/#review90588

Comment 20

[Randell Jesup [:jesup] (needinfo me)]

Comment on attachment 8807823 [details]
Bug 1311048 - Surface process id in application sharing for completeness (we never share own process).

https://reviewboard.mozilla.org/r/90850/#review90590

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/5b9dacd187bb
Pick out Firefox windows as scary by pid (only OSX atm). r=jesup
https://hg.mozilla.org/integration/autoland/rev/533d7494d9ca
Pick out Firefox windows as scary by pid on Windows. r=jesup
https://hg.mozilla.org/integration/autoland/rev/bec62243bf7e
Pick out Firefox windows as scary by pid on Linux. r=jesup
https://hg.mozilla.org/integration/autoland/rev/8da036418f24
Make it compile on android. r=jesup
https://hg.mozilla.org/integration/autoland/rev/b8f2d6428e38
Surface process id in application sharing for completeness (we never share own process). r=jesup

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5b9dacd187bb
https://hg.mozilla.org/mozilla-central/rev/533d7494d9ca
https://hg.mozilla.org/mozilla-central/rev/bec62243bf7e
https://hg.mozilla.org/mozilla-central/rev/8da036418f24
https://hg.mozilla.org/mozilla-central/rev/b8f2d6428e38