1453740 - Crash when shared window is minimized

Status: VERIFIED FIXED

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

Description

[Dan Minor [:dminor]]

From https://bugzilla.mozilla.org/show_bug.cgi?id=1432793#c40:

Precondition:
Make sure a folder is opened in background and not minimized (it will be needed later)

Steps:
1. Start Firefox.
2. Login to hangouts.google.com.
3. Go to meet.google.com (you have to be logged in with mozilla ldap account, or else it will say that the page is unsupported on Firefox).
4. Click on Start a new meeting.
5. Click on Start Meeting.
6. Click on Present Now and select a Window.
7. The folder from step 1 will be selected for screenshare.
8. After the screenshare started, minimize the folder.

On Ubuntu 16.04 (x64) this issue is not reproducible.

Here is a crash log from macOS: https://crash-stats.mozilla.com/report/index/04613a3b-74fd-4a96-8870-f21120180412#tab-details

Bug 1450658 says that Chrome always brings a window to the front when it is shared. I'm not sure if it also prevents it from being minimized.

The problem is that a minimized window has a resolution of 1x1 pixels, which causes the check here [1] to fail.

[1] https://searchfox.org/mozilla-central/rev/4114ad2cfcbc511705c7865a4a34741812f9a2a9/media/webrtc/trunk/webrtc/modules/video_coding/codecs/vp8/vp8_impl.cc#303

Comment 1

[Dan Minor [:dminor]]

Attachment: Bug 1453740 - Allow 1x1 windows in VP8EncoderImpl::InitEncode;

A minimized window has a resolution of 1x1. Although we removed minimized windows from the list
of available windows to share, nothing prevents the user from minimizing it during a call. With
the current code, this will cause InitEncode to fail, resulting in a fatal release assert.

I tested this patch with window sharing on meet.google.com and I was able to minimize and restore
the window several times without problem. While minimized, the window appears as a black screen
to the other meeting participants. It renders normally when restored.

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

Comment 2

[Andreas Pehrson [:pehrsons]]

Comment on attachment 8967475 [details]
Bug 1453740 - Allow 1x1 windows in VP8EncoderImpl::InitEncode;

https://reviewboard.mozilla.org/r/236144/#review242178

What's the upstream status of the same issue? Have they fixed it the same way?

Comment 3

[Dan Minor [:dminor]]

Their vp8 codec code is unchanged [1]. I tested Chrome, and the behaviour is the same as what we have with this patch, the screen goes black when the window is minimized.

[1] https://cs.chromium.org/chromium/src/third_party/webrtc/modules/video_coding/codecs/vp8/libvpx_vp8_encoder.cc?q=ValidSimu&sq=package:chromium&l=359

Comment 4

[Dan Minor [:dminor]]

For what it's worth, we have a release assertion where they have a debug assertion, so it's possible that their codec init is failing silently, and the end result, a black screen, is the same in both cases.

Comment 5

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1cece7d5df17
Allow 1x1 windows in VP8EncoderImpl::InitEncode; r=pehrsons

Comment 6

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/1cece7d5df17

Comment 7

[Dan Minor [:dminor]]

Comment on attachment 8967475 [details]
Bug 1453740 - Allow 1x1 windows in VP8EncoderImpl::InitEncode;

Approval Request Comment
[Feature/Bug causing the regression]:
I think this has been present since screensharing was implemented, a few months ago we switch a debug assert to a release assert and have since caught problems with screensharing.
[User impact if declined]:
Crashes if the shared window is minimized. This also fixes a crash if the shared window is maximized on OS X (Bug 1452561).
[Is this code covered by automated tests?]:
No.
[Has the fix been verified in Nightly?]:
Has landed in Nightly, has not been fixed.
[Needs manual test from QE? If yes, steps to reproduce]: 
Yes please. Comment 1 has the steps to reproduce.
[List of other uplifts needed for the feature/fix]:
None.
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
This only affects windows which are 1x1 pixel in size. We used to crash in that case anyway.
[String changes made/needed]:
None.

Comment 8

[Catalin Sasca, Desktop QA [:csasca]]

I have reproduced the issue on Nightly (2018-04-12) under Windows 10 (x64) using STR from Comment 0.

The issue is fixed on latest Nightly (2018-04-16). Tests were performed on Windows 10 (x64), Ubuntu 16.04 (x64) and macOS 10.12.

Comment 9

[Dan Minor [:dminor]]

I should have mentioned in my uplift request that this bug affects Google hangouts / meet. We're hoping to get Firefox support for hangouts / meet enabled for 60, so this bug also impacts that.

Comment 10

[Julien Cristau [:jcristau]]

Comment on attachment 8967475 [details]
Bug 1453740 - Allow 1x1 windows in VP8EncoderImpl::InitEncode;

fix a crash when minimizing a shared window on google meet, approved for 60.0b13

Comment 12

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/f621f04ecd45

Comment 13

[Catalin Sasca, Desktop QA [:csasca]]

The issue is fixed on Firefox 60.0b13 under Windows 10 (x64), Ubuntu 16.04 (x64) and macOS 10.11.