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

RESOLVED FIXED in Firefox 55

Status

()

RESOLVED FIXED
a year ago
a year ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Blocks: 1 bug)

unspecified
mozilla55
Points:
---

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

(Assignee)

Description

a year ago
See bug 1365483 for the rationale.
(Assignee)

Comment 1

a year ago
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()?
Blocks: 1365483
Flags: needinfo?(michael)

Comment 2

a year ago
(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.
Flags: needinfo?(michael)
(Assignee)

Comment 3

a year ago
(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

a year ago
(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 hidden (mozreview-request)

Comment 6

a year ago
mozreview-review
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?
Attachment #8868889 - Flags: review?(michael) → review+
(Assignee)

Comment 7

a year ago
mozreview-review-reply
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?
Assignee: nobody → jwwang

Comment 8

a year ago
(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 hidden (mozreview-request)

Comment 10

a year ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/9ea2cf9ede5e
Remove the call to AbstractThread::GetCurrent() in nsPermissionManager.cpp. r=mystor

Comment 11

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9ea2cf9ede5e
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.