1365513 - Remove the call to AbstractThread::GetCurrent() in nsPermissionManager.cpp

Status: RESOLVED FIXED

(Core :: DOM: Service Workers, enhancement)

Description

[JW Wang [:jwwang] [:jw_wang]]

See bug 1365483 for the rationale.

Comment 1

[JW Wang [:jwwang] [:jw_wang]]

http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/extensions/cookie/nsPermissionManager.cpp#3259

Hi Michael,
Does WhenPermissionsAvailable() run on the main thread? Is it OK to replace AbstractThread::GetCurrent() with AbstractThread::MainThread()?

Comment 2

[Nika Layzell [:nika] (ni? for response)]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #1)
> http://searchfox.org/mozilla-central/rev/
> 9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/extensions/cookie/
> nsPermissionManager.cpp#3259
> 
> Hi Michael,
> Does WhenPermissionsAvailable() run on the main thread? Is it OK to replace
> AbstractThread::GetCurrent() with AbstractThread::MainThread()?

Everything in nsPermissionManager is main-thread bound, however, we do want to label these runnables correctly. 

Could we make every one of the promises run on SystemGroup::AbstractMainThreadFor(TaskCategory::Other), except for the final call to the runnable passed to WhenPermissionsAvaliable which should dispatch to either `AbstractThread::MainThread()` or dispatch to a passed-in event target.

Comment 3

[JW Wang [:jwwang] [:jw_wang]]

(In reply to Michael Layzell [:mystor] from comment #2)
> Could we make every one of the promises run on
> SystemGroup::AbstractMainThreadFor(TaskCategory::Other), except for the
> final call to the runnable passed to WhenPermissionsAvaliable

Do you refer to |runnable->Run()| at #3261?
http://searchfox.org/mozilla-central/rev/9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/extensions/cookie/nsPermissionManager.cpp#3261

> which should
> dispatch to either `AbstractThread::MainThread()` or dispatch to a passed-in
> event target.

I can't find the 'passed-in event target' in WhenPermissionsAvailable().

Comment 4

[Nika Layzell [:nika] (ni? for response)]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #3)
> (In reply to Michael Layzell [:mystor] from comment #2)
> > Could we make every one of the promises run on
> > SystemGroup::AbstractMainThreadFor(TaskCategory::Other), except for the
> > final call to the runnable passed to WhenPermissionsAvaliable
> 
> Do you refer to |runnable->Run()| at #3261?
> http://searchfox.org/mozilla-central/rev/
> 9a7fbdee1d54f99cd548af95b81231d80e5f9ad1/extensions/cookie/
> nsPermissionManager.cpp#3261
> 
> > which should
> > dispatch to either `AbstractThread::MainThread()` or dispatch to a passed-in
> > event target.
> 
> I can't find the 'passed-in event target' in WhenPermissionsAvailable().

You would have to add it as a new argument to WhenPermissionsAvaliable. For now I would just change the call to Run() that you pointed out above to instead NS_DispatchToMainThread, and use SystemGroup::AbstractMainThreadFor(TaskCategory::Other) everywhere else.

We can improve the labeling of that last dispatch later. It should be very very uncommon (only used right now if the main thread in the parent process is really slow and we're performing in-content fetch interception).

Comment 5

[JW Wang [:jwwang] [:jw_wang]]

Attachment: Bug 1365513 - Remove the call to AbstractThread::GetCurrent() in nsPermissionManager.cpp.

See bug 1365483 for the rationale.

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

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Comment on attachment 8868889 [details]
Bug 1365513 - Remove the call to AbstractThread::GetCurrent() in nsPermissionManager.cpp.

https://reviewboard.mozilla.org/r/140510/#review144104

::: extensions/cookie/nsPermissionManager.cpp:3259
(Diff revision 1)
>    if (promises.IsEmpty()) {
>      aRunnable->Run();
>      return NS_OK;
>    }
>  
> +  auto* thread = SystemGroup::AbstractMainThreadFor(TaskCategory::Other);

Can you add a comment to nsIPermissionManager::whenPermissionsAvaliable in the idl file explaining that if permissions are not avaliable, the runnable will be dispatched to the SystemGroup?

Comment 7

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8868889 [details]
Bug 1365513 - Remove the call to AbstractThread::GetCurrent() in nsPermissionManager.cpp.

https://reviewboard.mozilla.org/r/140510/#review144104

> Can you add a comment to nsIPermissionManager::whenPermissionsAvaliable in the idl file explaining that if permissions are not avaliable, the runnable will be dispatched to the SystemGroup?

Not sure I get it. If any of the permissions is not available, the All promise will be rejected and |runnable| will not run at all, right?

Comment 8

[Nika Layzell [:nika] (ni? for response)]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #7)
> Comment on attachment 8868889 [details]
> Bug 1365513 - Remove the call to AbstractThread::GetCurrent() in
> nsPermissionManager.cpp.
> 
> https://reviewboard.mozilla.org/r/140510/#review144104
> 
> > Can you add a comment to nsIPermissionManager::whenPermissionsAvaliable in the idl file explaining that if permissions are not avaliable, the runnable will be dispatched to the SystemGroup?
> 
> Not sure I get it. If any of the permissions is not available, the All
> promise will be rejected and |runnable| will not run at all, right?

WhenPermissionsAvaliable will either:

a) If the current content process already has the given permissions available, it will synchronously invoke the runnable passed in

b) If the current content process does not have the given permissions available, it will await a promise, which will be fulfilled when the promises become available

c) If the browser shuts down, destroying the nsPermissionManager before the permissions become available, the promise will be rejected, and the runnable will not be run.

The comment which I wanted you to note was that in case b) the runnable will be run from within the SystemGroup.

Comment 9

[JW Wang [:jwwang] [:jw_wang]]

Comment on attachment 8868889 [details]
Bug 1365513 - Remove the call to AbstractThread::GetCurrent() in nsPermissionManager.cpp.

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/9ea2cf9ede5e
Remove the call to AbstractThread::GetCurrent() in nsPermissionManager.cpp. r=mystor

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9ea2cf9ede5e