Make DOMRequest and DOMCursor accessible on workers

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
2 months ago

People

(Reporter: aosmond, Assigned: aosmond)

Tracking

({dev-doc-complete})

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

(Whiteboard: [NG Device Storage])

Attachments

(1 attachment, 3 obsolete attachments)

Assignee

Description

4 years ago
DOMRequest is currently only accessible on the main thread. It be supported by workers we must:

1) Allow it to be constructed using nsIGlobalObject* (fed into DOMEventTargetHelper) in addition to nsPIDOMWindow*. The underlying Promise needs that to get a JS context and check state.

2) DOMRequestService::FireSuccessAsync (FireSuccessAsyncTask by extension) and DOMRequestService::FireErrorAsync must be changed to dispatch to the current thread and remove the assert checks for main thread. mozilla::ThreadsafeAutoSafeJSContext should be switched for AutoJSAPI and Init(nsIGlobalObject*) (gotten from DOMRequest::GetParentObject); this will assert that the current thread is owning thread.

3) Add Exposed=(Window,Worker) to the WebIDL definition.
Assignee

Updated

4 years ago
Assignee: nobody → aosmond
Status: NEW → ASSIGNED
Assignee

Comment 1

4 years ago
Posted patch bug1167650.patch, v1 (obsolete) — Splinter Review
In order to support device storage with workers, DOMRequest needs to updated as well until such a time we switch to promises.
Attachment #8609494 - Flags: review?(jonas)
Assignee

Comment 2

4 years ago
Comment on attachment 8609494 [details] [diff] [review]
bug1167650.patch, v1

