New issue
Advanced search Search tips

Issue 888772 link

Starred by 1 user
Status: Assigned
Owner:
fdoray@chromium.org
Cc:
etiennep@chromium.org
liber...@chromium.org
gab@chromium.org
dcheng@chromium.org
fdoray@chromium.org
robliao@chromium.org
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

ScopedAllowBaseSyncPrimitives doesn't play nicely with non-task scheduler runners

Project Member Reported by liber...@chromium.org, Sep 24 
context: https://chromium-review.googlesource.com/c/chromium/src/+/1220562/8/third_party/blink/renderer/platform/graphics/video_frame_resource_provider.cc

While writing code that sometimes runs on a TaskRunner from the task scheduler that's marked as MayBlock (TaskRunner "A"), and other times on a SingleThreadTaskRunner directly from a Thread (TaskRunner "B"), use of blocking primitives got a little weird.

In particular, TaskRunner A was marked as MayBlock.  So, on that runner the code should use ScopedAllowBaseSyncPrimitives during blocking.

However, when that same code runs on TaskRunner B, the ScopedAllow dchecks that it's outside of a MayBlock task runner.  Of course, it wouldn't require any ScopedAllow before blocking, if it only ran on TaskRunner B.  But, it has to work on both.

The best solution I came up with was to use ScopedAllowBaseSyncPrimitivesOutsideBlockingScope.  It works on TaskRunner A, even though it's MayBlock.  It also works on B, since it doesn't care that it's not MayBlock.

It all seems a little weird to me, though, since TaskRunner B seems like it's no less "MayBlock" than TaskRunner A, but it requires me to use the stronger primitive.

dcheng@ suggested that you would be the right person to file this with.

thanks!

Comment 1 by gab@chromium.org, Sep 25

Cc: fdoray@chromium.org etiennep@chromium.org gab@chromium.org
Owner: fdoray@chromium.org
Hmmmm, the issue seems to be that the thread you're on (CC IIUC) disallows blocking but not waiting? Since waiting is a subset of blocking (it's the same thing for scheduling but we strongly dislike waiting so we're stricter) that's a weird semantic for a thread to have and in fact we should probably auto-disallow waiting from base::DisallowBlocking(). We just introduced base::DisallowUnresponsiveTasks() as a catch all used by most threads at this point, I guess we missed the CC thread...

It's surprising to me that you consider it ok to wait on a thread that doesn't allow blocking..?

Comment 2 by liber...@chromium.org, Sep 25 

it's not something that i'm introducing.  the same code has run in VideoResourceUpdater on the cc thread for likely years.  i don't remember off-hand where the wait is, but i do remember it has a todo... :(

Comment 3 by gab@chromium.org, Sep 25 

Hmmm ok so cc thread bans blocking but not waiting though? And the problem is that for task scheduler threads you have to (as desired) explicitly allow waiting. And now that's not allow in a scope that bans blocking so you need the full on ScopedAllowBaseSyncPrimitivesOutsideBlockingScope hammer?

FWIW, if cc thread banned blocking & waiting already (as I think it should), this code would have needed ScopedAllowBaseSyncPrimitivesOutsideBlockingScope already.
►

Sign in to add a comment