1402818 - Assertion failed: IsGUIThread(false) in mouse_cursor_monitor_win.cc

Status: RESOLVED FIXED

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

Description

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

See https://treeherder.mozilla.org/#/jobs?repo=try&revision=b1cda02d4f1fae6c85e96f82ac13690299fca3c1

Or, if that is gone when you load it, enable test_getUserMedia_basicScreenshare.html on windows debug and run on try.

This started happening with the changes done to that test in bug 1380346.

Comment 1

[Dan Minor [:dminor]]

This assertion was added in the branch 57 update, check blame at [1] and it is not present in upstream webrtc.org [2] as the IsGUIThread checks are our code. I'm going to suggest that this was added by accident, when we were adding the IsGUIThread to the methods that require them.

[1] http://searchfox.org/mozilla-central/rev/f6dc0e40b51a37c34e1683865395e72e7fca592c/media/webrtc/trunk/webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc#112
[2] https://cs.chromium.org/chromium/src/third_party/webrtc/modules/desktop_capture/mouse_cursor_monitor_win.cc?q=MouseCursorMonitorWin&sq=package:chromium&l=102

Comment 2

[Dan Minor [:dminor]]

Attachment: Remove IsGUIThread() assertion from Capture()

Comment 3

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

Here's a try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=106483ae106cfe7b3b3dd1d25b6575a24a8767ac

I just wrote the patch myself for try, but content is the same.

Comment 4

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

(In reply to Dan Minor [:dminor] from comment #1)
> This assertion was added in the branch 57 update, check blame at [1] and it
> is not present in upstream webrtc.org [2] as the IsGUIThread checks are our
> code. I'm going to suggest that this was added by accident, when we were
> adding the IsGUIThread to the methods that require them.

We added a bunch of IsGUIThread() assertions after fixing a problem with not being on a UI thread in bug 1060738 (jimm), including this one.  You're just seeing the changes we're carrying being merged forward; if a change is part of the "rollup" patch, it likely was there before the update.  Check out the changeset before the webrtc.org landing to verify.

The really odd thing is that this was called from PlatformUIThread::Run().... 8-/ ??!

Comment 5

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

Comment on attachment 8911772 [details] [diff] [review]
Remove IsGUIThread() assertion from Capture()

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

This was added on purpose, and we *should* be running on a UI thread unless that class is entirely broken in the webrtc.org 57 update.  Asserting is right, I believe.  If PlatformUIThread isn't a UI thread, we need to make it one ASAP.

Comment 6

[Dan Minor [:dminor]]

> We added a bunch of IsGUIThread() assertions after fixing a problem with not
> being on a UI thread in bug 1060738 (jimm), including this one.  You're just
> seeing the changes we're carrying being merged forward; if a change is part
> of the "rollup" patch, it likely was there before the update.  Check out the
> changeset before the webrtc.org landing to verify.
> 
Yes, you're right and I should know better, I've been bitten by this before.

Comment 7

[Dan Minor [:dminor]]

After the call to ApplyConstraints() the same thread which previously was running ok fails the IsGUIThread() assertion the very next time Capture() is called. I see garbage values in the local variables in the debugger, so maybe we're failing to stop the thread after some data is freed, I'm not certain yet.

At any rate, it looks like the PlatformUIThread is a UI thread right up to the point after ApplyConstraints is called.

Comment 8

[Dan Minor [:dminor]]

Attachment: Set hwnd_ to NULL in Stop()

Comment 9

[Dan Minor [:dminor]]

Try job: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af6f5f53694fc4be6d32d9aaaf53015db6b2a294

Comment 10

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

Comment on attachment 8913396 [details] [diff] [review]
Set hwnd_ to NULL in Stop()

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

This makes sense, thanks! Can you uplift this to webrtc.org?

Comment 11

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

Try push with bug 1380346 which triggered this before: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b73790aacbe7c3c7a59babbc57d8d8d262daf022

Comment 12

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

2nd attempt: https://treeherder.mozilla.org/#/jobs?repo=try&revision=c7a237b0925667fe2b71e13b4b92b87bdc056308

Comment 13

[Dan Minor [:dminor]]

(In reply to Andreas Pehrson [:pehrsons] from comment #10)
> Comment on attachment 8913396 [details] [diff] [review]
> Set hwnd_ to NULL in Stop()
> 
> Review of attachment 8913396 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This makes sense, thanks! Can you uplift this to webrtc.org?

Thanks! Unless I'm misreading blame for the second time on this bug :) PlatformUIThread is our code and does not exist upstream. I guess at some point we should try to either upstream the whole thing or determine what we're doing differently that we require the changes.

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/835180921387
Set hwnd_ to NULL in PlatformUIThread::Stop(); r=pehrsons

Comment 15

[Andreas Pehrson [:pehrsons] (PTO; back in Aug)]

(In reply to Dan Minor [:dminor] from comment #13)
> (In reply to Andreas Pehrson [:pehrsons] from comment #10)
> > Comment on attachment 8913396 [details] [diff] [review]
> > Set hwnd_ to NULL in Stop()
> > 
> > Review of attachment 8913396 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > This makes sense, thanks! Can you uplift this to webrtc.org?
> 
> Thanks! Unless I'm misreading blame for the second time on this bug :)
> PlatformUIThread is our code and does not exist upstream. I guess at some
> point we should try to either upstream the whole thing or determine what
> we're doing differently that we require the changes.

Right, thanks for checking that!

Comment 16

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/835180921387

Comment 17

[Sylvestre Ledru [:Sylvestre]]

Do we want to uplift that to 57?

Comment 18

[Dan Minor [:dminor]]

Comment on attachment 8913396 [details] [diff] [review]
Set hwnd_ to NULL in Stop()

Approval Request Comment
[Feature/Bug causing the regression]:
This problem was present when this code was initially landed in Bug 1060738.
[User impact if declined]:
Running code that accesses UI functionality on Windows from a non-UI thread can lead to crashes. Without this fix, if Stop() and then Start() is called on the thread, what was previously a UI thread will no longer be one.
[Is this code covered by automated tests?]:
Yes, this issue was discovered by modifying one of our screen sharing unit tests.
[Has the fix been verified in Nightly?]:
Yes.
[Needs manual test from QE? If yes, steps to reproduce]: 
No.
[List of other uplifts needed for the feature/fix]:
None.
[Is the change risky?]:
No.
[Why is the change risky/not risky?]:
This just resets a variable so that initialization will happen properly if we call Stop() then Start().
[String changes made/needed]:
None.

Comment 19

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8913396 [details] [diff] [review]
Set hwnd_ to NULL in Stop()

This is not a new regression, recommend letting this fix ride the 58 train.