Closed Bug 783426 Opened 12 years ago Closed 12 years ago

Async firing for FireSuccess/Error on DOMRequest

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla18

People

(Reporter: qdot, Assigned: qdot)

References

Details

Attachments

(2 files, 4 obsolete files)

Patch 2 (v1) - Tests for async DOMRequest firing via DOMRequestService
3.21 KB, patch
sicking
: review+
Details | Diff | Splinter Review
Patch 1 (v5) - Implement async functions for firing DOMRequests via DOMRequestService
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.
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.
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.
New try at https://tbpl.mozilla.org/?tree=Try&rev=653fee109e76

Needs rereview due to change in rooting scheme.
Attachment #655046 - Flags: review?(khuey)
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.
Attachment #655847 - Flags: review?(khuey)
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
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: mozilla17 → mozilla18
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: