Async firing for FireSuccess/Error on DOMRequest

RESOLVED FIXED in mozilla18

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: qdot, Assigned: qdot)

Tracking

Trunk
mozilla18
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

3.21 KB, patch
sicking
: review+
Details | Diff | Splinter Review
3.65 KB, patch
sicking
: review+
khuey
: review+
Details | Diff | Splinter Review
Create FireErrorAsync/FireSuccessAsync functions that dispatch themselves to the main thread to run as a defered task, to deal with DOMRequests that fire right after creation.
Created attachment 653532 [details] [diff] [review]
Patch 1 (v1) - Implement async functions for firing DOMRequests via DOMRequestService
Attachment #653532 - Flags: review?(jonas)
Created attachment 653535 [details] [diff] [review]
Patch 2 (v1) - Tests for async DOMRequest firing via DOMRequestService

So the only way I could figure to run all of the tests and spin the event loop was to call finish at the end of the last error test. However, now there's the "what if the async domrequest never actually fires" issue. Should I set some sort of timeout event?
Attachment #653535 - Flags: review?(jonas)
Comment on attachment 653532 [details] [diff] [review]
Patch 1 (v1) - Implement async functions for firing DOMRequests via DOMRequestService

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

::: dom/base/DOMRequest.cpp
@@ +253,5 @@
> +                       const jsval& aResult) :
> +    mReq(aRequest),
> +    mResult(aResult)
> +  {
> +    static_cast<DOMRequest*>(mReq)->SetResultVal(aResult);

You shouldn't set the result until you fire the success event. Someone checking request.result should only get a value if the success event has fired.

But that leaves the problem of keeping mResult alive while waiting for the success event to fire. The easiest way to do that is to root mResult in the ctor, and un-root it out in the dtor.
Attachment #653532 - Flags: review?(jonas) → review-
Comment on attachment 653535 [details] [diff] [review]
Patch 2 (v1) - Tests for async DOMRequest firing via DOMRequestService

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

Mochitest has builtin timeouts, so that's already taken care of.

::: testing/mochitest/tests/SimpleTest/specialpowersAPI.js
@@ +924,5 @@
>      var serv = Cc["@mozilla.org/dom/dom-request-service;1"].
>        getService(Ci.nsIDOMRequestService);
>      var res = { __exposedProps__: {} };
> +    var props = ["createRequest", "fireError", "fireSuccess", 
> +								 "fireErrorAsync", "fireSuccessAsync"];

Whitespace is looking funky here.
Attachment #653535 - Flags: review?(jonas) → review+
(In reply to Jonas Sicking (:sicking) from comment #3)
> But that leaves the problem of keeping mResult alive while waiting for the
> success event to fire. The easiest way to do that is to root mResult in the
> ctor, and un-root it out in the dtor.

How do I root mResult alone (I'm used to rooting containing classes)? Do I also need to add it for cycle collection?
Check with mrbkap/bent/khuey/peterv or similar.
Created attachment 654264 [details] [diff] [review]
Patch 1 (v2) - Implement async functions for firing DOMRequests via DOMRequestService
Attachment #653532 - Attachment is obsolete: true
Created attachment 654270 [details] [diff] [review]
Patch 1 (v3) - Implement async functions for firing DOMRequests via DOMRequestService

Fixed rooting, removed functions from DOMRequest added in last patch since they're not longer needed.
Attachment #654264 - Attachment is obsolete: true
Attachment #654270 - Flags: review?(jonas)
Comment on attachment 654270 [details] [diff] [review]
Patch 1 (v3) - Implement async functions for firing DOMRequests via DOMRequestService

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

::: dom/base/DOMRequest.cpp
@@ +231,5 @@
> +                       const jsval& aResult) :
> +    mReq(aRequest),
> +    mResult(aResult)
> +  {
> +    JSContext* jsc = mReq->GetJSContextForEventHandlers();

nit: we usually call the js context 'cx'. Same in the dtor.

@@ +256,5 @@
> +class FireErrorAsyncTask : public nsRunnable
> +{
> +public:
> +  FireErrorAsyncTask(nsIDOMDOMRequest* aRequest,
> +                       const nsAString& aError) :

whitespace
Attachment #654270 - Flags: review?(jonas) → review+
Target Milestone: --- → mozilla17
Annnnnnnnnnnd now backed out again due to bustage. ;.;
Please send your patches to try before pushing again.
You'd better not try that again, or we'll have to revoke your commit access.
Created attachment 655046 [details] [diff] [review]
Patch 1 (v4) - Implement async functions for firing DOMRequests via DOMRequestService
Attachment #654270 - Attachment is obsolete: true
Attachment #655046 - Flags: review?(jonas)
Comment on attachment 655046 [details] [diff] [review]
Patch 1 (v4) - Implement async functions for firing DOMRequests via DOMRequestService

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

::: dom/base/DOMRequest.cpp
@@ +231,5 @@
> +                       const jsval& aResult) :
> +    mReq(aRequest),
> +    mResult(aResult)
> +  {
> +    nsresult rv;

Drop the nsresult decl.

@@ +248,5 @@
> +    // We need to build a new request, otherwise we assert since there won't be
> +    // a request available yet.
> +    JSAutoRequest ar(sc->GetNativeContext());
> +    static_cast<DOMRequest*>(mReq)->FireSuccess(mResult);
> +    JS_RemoveValueRoot(sc->GetNativeContext(), &mResult);

Let's move JS_RemoveValueRoot to the dtor.

@@ +256,5 @@
> +  ~FireSuccessAsyncTask()
> +  {
> +  }
> +private:
> +  nsIDOMDOMRequest* mReq;

This almost certainly needs to hold a strong reference to the request.  Lets make it an nsRefPtr<DOMRequest> (then you can lose the ugly static_cast too.

@@ +277,5 @@
> +    static_cast<DOMRequest*>(mReq)->FireError(mError);
> +    return NS_OK;
> +  }
> +private:
> +  nsIDOMDOMRequest* mReq;

Again, this almost certainly needs to be a strong reference.
Attachment #655046 - Flags: review?(khuey) → review-
(In reply to Kyle Huey [:khuey] (khuey@mozilla.com) from comment #20)
> ::: dom/base/DOMRequest.cpp
> @@ +231,5 @@
> > +                       const jsval& aResult) :
> > +    mReq(aRequest),
> > +    mResult(aResult)
> > +  {
> > +    nsresult rv;
> 
> Drop the nsresult decl.

But I use it on the next line? At least changed the moz_assert afterward to check for NS_SUCCEEDED on it.
 
> This almost certainly needs to hold a strong reference to the request.  Lets
> make it an nsRefPtr<DOMRequest> (then you can lose the ugly static_cast too.

Changed these to strong refs, but it only pushes the static_cast down to the FireSuccess/ErrorAsync calls in DOMRequestService.
Created attachment 655847 [details] [diff] [review]
Patch 1 (v5) - Implement async functions for firing DOMRequests via DOMRequestService
Attachment #655046 - Attachment is obsolete: true
Attachment #655046 - Flags: review?(jonas)
Attachment #655847 - Flags: review?(jonas)
Comment on attachment 655847 [details] [diff] [review]
Patch 1 (v5) - Implement async functions for firing DOMRequests via DOMRequestService

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

::: dom/base/DOMRequest.cpp
@@ +233,5 @@
> +  FireSuccessAsyncTask(DOMRequest* aRequest,
> +                       const jsval& aResult) :
> +    mReq(aRequest),
> +    mResult(aResult)
> +  {

Please add an NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); here.

@@ +250,5 @@
> +  }
> +
> +  ~FireSuccessAsyncTask()
> +  {
> +    nsresult rv;

Please add an NS_ASSERTION(NS_IsMainThread(), "Wrong thread!"); here.

@@ +290,5 @@
> +DOMRequestService::FireSuccessAsync(nsIDOMDOMRequest* aRequest,
> +                                    const jsval& aResult)
> +{
> +  NS_ENSURE_STATE(aRequest);
> +  nsCOMPtr<nsIRunnable> asyncTask = new FireSuccessAsyncTask(static_cast<DOMRequest*>(aRequest), aResult);

80 character lines please.

@@ +303,5 @@
> +DOMRequestService::FireErrorAsync(nsIDOMDOMRequest* aRequest,
> +                                  const nsAString& aError)
> +{
> +  NS_ENSURE_STATE(aRequest);
> +  nsCOMPtr<nsIRunnable> asyncTask = new FireErrorAsyncTask(static_cast<DOMRequest*>(aRequest), aError);

here too.
Attachment #655847 - Flags: review?(khuey) → review+
https://hg.mozilla.org/mozilla-central/rev/24e006688c87
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: mozilla17 → mozilla18
You need to log in before you can comment on or make changes to this bug.