This compiles and should mostly work, but probably has some issues. The nsIDOMDOMRequest.idl file should explain how to use this from script.

Comment on attachment 592998 [details] WIP Tiny request, can you split Request and RequestService into different files?

The .idl, or the .h or the .cpp, or all three? The .cpp for the service could probably go away completely and be inlined. Oh, but I do need to quickstub nsIDOMDOMRequest. The service won't be used in a perf critical path so there's no need to quickstub it.

May I ask what is DOMRequest?

Jonas, do you plan to merge SmsRequest and IDBRequest with DOMRequest? (In reply to Olli Pettay [:smaug] from comment #3) > May I ask what is DOMRequest? It's a way for new APIs to return some kind a promise. If you call a foo() function, it will return a DOMRequest object which will have a fire or success event fired on it at some point letting you know when things are done. For exemple: var request = doSomething(foo, bar); request.onsuccess = function() { alert('cool!'); } request.onerror = function() { alert('sad!'); }

(In reply to Jonas Sicking (:sicking) from comment #2) > The .idl, or the .h or the .cpp, or all three? /me votes for all three.

I do want to get rid of SmsRequest in favor of DOMRequest. I'd like to do the same with IDBRequest, but it'll require spec changes so it depends on if I can get those through.

We currently have permission issues. Something like request.onsuccess = function() { alert('cool!'); } gives me: 3 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/contacts/tests/test_contacts_basics.html | an unexpected uncaught JS exception reported through window.onerror - Permission denied to access property 'onsuccess'

Initial tests shows that this appears to be working!

The only real issue that I ran into here is dealing with the various wrappers that are created for the DOMRequest object. The problem is that the DOMRequest object is created in a chrome scope and then handed to the page. Initially this meant that the we got a COW wrapper (or whatever it's called these days) around the DOMRequest which prevented the page from using it. To fix this I made createRequest instead return a jsval, and the implementation of the createRequest function explicitly creates a js object using the webpage's global object as scope. This way XPConnect will create a X-Ray wrapper before handing the object to the calling chrome code, and then remove that X-Ray wrapper when the object is handed to the webpage. At least that's what I think is happening :-)

I see now a compartment mismatch error when I want to access request.result: -*- ContactManager: result: [] *** Compartment mismatch 0x115d12000 vs. 0x10a4d1000 Assertion failure: compartment mismatched, at /Volumes/mac/moz/ws3/js/src/jscntxtinlines.h:153 Program received signal EXC_BAD_ACCESS, Could not access memory. Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000000 CrashInJS () at /Volumes/mac/moz/ws3/js/src/jsutil.cpp:94 94 *((volatile int *) NULL) = 123; /* To continue from here in GDB: "return" then "continue". */ (gdb) bt #0 CrashInJS () at /Volumes/mac/moz/ws3/js/src/jsutil.cpp:94 #1 0x0000000103bce1dd in JS_Assert (s=0x103ffa64a "compartment mismatched", file=0x103ffa661 "/Volumes/mac/moz/ws3/js/src/jscntxtinlines.h", ln=153) at /Volumes/mac/moz/ws3/js/src/jsutil.cpp:114 #2 0x00000001039d7e75 in js::CompartmentChecker::fail (c1=0x115d12000, c2=0x10a4d1000) at jscntxtinlines.h:153 #3 0x00000001039d8d65 in js::CompartmentChecker::check (this=0x7fff5fbf2b98, c=0x10a4d1000) at jscntxtinlines.h:169 #4 0x00000001039d8e2e in js::CompartmentChecker::check (this=0x7fff5fbf2b98, obj=0x10aaabd00) at jscntxtinlines.h:177 #5 0x00000001039d8e83 in js::CompartmentChecker::check (this=0x7fff5fbf2b98, v=@0x7fff5fbf2ba8) at jscntxtinlines.h:187 #6 0x00000001039d5fd7 in js::assertSameCompartment<JSObject*, JS::Value> (cx=0x10d198560, t1=0x10ab97ee0, t2={data = {asBits = 18445477440788282624, debugView = {payload47 = 4473928960, tag = JSVAL_TAG_OBJECT}, s = {payload = {i32 = 178961664, u32 = 178961664, why = 178961664}}, asDouble = -nan(0xb80010aaabd00), asPtr = 0xfffb80010aaabd00, asWord = 18445477440788282624}}) at jscntxtinlines.h:261 #7 0x0000000103b4f3b4 in js::CallJSPropertyOp (cx=0x10d198560, op=0x1029d4600 <nsIDOMDOMRequest_GetResult>, receiver=0x10ab97ee0, id={asBits = 4473725248}, vp=0x7fff5fbf5968) at jscntxtinlines.h:363 #8 0x0000000103b0af1a in js::Shape::get (this=0x116342f38, cx=0x10d198560, receiver=0x10ab97ee0, obj=0x10ab97ee0, pobj=0x10ab97d90, vp=0x7fff5fbf5968) at jsscopeinlines.h:301 #9 0x0000000103afa5fa in js_NativeGetInline (cx=0x10d198560, receiver=0x10ab97ee0, obj=0x10ab97ee0, pobj=0x10ab97d90, shape=0x116342f38, getHow=2, vp=0x7fff5fbf5968) at /Volumes/mac/moz/ws3/js/src/jsobj.cpp:5230 #10 0x0000000103afafd3 in js_GetPropertyHelperInline (cx=0x10d198560, obj=0x10ab97ee0, receiver=0x10ab97ee0, id={asBits = 4473725248}, getHow=2, vp=0x7fff5fbf5968) at /Volumes/mac/moz/ws3/js/src/jsobj.cpp:5387 #11 0x0000000103afa970 in js::GetPropertyHelper (cx=0x10d198560, obj=0x10ab97ee0, id={asBits = 4473725248}, getHow=2, vp=0x7fff5fbf5968) at /Volumes/mac/moz/ws3/js/src/jsobj.cpp:5396 #12 0x0000000103ace297 in js::GetPropertyOperation (cx=0x10d198560, pc=0x10cff6be3 "5", lval=@0x10a500240, vp=0x7fff5fbf5968) at jsinterpinlines.h:288 #13 0x0000000103ab6a97 in js::Interpret (cx=0x10d198560, entryFrame=0x10a5001c0, interpMode=js::JSINTERP_NORMAL) at /Volumes/mac/moz/ws3/js/src/jsinterp.cpp:2674 #14 0x0000000103aab600 in js::RunScript (cx=0x10d198560, script=0x11633af18, fp=0x10a5001c0) at /Volumes/mac/moz/ws3/js/src/jsinterp.cpp:474 #15 0x0000000103ac5631 in js::InvokeKernel (cx=0x10d198560, args={<js::CallReceiver> = {usedRval_ = false, argv_ = 0x10a5001a8}, argc_ = 1}, construct=js::NO_CONSTRUCT) at /Volumes/mac/moz/ws3/js/src/jsinterp.cpp:537 #16 0x0000000103a26e4b in js::Invoke (cx=0x10d198560, args=@0x7fff5fbf61a0, construct=js::NO_CONSTRUCT) at jsinterp.h:157 #17 0x0000000103ac5d4a in js::Invoke (cx=0x10d198560, thisv=@0x7fff5fbf6258, fval=@0x7fff5fbf62c0, argc=1, argv=0x7fff5fbf6cb0, rval=0x7fff5fbf68c8) at /Volumes/mac/moz/ws3/js/src/jsinterp.cpp:569

Jonas fixed the compartment mismatch with a workaround.

What was the workaround? I was wondering where aResult actually comes from when I was reading the patch and haven't had a chance to go figure it out.

The way things work is something like this: 1. chrome script call createRequest and passes in a nsIDOMWindow object 2. createRequest creates a DOMRequest object and passes the nsIDOMWindow to the ctor (this is needed for event firing) 3. createRequest explicitly creates a js-object wrapper for the DOMRequest. The wrapper is created using the nsIDOMWindow as parent 4. createRequest returns the js-object wrapper to the chrome code 5. XPConnect wraps a xray wrapper around the js-object wrapper (at least this is my guess) 6. chrome code returns js-object to content code. 7. XPConnect unwraps the xray wrapper and returns the "normal" js-object wrapper to the content code 8. We wait 9. chrome code calls fireSuccess and passes in a jsval and the xray wrapper as arguments 10. the fireSuccess function calls an internal function on the DOMRequest and passes along the jsval 11. the internal function sets the jsval as mResult member on the DOMRequest 12. the internal function fires a "success" DOM event on the DOMRequest 13. a content event handler is called in response to the event firing. 14. the content code calls the .result getter on the DOMRequest 15. the getter returns the jsval in mResult What happens with the patch as-is is that we return the jsval, which is still whatever chrome object was passed in step 9, to the content code. Since the getter is quick stubbed XPConnect doesn't wrap a COW (or whatever they are called these days) around the jsval, and so we end up with a compartment missmatch (as well as security problems). The workaround was to remove quickstubs. This way XPConnect wraps a COW around the jsval and things work fine. Johnny suggested that we should make quickstubs always check if wrapping is needed. Alternatively we could annotate this function such that quickstubs knows that this check is needed in this case. It's hard to wrap the jsval on the way "to" mResult. This because as far as XPConnect sees, step 9 is calling a function on a chrome object and passes in a chrome object. No wrapping is needed. Another way to fix this would be to make the fireSuccess function wrap as needed since the API is explicitly designed to pass jsvals from chrome code to content code.

Comment on attachment 593268 [details] [diff] [review] Patch to fix Review of attachment 593268 [details] [diff] [review]: ----------------------------------------------------------------- (taking this review from mrbkap because I think he is on vacation) r-, mostly because of mDone never set to true (thus useless). In addition, I would like to see a new version of this patch. ::: dom/base/DOMRequest.cpp @@ +12,5 @@ > +#include "nsEventDispatcher.h" > +#include "nsIPrivateDOMEvent.h" > + > +using mozilla::dom::DOMRequest; > +using mozilla::dom::DOMRequestService; Our code seems to usually do: namespace mozilla { namespace dom { namespace class { // Implementation } // namespace class } // namespace dom } // namespace mozilla Though, I think bent doesn't want to use that coding style too... @@ +17,5 @@ > + > +DOMRequest::DOMRequest(nsIDOMWindow* aWindow) > + : mDone(false), > + mResult(JSVAL_VOID), > + mRooted(false) nit: comma at the beginning of the line is better because anyone can add a line without modifying the previous one @@ +21,5 @@ > + mRooted(false) > +{ > + nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow); > + if (window) { > + mOwner = window->GetCurrentInnerWindow(); You should take into account that aWindow could be a inner window or are you sure the callers will always respect that assumption? In that case, an assert seems appropriate. @@ +43,5 @@ > + > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(DOMRequest, > + nsDOMEventTargetWrapperCache) > + tmp->mResult = JSVAL_VOID; > + tmp->UnrootResultVal(); Do that only if the value is currently rooted. @@ +54,5 @@ > + // Don't need NS_IMPL_CYCLE_COLLECTION_TRACE_PRESERVED_WRAPPER because > + // nsDOMEventTargetHelper does it for us. > + if (JSVAL_IS_GCTHING(tmp->mResult)) { > + void *gcThing = JSVAL_TO_GCTHING(tmp->mResult); > + NS_IMPL_CYCLE_COLLECTION_TRACE_JS_CALLBACK(gcThing, "mResultVal") nit: s/"mResultVal"/"mResult"/ @@ +73,5 @@ > +NS_IMETHODIMP > +DOMRequest::GetReadyState(nsAString& aReadyState) > +{ > + mDone ? aReadyState.AssignLiteral("done") : > + aReadyState.AssignLiteral("pending"); nit: IMO, it's more readable with : below the ?. @@ +79,5 @@ > + return NS_OK; > +} > + > +NS_IMETHODIMP > +DOMRequest::GetResult(jsval *aResult) nit: |jsval *aResult| seems inconsistent with the rest of the file, you probably want |jsval* aResult|. @@ +81,5 @@ > + > +NS_IMETHODIMP > +DOMRequest::GetResult(jsval *aResult) > +{ > + *aResult = mResult; I think it might be interesting to add something like: if (!mDone) { NS_ASSERTION(mResult == JSVAL_VOID, "..."); *aResult = JSVAL_VOID; return NS_OK; } So we are sure we are respecting what is required per spec and we get an assert if things go wrong. @@ +89,5 @@ > + > +NS_IMETHODIMP > +DOMRequest::GetError(nsIDOMDOMError** aError) > +{ > + NS_IF_ADDREF(*aError = mError); Ditto but using nsnull. @@ +97,5 @@ > + > +void > +DOMRequest::FireSuccess(jsval aResult) > +{ > + NS_ABORT_IF_FALSE(!mDone, "Already fired success/error"); Seems like mDone is never set to true... @@ +108,5 @@ > + > +void > +DOMRequest::FireError(const nsAString& aError) > +{ > + NS_ABORT_IF_FALSE(!mDone, "Already fired success/error"); Seems like mDone is never set to true... @@ +118,5 @@ > + > +void > +DOMRequest::FireEvent(const nsAString& aType) > +{ > + if (NS_FAILED(CheckInnerWindowCorrectness())) { Can't you just set the inner window as mOwner in the ctor? @@ +138,5 @@ > + privevent->SetTrusted(true); > + > + event->InitEvent(aType, false, false); > + > + DispatchDOMEvent(nsnull, event, nsnull, nsnull); Could you tell me why this code wouldn't work? nsRefPtr<nsDOMEvent> event = new nsDOMEvent(nsnull, nsnull); nsresult rv = event->InitEvent(aType, false, false); if (NS_FAILED(rv)) { return; } rv = event->SetTrusted(PR_TRUE); if (NS_FAILED(rv)) { return; } bool dummy; aRequest->DispatchEvent(event, &dummy); Seems more common. In addition, there is a bug that aims to have a nsContentUtils::DispatchToSelf() method doing exactly this. @@ +171,5 @@ > +DOMRequestService::FireSuccess(nsIDOMDOMRequest* aRequest, > + const jsval& aResult) > +{ > + DOMRequest* req = static_cast<DOMRequest*>(aRequest); > + req->FireSuccess(aResult); nit: you could also simply do: static_cast<DOMRequest*>(aRequest)->FireSuccess(aResult); @@ +181,5 @@ > +DOMRequestService::FireError(nsIDOMDOMRequest* aRequest, > + const nsAString& aError) > +{ > + DOMRequest* req = static_cast<DOMRequest*>(aRequest); > + req->FireError(aError); nit: static_cast<DOMRequest*>(aRequest)->FireError(aError); ::: dom/base/DOMRequest.h @@ +50,5 @@ > + void FireEvent(const nsAString& aType); > + > + void RootResultVal() > + { > + if (!mRooted) { I would ASSERT instead of checking. That should not happen. @@ +58,5 @@ > + } > + > + void UnrootResultVal() > + { > + if (mRooted) { ditto (assert) @@ +65,5 @@ > + } > + } > +}; > + > +class DOMRequestService : public nsIDOMRequestService I would prefer a specific header for that. @@ +83,5 @@ > + > +} // namespace dom > +} // namespace mozilla > + > +#define DOMREQUEST_SERVICE_CONTRACTID "@mozilla.org/dom/dom-request-service;1" You could move this to the IDL too. ::: dom/base/nsDOMClassInfo.cpp @@ +4365,5 @@ > DOM_CLASSINFO_MAP_ENTRY(nsIDOMDOMError) > DOM_CLASSINFO_MAP_END > > + DOM_CLASSINFO_MAP_BEGIN(DOMRequest, nsIDOMDOMRequest) > + DOM_CLASSINFO_MAP_ENTRY(nsIDOMDOMRequest) I think you are missing: DOM_CLASSINFO_MAP_ENTRY(nsIDOMEventTarget) ::: dom/base/nsIDOMDOMRequest.idl @@ +17,5 @@ > + readonly attribute jsval result; > + readonly attribute nsIDOMDOMError error; > + > + attribute nsIDOMEventListener onsuccess; > + attribute nsIDOMEventListener onerror; Could you try to keep the alignment/identation we usually see in idl files: readonly attribute Type foo; attribute Type2 foobar; @@ +21,5 @@ > + attribute nsIDOMEventListener onerror; > +}; > + > +[scriptable, builtinclass, uuid(eebcdf29-f8fa-4c36-bbc7-2146b1cbaf7b)] > +interface nsIDOMRequestService : nsISupports I would prefer to have this moved to a specific idl. ::: layout/build/nsLayoutCID.h @@ +124,5 @@ > { 0x1a26a7b7, 0xd06e, 0x4f45, { 0x8b, 0x45, 0xd7, 0xad, 0x60, 0xf7, 0xa9, 0xab } } > > +// {3160e271-138d-4cc7-9d63-6429f16957c7} > +#define DOMREQUEST_SERVICE_CID \ > +{ 0x3160e271, 0x138d, 0x4cc7, { 0x9d, 0x63, 0x64, 0x29, 0xf1, 0x69, 0x57, 0xc7 } } Could you put that in nsIDOMDOMRequest.idl?

Two other things about this patch: - we need tests (and I believe that should be fairly easy to do); - we need a solution to handle IPC, this implementation seems too much focused on JS modules.

> ::: dom/base/DOMRequest.cpp > @@ +12,5 @@ > > +#include "nsEventDispatcher.h" > > +#include "nsIPrivateDOMEvent.h" > > + > > +using mozilla::dom::DOMRequest; > > +using mozilla::dom::DOMRequestService; > > Our code seems to usually do: > namespace mozilla { > namespace dom { > namespace class { > > // Implementation > > } // namespace class > } // namespace dom > } // namespace mozilla > > Though, I think bent doesn't want to use that coding style too... I prefer using. > @@ +21,5 @@ > > + mRooted(false) > > +{ > > + nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow); > > + if (window) { > > + mOwner = window->GetCurrentInnerWindow(); > > You should take into account that aWindow could be a inner window or are you > sure the callers will always respect that assumption? In that case, an > assert seems appropriate. In fact, this totally fails because we always get an inner window. Fixed the code to handle both inner and outer windows. > @@ +43,5 @@ > > + > > +NS_IMPL_CYCLE_COLLECTION_UNLINK_BEGIN_INHERITED(DOMRequest, > > + nsDOMEventTargetWrapperCache) > > + tmp->mResult = JSVAL_VOID; > > + tmp->UnrootResultVal(); > > Do that only if the value is currently rooted. Easier to move that check to the UnrootResultVal function. > @@ +73,5 @@ > > +NS_IMETHODIMP > > +DOMRequest::GetReadyState(nsAString& aReadyState) > > +{ > > + mDone ? aReadyState.AssignLiteral("done") : > > + aReadyState.AssignLiteral("pending"); > > nit: IMO, it's more readable with : below the ?. I disagree > @@ +118,5 @@ > > + > > +void > > +DOMRequest::FireEvent(const nsAString& aType) > > +{ > > + if (NS_FAILED(CheckInnerWindowCorrectness())) { > > Can't you just set the inner window as mOwner in the ctor? The idea is that if the user navigates we should not fire the event. See the implementation of CheckInnerWindowCorrectness > @@ +65,5 @@ > > + } > > + } > > +}; > > + > > +class DOMRequestService : public nsIDOMRequestService > > I would prefer a specific header for that. I don't :) > @@ +83,5 @@ > > + > > +} // namespace dom > > +} // namespace mozilla > > + > > +#define DOMREQUEST_SERVICE_CONTRACTID "@mozilla.org/dom/dom-request-service;1" > > You could move this to the IDL too. I think I prefer as-is > @@ +21,5 @@ > > + attribute nsIDOMEventListener onerror; > > +}; > > + > > +[scriptable, builtinclass, uuid(eebcdf29-f8fa-4c36-bbc7-2146b1cbaf7b)] > > +interface nsIDOMRequestService : nsISupports > > I would prefer to have this moved to a specific idl. I prefer as-is > ::: layout/build/nsLayoutCID.h > @@ +124,5 @@ > > { 0x1a26a7b7, 0xd06e, 0x4f45, { 0x8b, 0x45, 0xd7, 0xad, 0x60, 0xf7, 0xa9, 0xab } } > > > > +// {3160e271-138d-4cc7-9d63-6429f16957c7} > > +#define DOMREQUEST_SERVICE_CID \ > > +{ 0x3160e271, 0x138d, 0x4cc7, { 0x9d, 0x63, 0x64, 0x29, 0xf1, 0x69, 0x57, 0xc7 } } > > Could you put that in nsIDOMDOMRequest.idl? I prefer as-is

Comment on attachment 599636 [details] [diff] [review] Patch v2 Review of attachment 599636 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/DOMRequest.cpp @@ +21,5 @@ > + , mResult(JSVAL_VOID) > + , mRooted(false) > +{ > + nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(aWindow); > + mOwner = window->IsInnerWindow() ? window.get() : I realized that in the previous patch, you null checked window. Can that still happen? @@ +92,5 @@ > +NS_IMETHODIMP > +DOMRequest::GetError(nsIDOMDOMError** aError) > +{ > + NS_ASSERTION(mDone || !mError, > + "Result should be undefined when pending"); Copy/paste error: the message here should refer to error, I think. ::: dom/base/DOMRequest.h @@ +72,5 @@ > + NS_DECL_ISUPPORTS > + NS_DECL_NSIDOMREQUESTSERVICE > + > + // Returns an owning reference! No one should call this but the factory. > + static DOMRequestService* FactoryCreate() Maybe a stupid question, but can this return already_AddRefed<DOMRequestService>?

Checked in a few days ago: http://hg.mozilla.org/mozilla-central/rev/afc1125f9c14 (In reply to Blake Kaplan (:mrbkap) from comment #19) > I realized that in the previous patch, you null checked window. Can that > still happen? No, that should never happen. I talked with blake about this and since it can never be null I also removed the nullcheck for the sgo. > Copy/paste error: the message here should refer to error, I think. Fixed > Maybe a stupid question, but can this return > already_AddRefed<DOMRequestService>? This signature is forced by the macros used to create XPCOM factories :(

You forgot to hook mError up to cycle collection.

Hmm.. mError is just a DOMError object, so it shouldn't be able to participate in cycles. mResult does seem like we need to cc though!

Documented: https://developer.mozilla.org/en/DOM/DOMRequest https://developer.mozilla.org/en/DOM/DOMRequest.onsuccess https://developer.mozilla.org/en/DOM/DOMRequest.onerror https://developer.mozilla.org/en/DOM/DOMRequest.result https://developer.mozilla.org/en/DOM/DOMRequest.readyState And listed on Firefox 13 for developers.

It looks like at present DOMRequest is not part of a specification. True? Either way, is there a spec or draft spec I can link to? Is it planned to be submitted for spec?

DOM Core would have been the best specification to host that but I asked around and it can't be added because of the onfoo methods which need a dependency to HTML specification (and for sanity, DOM Core doesn't want that). The solution is to have that part moved to DOM Core but it's not going to happen very soon I believe. In the mean time, I don't really know we could do.