Still a few stuff to figure out: - 'success' error should be send before or after the chaining (callbacks at all)? - for the moment, when an error happen, the only way to stop calling errors callbacks is to return a DOMRequest (Alex's API doesn't match that behaviour); - there are still some leaks :(

Comment on attachment 712219 [details] [diff] [review] Patch Talked with mounir about this and I think we agreed to wait an see where DOMFuture goes before making a move here.

(In reply to Jonas Sicking (:sicking) from comment #1) > Talked with mounir about this and I think we agreed to wait an see where > DOMFuture goes before making a move here. That's a fair decision. What are the particular points you're waiting on? Is there any way to help move things forward on the spec side?

Should this be closed as WONTFIX in favour of Futures?

It might still be good to add to ease migration.

I believe that if we end up implementing something to ease migration, we should probably have something like: interface DOMRequest : Future { readonly attribute any result; readonly attribute DOMError error; readonly attribute DOMString status; (or whatever the same is) + events }; It would be safer and easier than improving DOMRequest to be compatible with Future.

Un-assigning myself because my experiment might unlikely be what we will be doing. I think that a better solution to implement this would be to make DOMRequest inherits from Promises C++ implementation and add GetResult() GetError() and GetStatus() methods and the event handling in the sub-class. That should be easier than adding .then() and all the other Promises properties to DOMRequest and more future-friendly.

I would be very unusual to add event handling in the subclass. EventTarget is in general the first interface in the prototype chain. But perhaps you're talking about C++ side only. Could we wrap a promise object inside DOMRequest, and then just forward method calls to the inner object?

(In reply to Olli Pettay [:smaug] from comment #7) > Could we wrap a promise object inside DOMRequest, and then just forward > method calls to the inner object? I wouldn't recommand that approach. The long term goal would be to get rid of DOMRequest. I drafted a plan at https://groups.google.com/d/msg/mozilla.dev.webapi/4O_ffcuLjdE/e77aUBlhv_IJ Note a couple of messages later Jérémie Patonnier's idea to make using Promise methods/props instead of DOMRequest a Marketplace criteria to accept an app (something that can't be done on the web :-) )

I'd be interested to know if the inclusion of thenables in the promise spec affects this bug in any way by simplifying it. Can DOMRequest just act as a thenable? David, I'd appreciate it if you could summarize the conclusions derived from here and the mailing list, since all the suggestions seem to be 'maybe we could do this' and not conclusive (I'm not saying thats a bad thing.) As prickly as this issue is, DOMRequests suck IMHO and if we can get rid of them faster, that's awesome!

(In reply to Nikhil Marathe [:nsm] (needinfo? please) from comment #9) > I'd be interested to know if the inclusion of thenables in the promise spec > affects this bug in any way by simplifying it. Can DOMRequest just act as a > thenable? I don't think it affects this bug and the most likely scenario so far which is DOMRequest inheriting from Promise. > David, I'd appreciate it if you could summarize the conclusions derived from > here and the mailing list, since all the suggestions seem to be 'maybe we > could do this' and not conclusive (I'm not saying thats a bad thing.) I don't think there is a conclusion yet. I proposed a plan in the mailing-list (see comment #8), Jérémie Patonnier seemed to agree and I haven't seen oppositions yet. Of course, this plan requires coordination from several teams (dev-engage for the hacks posts, marketplace for the extra review work, engineering to make DOMRequest inherit from Promise, doc to make the doc changes), so that's some work. I'm happy to do the coordination, MozHacks and doc part, but I'd need a signal that engineering and Marketplace are in. Needinfo'ing smaug for the engineering part (unless you can make that call, Nikhil?). Who's the right person to contact on the marketplace/app-review side?

(In reply to David Bruant from comment #10) > Who's the right person to contact on the marketplace/app-review side? needinfo'ing Lisa Brewster since she's listed as Apps reviews Lead in https://wiki.mozilla.org/Marketplace/Reviewers/Apps Lisa: as an attempt to get rid of DOMRequest, one thing that could help would be to reject apps that do use DOMRequest (an easy alternative will be provided and documented). Do you think it's possible to include such a criteria when it comes to rejecting apps eventually?

I don't know what info you're asking. But anyhow, how could https://groups.google.com/forum/#!msg/mozilla.dev.webapi/4O_ffcuLjdE/e77aUBlhv_IJ work? DOMRequest inherits EventTarget. DOMRequest inheriting EventTarget and Promise at the same time? We don't have multiple inheritance.

(In reply to Olli Pettay [:smaug] from comment #12) > I don't know what info you're asking. More or less whether this bug is something that Gecko folks are interested in doing and how. > But anyhow, how could > https://groups.google.com/forum/#!msg/mozilla.dev.webapi/4O_ffcuLjdE/ > e77aUBlhv_IJ work? DOMRequest inherits EventTarget. DOMRequest inheriting > EventTarget and Promise at the same time? We don't have multiple inheritance. An earlier version of Promises did inherit from EventTarget [1]. But of course that's not the case anymore... Alright. Adding a single .then method it is, I guess? Support of thenables will indeed be of help. [1] https://github.com/slightlyoff/Promises/blob/84c03cb0ecf40b1c6dd7edb1604b0ede2fbf3c35/DOMFuture.idl#L259

Adding Andrew (app review tech lead) and Mark (owner of the app validator who can hopefully enforce this request during app submission instead of wasting our reviewers' precious human brains :)

I can add the then(). This then() will need to have the same semantics as Promise.then (returned value of onFulfill is cast to a Promise). I am not aware of the __proto__ intricacies involved, so it will be fantastic if David can write a test suite. Note that DOM Promises don't support subclassing.

(In reply to David Bruant from comment #11) > (In reply to David Bruant from comment #10) > > Who's the right person to contact on the marketplace/app-review side? > needinfo'ing Lisa Brewster since she's listed as Apps reviews Lead in > https://wiki.mozilla.org/Marketplace/Reviewers/Apps > Lisa: as an attempt to get rid of DOMRequest, one thing that could help > would be to reject apps that do use DOMRequest (an easy alternative will be > provided and documented). Do you think it's possible to include such a > criteria when it comes to rejecting apps eventually? Rejecting apps would be an extreme step if the DOMRequest API is still valid and available and I can only see it being appropriate if there is a security or particularly bad performance issue. What's the issue with DOMRequest exactly? We could only apply such a rule to packaged apps btw, the code for hosted apps isn't validated (and could change at any point anyway)

And would make it more difficult (even if possible) to do apps for different FxOS versions.

We should not use DOMRequest for anything new. It might make sense to add a then() method on it (returning a promise) to make transition easier, but other than that I don't think we should do much about it for now. DOMRequest itself is not really at fault, it's the APIs that emit it.

(In reply to Andrew Williamson [:eviljeff] from comment #16) > Rejecting apps would be an extreme step if the DOMRequest API is still valid > and available and I can only see it being appropriate if there is a security > or particularly bad performance issue. What's the issue with DOMRequest > exactly? It's a bad API that composes poorly with other APIs. I don't think there'll be security issues. There is no particularly bad performance issues with DOMRequest per se, but it encourages a bad way of programming asynchronously (callback-based) which leads to poor programming with async stuffs which leads to perf issues. It's indirect, but it can matter in big enough applications. Arguably, code using DOMRequest is harder to maintain, maybe to an extent that hurts perf and security. But I understand it might not be good enough a reason to reject apps with DOMRequest. > We could only apply such a rule to packaged apps btw, the code for hosted > apps isn't validated (and could change at any point anyway) Of course. But I think no non-privileged APIs uses DOMRequest, I'll need to check. (In reply to Julien Wajsberg [:julienw] from comment #17) > And would make it more difficult (even if possible) to do apps for different > FxOS versions. Mozilla could ship a polyfill so that in old FirefoxOS phones, methods returning DOMRequest returns Promises instead. Platform fragmentation is a problem we know how to solve ;-)

I think once the changes have landed in a branch (fx1.4?) we could add a validation warning that DOMRequest is deprecated and they should do XXX instead. Then during the code review for privileged apps the reviewer could feed it back to the developer. If, at some future point, old FxOS versions are polyfilled as suggested, and its still coming up as an issue we can revisit if rejecting is necessary - but once the docs, APIs, template apps ,etc are updated I'd imagine most developers will use the new method anyway, especially if its as bad to use as you say.

I think we should add a .then() function to DOMRequest and then do nothing more for now. Most of the APIs that we have that return DOMRequests are in need of getting specced and cleaned up anyway. So those APIs will likely change in backwards incompatible ways at that time. Forcing people to switch from DOMRequest to Promise but otherwise keep the current API is mainly busy work with little value otherwise. By adding DOMRequest.then() we enable developers that want to use the better Promise patterns to do so. But we don't force people to update their code if they don't care. DOMRequest.then() should return a Promise though. And obviously should conform to the Promise spec's definition of the .then() function.

This is implemented by creating a Promise object internally and forwarding the .then() call to it. Any further callbacks passed to future .then() calls will be added as callbacks on the same internal promise object. We also take care of resolving or rejecting the promise if the success/error event of the DOMRequest object has been fired before .then() is called.

Julian pointed out an important problem. Sometimes we use DOMRequest as a cursor. See specifically nsIDOMRequestService.createCursor and DOMCursor. The easiest solution seems to be to make DOMCursors simply not have a .then() function. So perhaps make DOMCursor not subclass DOMRequest but instead copy its interface?

(In reply to Jonas Sicking (:sicking) from comment #23) > Julian pointed out an important problem. Sometimes we use DOMRequest as a > cursor. See specifically nsIDOMRequestService.createCursor and DOMCursor. > > The easiest solution seems to be to make DOMCursors simply not have a > .then() function. So perhaps make DOMCursor not subclass DOMRequest but > instead copy its interface? Sure. I'll use a shared interface to avoid the copying though.

This is implemented by creating a Promise object internally and forwarding the .then() call to it. Any further callbacks passed to future .then() calls will be added as callbacks on the same internal promise object. We also take care of resolving or rejecting the promise if the success/error event of the DOMRequest object has been fired before .then() is called.

Comment on attachment 8496454 [details] [diff] [review] Implement DOMRequest.then; r=sicking Review of attachment 8496454 [details] [diff] [review]: ----------------------------------------------------------------- Drive by review. ::: dom/base/DOMRequest.cpp @@ +119,5 @@ > > FireEvent(NS_LITERAL_STRING("success"), false, false); > + > + if (mPromise) { > + AutoSafeJSContext cx; DOMRequest may be used on workers (IDB), so either use ThreadsafeAutoSafeJSContext, or AutoJSAPI, both of which do the right thing. @@ +121,5 @@ > + > + if (mPromise) { > + AutoSafeJSContext cx; > + JS::Rooted<JS::Value> result(cx, mResult); > + mPromise->MaybeResolve(result); Since you have a JS::Value, you could use the 2 arg form of MaybeResolve(cx, jsval) to avoid conversion attempts. You will have to be sure the value is in the correct compartment (in this case it should be). @@ +220,5 @@ > + if (aRv.Failed()) { > + return nullptr; > + } > + > + if (mDone) { Not sure why you are handling this manually. If the promise is already resolved/rejected, it will deal with then() callbacks correctly.

Comment on attachment 8496454 [details] [diff] [review] Implement DOMRequest.then; r=sicking Review of attachment 8496454 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the drive-by! ::: dom/base/DOMRequest.cpp @@ +220,5 @@ > + if (aRv.Failed()) { > + return nullptr; > + } > + > + if (mDone) { Note that I do this on mPromise, not the newly created promise. This is where we would resolve/reject mPromise itself. I rely on what you're saying to take care of things for the newly created promise.

This is implemented by creating a Promise object internally and forwarding the .then() call to it. Any further callbacks passed to future .then() calls will be added as callbacks on the same internal promise object. We also take care of resolving or rejecting the promise if the success/error event of the DOMRequest object has been fired before .then() is called.

(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #27) > Comment on attachment 8496454 [details] [diff] [review] > Implement DOMRequest.then; r=sicking > > Review of attachment 8496454 [details] [diff] [review]: > ----------------------------------------------------------------- > > Thanks for the drive-by! > > ::: dom/base/DOMRequest.cpp > @@ +220,5 @@ > > + if (aRv.Failed()) { > > + return nullptr; > > + } > > + > > + if (mDone) { > > Note that I do this on mPromise, not the newly created promise. This is > where we would resolve/reject mPromise itself. I rely on what you're saying > to take care of things for the newly created promise. Makes sense. Could you add a comment before landing? it isn't entirely obvious. I think what you are saying is 'mPromise' is only set to a valid Promise when somebody expresses interest for the first time by calling then(). At that point, the DOMRequest might already be 'resolved', so you need to 'bring the promise in sync' with the state of the DOMRequest. In that case, couldn't you move this block to inside the |if (!mPromise)| check?

Comment on attachment 8497837 [details] [diff] [review] Implement DOMRequest.then; r=sicking Review of attachment 8497837 [details] [diff] [review]: ----------------------------------------------------------------- ::: dom/base/DOMRequest.cpp @@ +119,5 @@ > > FireEvent(NS_LITERAL_STRING("success"), false, false); > + > + if (mPromise) { > + AutoSafeJSContext cx; Did you miss this? :)

Comment on attachment 8496454 [details] [diff] [review] Implement DOMRequest.then; r=sicking Review of attachment 8496454 [details] [diff] [review]: ----------------------------------------------------------------- r- for the context thing. ::: dom/base/DOMRequest.cpp @@ +119,5 @@ > > FireEvent(NS_LITERAL_STRING("success"), false, false); > + > + if (mPromise) { > + AutoSafeJSContext cx; Why don't you need to grab the right JS context here? @@ +220,5 @@ > + if (aRv.Failed()) { > + return nullptr; > + } > + > + if (mDone) { Alternatively don't create the promise lazily. I'm fine either way.

This is implemented by creating a Promise object internally and forwarding the .then() call to it. Any further callbacks passed to future .then() calls will be added as callbacks on the same internal promise object. We also take care of resolving or rejecting the promise if the success/error event of the DOMRequest object has been fired before .then() is called.

Comment on attachment 8499571 [details] [diff] [review] Implement DOMRequest.then; r=sicking Addressed the review comments. I kept the creation of the promise lazy, I think with the comments that I added things are more clear.

Comment on attachment 8499571 [details] [diff] [review] Implement DOMRequest.then; r=sicking Review of attachment 8499571 [details] [diff] [review]: ----------------------------------------------------------------- r=me for the other pieces. ::: dom/base/DOMRequest.cpp @@ +121,5 @@ > + > + if (mPromise) { > + ThreadsafeAutoJSContext cx; > + JS::Rooted<JS::Value> result(cx, mResult); > + mPromise->MaybeResolve(cx, result); Someone that knows these things better than me should review the JSContext stuff here.

Comment on attachment 8499571 [details] [diff] [review] Implement DOMRequest.then; r=sicking (In reply to Jonas Sicking (:sicking) from comment #34) > ::: dom/base/DOMRequest.cpp > @@ +121,5 @@ > > + > > + if (mPromise) { > > + ThreadsafeAutoJSContext cx; > > + JS::Rooted<JS::Value> result(cx, mResult); > > + mPromise->MaybeResolve(cx, result); > > Someone that knows these things better than me should review the JSContext > stuff here. Boris, can you review those bits please? Thanks!

Comment on attachment 8499571 [details] [diff] [review] Implement DOMRequest.then; r=sicking >+ mResult = JSVAL_VOID; mResult.setUndefined(); But why do you need to do that at all? >+ ThreadsafeAutoJSContext cx; No, please don't. This should work fine: mPromise->MaybeResolve(result); >+ JS::Rooted<JS::Value> result(aCx, mResult); Likewise, please just call the single-argument version of MaybeResolve. The two-argument one would involve you wrapping mResult into the compartment of aCx and all that jazz, which you don't want to deal with. >+ nsRefPtr<Promise> promise = mPromise->Then(aCx, aResolveCallback, >+ aRejectCallback, aRv); Why not just: return mPromise->Then(aCx, aResolveCallback, aRejectCallback, aRv); ? >- AutoSafeJSContext cx; >+ ThreadsafeAutoJSContext cx; This is changing behavior for mainthread. Please use ThreadsafeAutoSafeJSContext for now, I guess. Rejecting things with DOMError is kinda weird, since it's not an Error subtype, but I guess it's preexisting. Is there value in filing a bug on making it be one, more or less? Specifically: 1) Mark it as [ExceptionClass] 2) Mark its name/message as [LenientThis] 3) Add a readonly [Replaceable] "stack" property (that returns empty string, I guess). 4) Add an overload of single-argument MaybeReject that takes a const nsRefPtr<DOMError>&. 5) Remove the DOMError overload of MaybeRejectBrokenly.

We should remove DOMError from Gecko and replace its use with DOMException. But obviously not in this bug. Bz, do you think making sure that we reject with an Error is something that's blocking this bug?

(In reply to Boris Zbarsky [:bz] from comment #36) > Comment on attachment 8499571 [details] [diff] [review] > Implement DOMRequest.then; r=sicking > > >+ mResult = JSVAL_VOID; > > mResult.setUndefined(); > > But why do you need to do that at all? I'm just moving this code from the header to the cpp... > >+ ThreadsafeAutoJSContext cx; > > No, please don't. This should work fine: > > mPromise->MaybeResolve(result); > > >+ JS::Rooted<JS::Value> result(aCx, mResult); > > Likewise, please just call the single-argument version of MaybeResolve. The > two-argument one would involve you wrapping mResult into the compartment of > aCx and all that jazz, which you don't want to deal with. I don't understand what you're suggesting exactly. The single argument version of MaybeResolve only works for stuff that ToJSValue supports, right? mResult is a JS::Heap<JS::Value> and AFAICT that is not supported by ToJSValue. Also, that single argument version seems to just call MaybeSomething which does all of what you say we should avoid here, so I'm puzzled.

> Bz, do you think making sure that we reject with an Error is something that's blocking > this bug? Absolutely not. That's why I asked a question instead of saying to fix it. ;) > mResult is a JS::Heap<JS::Value> and AFAICT that is not supported by ToJSValue. Ah, I see. We have a ToJSValue overload for JS::Handle<JS::Value> but not for Heap<Value> or Rooted<Value>. We should just add those, so you can MaybeResolve(mResult). They'll look like a copy/paste of http://hg.mozilla.org/mozilla-central/file/4bad24a306b2/dom/bindings/ToJSValue.h#l231 but with a different type for aArgument. > which does all of what you say we should avoid here Not quite. It does things like enter the right compartments, wrap things into those compartments, etc. Things that you can't easily do in this code, actually.

Backed out for non-unified build bustage, despite two attempts at fixing: https://hg.mozilla.org/integration/mozilla-inbound/rev/923a414099b1 https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2868263&repo=mozilla-inbound https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2868261&repo=mozilla-inbound

This also broke Mnw tests: https://treeherder.mozilla.org/ui/logviewer.html#?job_id=2866750&repo=mozilla-inbound

(In reply to Wes Kocher (:KWierso) from comment #42) > This also broke Mnw tests: > https://treeherder.mozilla.org/ui/logviewer. > html#?job_id=2866750&repo=mozilla-inbound This was caused by <http://mxr.mozilla.org/mozilla-central/source/dom/telephony/test/marionette/head.js?rev=34dbc25304b9#1320>. Now that DOMRequest is a thenable, this promise will be resolved to the result of the DOMRequest. Filed bug 1080883 to fix the API design.

Second try: https://hg.mozilla.org/integration/mozilla-inbound/rev/3a89e23a25f0

Sorry I backed this out for rooting analysis bustage: https://hg.mozilla.org/integration/mozilla-inbound/rev/16a8a5c8b96a https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla-inbound-linux64-br-haz/20141011064725/hazards.txt.gz I looked at it briefly but I didn't see an obviously-correct fix.

(In reply to Jan de Mooij [:jandem] from comment #45) > Sorry I backed this out for rooting analysis bustage: > > https://hg.mozilla.org/integration/mozilla-inbound/rev/16a8a5c8b96a > > https://ftp-ssl.mozilla.org/pub/mozilla.org/firefox/tinderbox-builds/mozilla- > inbound-linux64-br-haz/20141011064725/hazards.txt.gz > > I looked at it briefly but I didn't see an obviously-correct fix. Hmm. Boris, it seems like passing a JS::Heap to MaybeResolve is not the right thing to do. What should I do instead?

Pass a JS::Heap reference? For that matter, you don't want to pass JS::Rooted by value either; passing it by reference should be ok.

Ah, right. :-) Third try: https://hg.mozilla.org/integration/mozilla-inbound/rev/a14b8bf72b16

We triaged this bug and the needed documentation is: - update of https://developer.mozilla.org/en-US/docs/Web/API/DOMRequest - creation of https://developer.mozilla.org/en-US/docs/Web/API/DOMRequest.then - update of https://developer.mozilla.org/en-US/Firefox/Releases/36