Andrew Osmond [:aosmond] (he/him) Assignee
	Description • 9 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.

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 1 • 9 years ago

In order to support device storage with workers, DOMRequest needs to updated as well until such a time we switch to promises.

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 2 • 9 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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 3 • 9 years ago

Last I checked, DOMRequest needed to be killed with fire.

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 4 • 9 years ago

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...

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 5 • 9 years ago

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+.

	Andrew Overholt [:overholt]
	Comment 6 • 9 years ago

(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.

	Jonas Sicking (:sicking) No longer reading bugmail consistently
	Comment 7 • 9 years ago

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.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 8 • 9 years ago

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?

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 9 • 9 years ago

Comment on attachment 8612548 [details] [diff] [review]
bug1167650.patch, v2

Canceling review to get questions addressed. Please re-request whenever.

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 10 • 9 years ago

(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.

	Ben Turner (not reading bugmail, use the needinfo flag!)
	Comment 11 • 9 years ago

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.

	Pulsebot
	Comment 12 • 9 years ago

https://hg.mozilla.org/integration/b2g-inbound/rev/3723cf728d14

	Andrew Osmond [:aosmond] (he/him) Assignee
	Comment 13 • 9 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.

	Carsten Book [:Tomcat]
	Comment 14 • 9 years ago

https://hg.mozilla.org/mozilla-central/rev/3723cf728d14

	Chris Mills [:cmills]
	Comment 15 • 9 years ago

I've added worker availability information to the relevant pages:

https://developer.mozilla.org/en-US/docs/Web/API/DOMRequest (and member subpages)
https://developer.mozilla.org/en-US/docs/Web/API/DOMCursor (and member subpages)

https://developer.mozilla.org/en-US/docs/Web/API/Worker/Functions_and_classes_available_to_workers
https://developer.mozilla.org/en-US/Firefox/Releases/41#Miscellaneous