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

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jwwang, Assigned: jwwang)

Tracking

(Blocks 1 bug)

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment)

Assignee

Description

2 years ago
See bug 1365483 for the rationale.
Comment hidden (mozreview-request)
Assignee

Updated

2 years ago
Assignee: nobody → jwwang
Assignee

Updated

2 years ago
Blocks: 1365483

Comment 2

2 years ago
mozreview-review
Comment on attachment 8868410 [details]
Bug 1365504 - Remove the call to AbstractThread::GetCurrent() in ServiceWorkerUpdaterChild.cpp.

https://reviewboard.mozilla.org/r/140004/#review143474

::: dom/workers/ServiceWorkerUpdaterChild.cpp:23
(Diff revision 1)
>  {
>    MOZ_ASSERT(aPromise);
>    MOZ_ASSERT(aSuccessRunnable);
>    MOZ_ASSERT(aFailureRunnable);
>  
> -  aPromise->Then(AbstractThread::GetCurrent(), __func__,
> +  aPromise->Then(AbstractThread::MainThread(), __func__,

This ia a PBackground actor which means its possible to use off the main thread.  Currently we don't, but its not obvious from looking at this file.

Please add a MOZ_ASSERT(NS_IsMainThread()) here to note the restriction you are introducing.

By the way, this is exactly the kind of change I meant when I said it becomes more difficult to write code that works on either main thread or worker threads.

I still find it ridiculous we can't have a "current thread" concept in the system.
Attachment #8868410 - Flags: review?(bkelly) → review+
Assignee

Comment 3

2 years ago
mozreview-review
Comment on attachment 8868410 [details]
Bug 1365504 - Remove the call to AbstractThread::GetCurrent() in ServiceWorkerUpdaterChild.cpp.

https://reviewboard.mozilla.org/r/140004/#review143736

::: dom/workers/ServiceWorkerUpdaterChild.cpp:23
(Diff revision 1)
>  {
>    MOZ_ASSERT(aPromise);
>    MOZ_ASSERT(aSuccessRunnable);
>    MOZ_ASSERT(aFailureRunnable);
>  
> -  aPromise->Then(AbstractThread::GetCurrent(), __func__,
> +  aPromise->Then(AbstractThread::MainThread(), __func__,

We can use AbstractThread::CreateXPCOMThreadWrapper(NS_GetCurrentThread(), false) which gives you an AbstractThread wrapping around the current nsIThread.

However, AbstractThread::CreateXPCOMThreadWrapper is broken for now (see bug 1365483 comment 10).

So I will use AbstractThread::MainThread() now here, and change it later after bug 1364821 is fixed.
Comment hidden (mozreview-request)
The MainThread() is fine.  I don't think we should require allocation overhead for something like this.

Comment 6

2 years ago
Pushed by jwwang@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1afba65a7758
Remove the call to AbstractThread::GetCurrent() in ServiceWorkerUpdaterChild.cpp. r=bkelly

Comment 7

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1afba65a7758
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.