1313989 - Get rid of the MutexAutoUnlock in nsThread::GetIdleEvent

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect, P3)

Description

[Andreas Farre [:farre]]

It would be better if we didn't release the lock while expecting events.

This also means that nsIIdlePeriod::GetIdlePeriodHint never should post runnables to the current thread.

Comment 1

[Andreas Farre [:farre]]

Attachment: Bug 1313989 - Remove MutexAutoUnlock in nsThread::GetIdleEvent.

Rewrite VsyncRefreshDriverTimer::GetTimerRate to always use the cached
value of the vsync rate in VsyncChild to avoid processing events on
the main thread.

Since VsyncChild::GetTimerRate is called in VsyncRefreshDriverTimer's
constructor, that cached value is bound to be set soon. This should
make the period of time we need to guess in
VsyncRefreshDriverTimer::GetTimerRate very short.

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

Comment 2

[Nathan Froyd [:froydnj]]

Comment on attachment 8809317 [details]
Bug 1313989 - Remove MutexAutoUnlock in nsThread::GetIdleEvent.

https://reviewboard.mozilla.org/r/91906/#review91948

r=me on the XPCOM parts.  Is there some assert we can stick somewhere so doing IPC in situations like this would result in a crash rather than a deadlock?

::: layout/ipc/VsyncChild.h:41
(Diff revision 1)
>    TimeDuration GetVsyncRate();
> +  TimeDuration VsyncRate();

I don't know what Matt thinks, but I think there ought to be some comment explaining why we have two different functions that appear by their names to do the same thing.  The comment in nsRefreshDriver is helpful, but some of it surely belongs here as well.

Comment 3

[Andreas Farre [:farre]]

Comment on attachment 8809317 [details]
Bug 1313989 - Remove MutexAutoUnlock in nsThread::GetIdleEvent.

https://reviewboard.mozilla.org/r/91906/#review91948

I guess that something like the asserts in the beginning of: https://dxr.mozilla.org/mozilla-central/source/nsprpub/pr/src/pthreads/ptsynch.c#198-201 but inverted.

That is, if the current thread owns the lock and we're trying to grab it, then assert?

> I don't know what Matt thinks, but I think there ought to be some comment explaining why we have two different functions that appear by their names to do the same thing.  The comment in nsRefreshDriver is helpful, but some of it surely belongs here as well.

Definitely. Will do.

Comment 4

[Andreas Farre [:farre]]

Comment on attachment 8809317 [details]
Bug 1313989 - Remove MutexAutoUnlock in nsThread::GetIdleEvent.

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

Comment 5

[Matt Woodrow (:mattwoodrow)]

Comment on attachment 8809317 [details]
Bug 1313989 - Remove MutexAutoUnlock in nsThread::GetIdleEvent.

https://reviewboard.mozilla.org/r/91906/#review92108

Comment 6

[Andreas Farre [:farre]]

Comment on attachment 8809317 [details]
Bug 1313989 - Remove MutexAutoUnlock in nsThread::GetIdleEvent.

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

Comment 7

[Andreas Farre [:farre]]

Comment on attachment 8809317 [details]
Bug 1313989 - Remove MutexAutoUnlock in nsThread::GetIdleEvent.

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

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/188b07c3d90a
Remove MutexAutoUnlock in nsThread::GetIdleEvent. r=froydnj,mattwoodrow

Comment 9

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

https://hg.mozilla.org/mozilla-central/rev/188b07c3d90a