Remove the call to AbstractThread::GetCurrent() in ServiceWorkerUpdaterChild.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

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

(Assignee)

Description

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

Updated

a year ago
Assignee: nobody → jwwang
(Assignee)

Updated

a year ago
Blocks: 1365483

Comment 2

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

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

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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/1afba65a7758
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.