Review of attachment 8609494 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMRequest.cpp
@@ +384,5 @@
>  {
>    NS_ENSURE_STATE(aRequest);
>    nsCOMPtr<nsIRunnable> asyncTask =
>      new FireErrorAsyncTask(static_cast<DOMRequest*>(aRequest), aError);
> +  if (NS_WARN_IF(NS_FAILED(NS_DispatchToCurrentThread(asyncTask)))) {

I looked at the native users of FireErrorAsync and it appears they are all running on the main thread at present. I think the original implementation would work if you called it on another thread because it doesn't do anything that requires the main thread (except perhaps if the ref counting for DOMRequest isn't thread safe...), and if so this patch is more restrictive than before.
Assignee

Updated

4 years ago
Whiteboard: [NG Device Storage]
Last I checked, DOMRequest needed to be killed with fire.
Assignee

Comment 4

4 years ago
Posted patch bug1167650.patch, v2 (obsolete) — Splinter Review
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8264051daff2

(In reply to :Ms2ger from comment #3)
> Last I checked, DOMRequest needed to be killed with fire.

One day soon hopefully :). I'm not entirely sure what to do about all of the existing users of our APIs who assume a DOMRequest (as opposed to using the .then promise-like function provided). Presumably there are apps outside of Gaia which use device storage...
Attachment #8609494 - Attachment is obsolete: true
Attachment #8609494 - Flags: review?(jonas)
Attachment #8612548 - Flags: review?(jonas)
Assignee

Updated

4 years ago
Summary: Make DOMRequest accessible on workers → Make DOMRequest and DOMCursor accessible on workers
Assignee

Updated

4 years ago
Blocks: 1171170
Comment on attachment 8612548 [details] [diff] [review]
bug1167650.patch, v2

Review of attachment 8612548 [details] [diff] [review]:
-----------------------------------------------------------------

It's probably better for bent to review this.

I definitely agree that we should do this though. I just don't feel comfortable enough with worker details to r+.
Attachment #8612548 - Flags: review?(jonas) → review?(bent.mozilla)
(In reply to Andrew Osmond [:aosmond] from comment #4)
> (In reply to :Ms2ger from comment #3)
> > Last I checked, DOMRequest needed to be killed with fire.
> 
> One day soon hopefully :). I'm not entirely sure what to do about all of the
> existing users of our APIs who assume a DOMRequest (as opposed to using the
> .then promise-like function provided). Presumably there are apps outside of
> Gaia which use device storage...

Device Storage isn't exposed to anything other than privileged/certified apps but yes, there could be users outside of Gaia.
We should definitely not spend engineering effort on "cleaning up" the DeviceStorage API. There are no plans to expose it beyond FirefoxOS specific signed content, so I don't see that this affects general web content in any way. It's no more a problem than that we have some chrome APIs which use callbacks rather than promises.
Comment on attachment 8612548 [details] [diff] [review]
bug1167650.patch, v2

Review of attachment 8612548 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/DOMRequest.cpp
@@ +250,5 @@
>  }
>  
>  NS_IMETHODIMP
> +DOMRequestService::CreateWorkerRequest(nsIGlobalObject* aGlobal,
> +                                       nsIDOMDOMRequest** aRequest)

Nit: I think it would be wise to |MOZ_ASSERT(!NS_IsMainThread())| here, and |MOZ_ASSERT(NS_IsMainThread())| in DOMRequestService::CreateRequest().

@@ +325,5 @@
>    Dispatch(DOMRequest* aRequest,
>             const JS::Value& aResult)
>    {
> +    AutoJSAPI jsapi;
> +    if (NS_WARN_IF(!jsapi.Init(aRequest->GetOwnerGlobal()))) {

Hm, I don't think this change is necessary. Why did you do this?
Comment on attachment 8612548 [details] [diff] [review]
bug1167650.patch, v2

Canceling review to get questions addressed. Please re-request whenever.
Attachment #8612548 - Flags: review?(bent.mozilla)
Assignee

Comment 10

4 years ago
Posted patch bug1167650.patch, v3 (obsolete) — Splinter Review
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #8)
> Comment on attachment 8612548 [details] [diff] [review]
> bug1167650.patch, v2
> 
> Review of attachment 8612548 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/base/DOMRequest.cpp
> @@ +250,5 @@
> >  }
> >  
> >  NS_IMETHODIMP
> > +DOMRequestService::CreateWorkerRequest(nsIGlobalObject* aGlobal,
> > +                                       nsIDOMDOMRequest** aRequest)
> 
> Nit: I think it would be wise to |MOZ_ASSERT(!NS_IsMainThread())| here, and
> |MOZ_ASSERT(NS_IsMainThread())| in DOMRequestService::CreateRequest().

I added the assert to CreateRequest but removed CreateWorkerRequest. I checked and nobody seems to use the create methods exposed by DOMRequestService, so the worker addition probably isn't necessary...

> 
> @@ +325,5 @@
> >    Dispatch(DOMRequest* aRequest,
> >             const JS::Value& aResult)
> >    {
> > +    AutoJSAPI jsapi;
> > +    if (NS_WARN_IF(!jsapi.Init(aRequest->GetOwnerGlobal()))) {
> 
> Hm, I don't think this change is necessary. Why did you do this?

For some reason I was under the impression that AutoJSAPI::Init explicitly verified the owning thread of nsIGlobalObject is the current thread, either as an assert or returning an error code (rather than just trusting we are on the right thread like ThreadsafeAutoSafeJSContext). But looking at the call flow again, that doesn't appear to be the case. I've removed this change.
Attachment #8612548 - Attachment is obsolete: true
Attachment #8626725 - Flags: review?(bent.mozilla)
Comment on attachment 8626725 [details] [diff] [review]
bug1167650.patch, v3

Review of attachment 8626725 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good! Thanks.

::: dom/base/DOMRequest.cpp
@@ +317,3 @@
>      mozilla::ThreadsafeAutoSafeJSContext cx;
>      nsRefPtr<FireSuccessAsyncTask> asyncTask = new FireSuccessAsyncTask(cx, aRequest, aResult);
> +    if (NS_WARN_IF(NS_FAILED(NS_DispatchToCurrentThread(asyncTask)))) {

NS_DispatchToCurrentThread should really never fail... How about just:

  MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToCurrentThread(asyncTask)));

@@ +369,5 @@
>  {
>    NS_ENSURE_STATE(aRequest);
>    nsCOMPtr<nsIRunnable> asyncTask =
>      new FireErrorAsyncTask(static_cast<DOMRequest*>(aRequest), aError);
> +  if (NS_WARN_IF(NS_FAILED(NS_DispatchToCurrentThread(asyncTask)))) {

Same here.
Attachment #8626725 - Flags: review?(bent.mozilla) → review+
Assignee

Comment 13

4 years ago
(In reply to Ben Turner [:bent] (use the needinfo flag!) from comment #11)
> Comment on attachment 8626725 [details] [diff] [review]
> bug1167650.patch, v3
> 
> Review of attachment 8626725 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This looks good! Thanks.
> 
> ::: dom/base/DOMRequest.cpp
> @@ +317,3 @@
> >      mozilla::ThreadsafeAutoSafeJSContext cx;
> >      nsRefPtr<FireSuccessAsyncTask> asyncTask = new FireSuccessAsyncTask(cx, aRequest, aResult);
> > +    if (NS_WARN_IF(NS_FAILED(NS_DispatchToCurrentThread(asyncTask)))) {
> 
> NS_DispatchToCurrentThread should really never fail... How about just:
> 
>   MOZ_ALWAYS_TRUE(NS_SUCCEEDED(NS_DispatchToCurrentThread(asyncTask)));
> 
> @@ +369,5 @@
> >  {
> >    NS_ENSURE_STATE(aRequest);
> >    nsCOMPtr<nsIRunnable> asyncTask =
> >      new FireErrorAsyncTask(static_cast<DOMRequest*>(aRequest), aError);
> > +  if (NS_WARN_IF(NS_FAILED(NS_DispatchToCurrentThread(asyncTask)))) {
> 
> Same here.

Landed with these changes.
Attachment #8626725 - Attachment is obsolete: true
Attachment #8627015 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/3723cf728d14
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
Component: DOM → DOM: Core & HTML
Product: Core → Core
You need to log in before you can comment on or make changes to this bug.