Currently we have no way of knowing when addPluginDirectory/removePluginDirectory are finished and whether they succeeded. This makes a) testing hard to & unreliable (need to use timeouts to guesstimate when it is done and b) the addon manager implementation unreliable if there are failures We should fix this by returning a Promise<bool> from those methods.

ni?jesup for heads-up.

ni?jesup for heads-up.

mozIGeckoMediaPluginService is IDL, and IDL is currently not promise-capable. This might be really hard.

So, from a quick peek around the tree, it looks like we can pass Promise<T> through as nsISupport. This here: http://hg.mozilla.org/mozilla-central/annotate/ae4d9b4ff2ee/dom/interfaces/base/nsIServiceWorkerManager.idl#l41 ... should chain up via: http://hg.mozilla.org/mozilla-central/annotate/ae4d9b4ff2ee/dom/workers/ServiceWorkerContainer.cpp#l119 http://hg.mozilla.org/mozilla-central/annotate/ae4d9b4ff2ee/dom/workers/ServiceWorkerManager.cpp#l669

Other interfaces pass it through as jsval though, which may be preferred?

I would go with `jsval`, and then use JSAPI to look for a method `then`. It's much nicer with webidl, of course.

So, I'm not certain right now what I'm being asked to do, and where I can find an example of that if it's more than just returning 'bool' from the C++/IDL interface. Yoric? Georg?

jesup: See comment 4 for an example.

We discussed this in IRC. So, for Promise::Create() we need an nsIGlobalObject. In the context of mozIGeckoMediaPluginService we don't get a window, but we can get an [implicit_jscontext]. From IRC, jesup found xpc::NativeGlobal() as a possible way to get a global object: |nsIGlobalObject *global = xpc::NativeGlobal(JS::CurrentGlobalOrNull(jscontext))| http://hg.mozilla.org/mozilla-central/annotate/9781037ac408/dom/crypto/WebCryptoTask.cpp#l1217 Olli, do you know if that is the proper approach? The relevant caller contexts are AddonManager providers as well as tests (xpcshell or mochitest).

Actual needinfo for comment 9.

So in which context the Promise would live? Is this all for chrome only stuff and various different possible contexts (different JS global object)? Getting global from cx should be fine, assuming nothing stores the Promise too long on C++ side. Another option, which bholley probably doesn't quite like, is to use PrivilegedJunkScope.

(In reply to Olli Pettay [:smaug] from comment #11) > So in which context the Promise would live? Is this all for chrome only > stuff and various different > possible contexts (different JS global object)? Definitely chrome-only. I assume different contexts are possible, but i'm not a 100% clear on how things work for this. > Getting global from cx should be fine, assuming nothing stores the Promise > too long on C++ side. This is probably "not too long" - the worst case here is from the delay on adding a new plugin directory, hitting the disk for its info and jumping between main and GMP thread. Would PrivilegedJunkScope be better as we couldn't keep contexts alive that we shouldn't?

global from the cx sounds like the right approach here, assuming the API is to be called from JS only.

We call addPluginDir from C++ in order to make MOZ_GMP_PATH work. Please make sure you don't break that, as GMP authors depend on that, and so do our tests.

(In reply to Georg Fritzsche [:gfritzsche] from comment #0) > Currently we have no way of knowing when > addPluginDirectory/removePluginDirectory are finished and whether they > succeeded. One (kinda gross) way to wait for an async operation is to dispatch an observer service notification when your async operation is complete, and run your continuation task in the observer. That's how I simulate a wait until a GMP has shutdown in the GMP async shutdown gtests: http://mxr.mozilla.org/mozilla-central/source/content/media/gtest/TestGMPCrossOrigin.cpp#567

Right, we have that notification pattern all over the place, but would like to avoid using it here if possible. In this case passing a promise around doesn't seem to hard. (In reply to Chris Pearce (:cpearce) from comment #14) > We call addPluginDir from C++ in order to make MOZ_GMP_PATH work. Please > make sure you don't break that, as GMP authors depend on that, and so do our > tests. I don't see any C++ callers, the MOZ_GMP_PATH handling doesn't use that public API: http://dxr.mozilla.org/mozilla-central/source/dom/media/gmp/GMPService.cpp#592

Queole will take a look here if he finds the time. Jesup, could you maybe help out with guidance here while i'm on vacation?

So I've been looking at this, but there is a lot I don't understand. As far as I get it, what we want to do is to modify the code of AddPluginDirectory() and RemovePluginDirectory() functions so that they return promises. To do that we should: - create a global object (as in link from comment #9) - feed Promise::Create() with it to get a promise and handle it more or less like in code linked in comment #4. I tried to modify AddPluginDirectory() to achieve that, but I haven't succeeded yet. Included patch compiles, but GMP plugin does not launch. There are several things I don't understand here, including: - at least, did I get the global approach right? - how should I handle the JScontext variable? Should I use a JSContext* or an AutoJSContext, and how should it be initialized (or should it be somehow provided as an argument to AddPluginDirectory())? - what should be the return value for AddPluginFunction? I feel like I should replace the |void| at line [1] but couldn't find what type to put instead. I tried nsISupports as in [2], but couldn't find a correct value to return from AddPluginDirectory() (I only got a lot of compiler errors when trying this) - how should I handle the promise in C++? I couldn't find any documentation about the C++ part for promises on MDN :( Jesup, could you please provide some guidance about this? [1] http://hg.mozilla.org/mozilla-central/annotate/c9cfa9b91dea/dom/media/gmp/mozIGeckoMediaPluginService.idl#l84 [2] http://hg.mozilla.org/mozilla-central/annotate/ae4d9b4ff2ee/dom/interfaces/base/nsIServiceWorkerManager.idl#l41

Comment on attachment 8527307 [details] [diff] [review] gmp-setup-promise.patch − non functional I'm still busy at the Portland meetings this week, but i should be looking over this next week.

Sorry, it took me a bit with getting here due to being sick. (In reply to Qeole [:qeole] from comment #18) [...] > - how should I handle the JScontext variable? Should I use a JSContext* or > an AutoJSContext, and how should it be initialized (or should it be somehow > provided as an argument to AddPluginDirectory())? You can get an implicit_jscontext like e.g. here: http://hg.mozilla.org/mozilla-central/annotate/be1f49e80d2d/toolkit/components/telemetry/nsITelemetry.idl#l155 http://hg.mozilla.org/mozilla-central/annotate/d7c76fe69e9a/toolkit/components/telemetry/Telemetry.cpp#l1756 > - what should be the return value for AddPluginFunction? I feel like I > should replace the |void| at line [1] but couldn't find what type to put > instead. I tried nsISupports as in [2], but couldn't find a correct value to > return from AddPluginDirectory() (I only got a lot of compiler errors when > trying this) We should use jsval in the IDL, you can check newHistogram() above on how that maps to the C++ signature. > - how should I handle the promise in C++? I couldn't find any documentation > about the C++ part for promises on MDN :( Do you mean for resolving it? Promise.h has the interface: https://dxr.mozilla.org/mozilla-central/source/dom/promise/Promise.h ... Promise::MaybeResolve(T) looks good to me :)

So, what's definitely missing now is * returning the promise to the caller as a jsval * passing the promise through to the PathRunnable() etc. so it can be resolved once we added the plugin on the GMP thread

Work in progress patch: compiles but does not work (segmentation fault at Firefox launch, launching a test says “TEST-INFO | Main app process: killed by SIGSEGV”) Georg, I think I added everything we discussed above and by IRC; now I'm trying to find what makes it crash.

Thanks, i'm busy this week, but i will try to get to this if possible.

Sorry for the late follow-up, i have been a little swamped :( (In reply to Qeole [:qeole] from comment #22) > Georg, I think I added everything we discussed above and by IRC; now I'm > trying to find what makes it crash. Did you have any luck with this? Do you have a stack for the crash?

Comment on attachment 8547174 [details] [diff] [review] gmp-setup-promise.patch v2 − non functional Review of attachment 8547174 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, this looks like what we talked about. smaug, does the context and ToJSValue() usage here look ok to you? ::: dom/media/gmp/GMPService.cpp @@ +675,1 @@ > { AddPluginDirectory() & RemovePluginDirectory() look the same except the true/false value they pass to PathRunnable(). Can we just merge them into, say, LaunchPathRunnable()? ::: dom/media/gmp/GMPService.h @@ +86,5 @@ > class PathRunnable : public nsRunnable > { > public: > PathRunnable(GeckoMediaPluginService* service, const nsAString& path, > + Promise* promise, bool add) We could just stay with |const nsRefPtr<Promise>&| instead of dropping to plain pointers.

Comment on attachment 8547174 [details] [diff] [review] gmp-setup-promise.patch v2 − non functional Should be ok, since there should be AutoJSAPI somewhere on the stack when the methods are called.

WIP: I've rebased the patch on latest version of mozilla-central, but I did not succeed in correcting the bug. Behavior is the same as stated in comment #22: > compiles but does not work (segmentation fault at Firefox launch, launching a > test says “TEST-INFO | Main app process: killed by SIGSEGV”) Any mochitest crashes before Fx window is displayed. I ran it through gdb and got a backtrace (see attachment below): it seems that my call to |mPromise->MaybeResolve(res)| has an issue. It calls several functions until |GetCurrentThreadWorkerPrivate()->GetJSContext();| is called with |GetCurrentThreadWorkerPrivate()| returning a null pointer. Then |GetJSContext()| calls |AssertIsOnWorkerThread()| which in turn tries to access the |mPRThread| member of this null pointer, which triggers a segmentation fault. I'll try to keep on searching, but any hint would be welcome :/

> it seems that my call to |mPromise->MaybeResolve(res)| has an issue. There are several issues here (e.g. if MaybeResolve somehow managed to succeed, you would definitely have noticed when you did the Release on the Promise), but they all come down to the same thing: a Promise is a single-threaded object, and you're only allowed to touch it from the thread it lives on. We should probably add some NS_ASSERT_OWNINGTHREAD to API entrypoints to make that clearer to people. Could you please file a followup bug on that? In any case, whatever code it is that wants to resolve the promise needs to post a runnable to that promise's home thread to do so.

(In reply to Not doing reviews right now from comment #29) > > it seems that my call to |mPromise->MaybeResolve(res)| has an issue. > > There are several issues here (e.g. if MaybeResolve somehow managed to > succeed, you would definitely have noticed when you did the Release on the > Promise), but they all come down to the same thing: a Promise is a > single-threaded object, and you're only allowed to touch it from the thread > it lives on. We should probably add some NS_ASSERT_OWNINGTHREAD to API > entrypoints to make that clearer to people. Could you please file a > followup bug on that? I've filed 1146418 for that. > In any case, whatever code it is that wants to resolve the promise needs to > post a runnable to that promise's home thread to do so. qeole - I could see the basic issue here from the stack trace and bz explained why you can't use Promises like this. Hopefully this should help get you past this issue, let me know if I can help further.

Thanks bz and bobowen for the hint about thread issues. I've been trying to use a runnable to resolve the promise from main thread (where it is created), but I couldn't make it work. Enclosed (non functional) patch contains one of my attempts: I added a ResolvePromiseTask runnable, but I believe that passing the reference to the promise from thread to thread makes it “thread-unsafe” (I take it from the following error message I get at runtime, when it segfaults: > Hit MOZ_CRASH(Promise not thread-safe) at /[…]/mozilla-central/dom/promise/Promise.cpp:320 ) bobowen, can I ask you for a little more guidance here, please? Could you have a look at this and tell me if I have the right approach at all with the new runnable? If so, how should I transmit the promise reference from the (Add|Remove)PluginDirectory function to the ResolvePromiseTask, without passing by the PathRunnable?

(In reply to Qeole [:qeole] from comment #31) > Created attachment 8583172 [details] [diff] [review] > gmp-setup-promise.patch v4 − not functional > > Thanks bz and bobowen for the hint about thread issues. > > I've been trying to use a runnable to resolve the promise from main thread > (where it is created), but I couldn't make it work. > > Enclosed (non functional) patch contains one of my attempts: I added a That patch only seems to be a rebase of V3, I can't see any actual changes? > ResolvePromiseTask runnable, but I believe that passing the reference to the > promise from thread to thread makes it “thread-unsafe” (I take it from the > following error message I get at runtime, when it segfaults: > > Hit MOZ_CRASH(Promise not thread-safe) at /[…]/mozilla-central/dom/promise/Promise.cpp:320 ) So, it looks like you're adding a reference on the wrong thread and that is what is crashing. The code, that I assume was used as an example, from comment 4, is using the Promise in a runnable, but it's all on the same thread.

Awwww sorry, wrong patch indeed. I made a |hg qfold| on my side, but it looks like I forgot one patch. I should definitely have checked that :(. I'm enclosing the version which does add a runnable for resolving the promise in main thread; but as stated in comment #31, there is still something I'm doing wrong.

Comment on attachment 8583492 [details] [diff] [review] gmp-setup-promise.patch v4[corrected] − not functional Review of attachment 8583492 [details] [diff] [review]: ----------------------------------------------------------------- I'm not an expert in GMP or threading issues, so I might lead you astray here. But maybe I can help and then someone who really knows what they're talking about can review. :-) (Note: it looks like this patch has just been bitrotted by one that deletes the old plugin from disk on removal, you'll probably have to convert that function as well.) Is it possible to create the resolving runnable on the main thread, which holds the promise and pass that to the GMPThread instead of the Promise? You would need to be able to set the result on it, before dispatching it back to the main thread. I'm not really sure if it is acceptable to pass an object holding a reference to the Promise, looks like it is from [1]. I've scoped this idea out below and added some general comments, hope they help. The only other alternative I can think of is holding the Promise on the main thread somehow. The GeckoMediaPluginSerivce appears to be a singleton, so I'm guessing we can't store it there or we'd have to store in some sort of map and pass the key. [1] https://hg.mozilla.org/mozilla-central/file/37d3dcbf23a9/dom/workers/ServiceWorkerClients.cpp#l44 ::: dom/media/gmp/GMPService.cpp @@ +39,5 @@ > +#include "mozilla/dom/Promise.h" > + > +using namespace mozilla::dom; > + > + nit: extra line @@ +682,2 @@ > { > + bool res; I think "result" would be better, same elsewhere. @@ +687,5 @@ > + res = mService->RemoveOnGMPThread(mPath); > + } > + > + nsCOMPtr<nsIRunnable> maybeResolvePromise = > + new ResolvePromiseTask(mPromise, res); This is where you're adding a reference to the Promise on the GMP thread, which causes your crash. Instead we'd just set our result on the passed callback and dispatch it. @@ +712,1 @@ > MOZ_ASSERT(NS_IsMainThread()); I think the MOZ_ASSERT should stay at the top of the function. @@ +715,5 @@ > + JS_ClearPendingException(aCx); > + return NS_ERROR_UNEXPECTED; > + } > + > + return GMPDispatch(new PathRunnable(this, aDirectory, promise, true)); So we create the ResolvePromiseTask/Callback here and pass instead of promise. @@ +734,5 @@ > + if (result.Failed()) { > + return result.ErrorCode(); > + } > + > + MOZ_ASSERT(NS_IsMainThread()); Same here, at the top. ::: dom/media/gmp/GMPService.h @@ +95,5 @@ > GMPParent* ClonePlugin(const GMPParent* aOriginal); > nsresult EnsurePluginsOnDiskScanned(); > nsresult InitStorage(); > > + class ResolvePromiseTask : public nsRunnable Maybe this should be called ResolvePromiseCallback, if the idea of constructing this on main thread and passing to GMP thread works. @@ +98,5 @@ > > + class ResolvePromiseTask : public nsRunnable > + { > + public: > + ResolvePromiseTask(const nsRefPtr<Promise>& promise, bool res) The coding style guidelines say arguments should be prefixed with "a". So, aPromise. @@ +102,5 @@ > + ResolvePromiseTask(const nsRefPtr<Promise>& promise, bool res) > + : mPromise(promise) > + , mRes(res) > + { > + MOZ_ASSERT(!NS_IsMainThread()); This would need to be the opposite and we would need a function to set the result. @@ +128,5 @@ > > private: > nsRefPtr<GeckoMediaPluginService> mService; > nsString mPath; > + nsRefPtr<Promise> mPromise; We'd hold the callback here instead.

Yoric mentioned nsMainThreadPtrHandle<>, would that help with the cross-thread referencing issues?

(In reply to Georg Fritzsche [:gfritzsche] from comment #35) > Yoric mentioned nsMainThreadPtrHandle<>, would that help with the > cross-thread referencing issues? Ah, yes, looks like you might just be able to replace the appropriate nsRefPtrs with those.

So here is another WIP patch. The |nsMainThreadPtrHandle<Promise>| made me able to hold the reference to the promise; everything compiles, Fx does not crash anymore, and the patch has been rebased on yesterday's m-c build. I still have to 1) test it: I intended to use my patch for bug 1046052 to do this, but it turns out that patch may have an issue with the addPluginDirectory() call (I left a note to Georg about that on IRC, I'll investigate further). So I'm still trying to figure out an easy way to test the promise here on a successful addPluginDirectory() call. I managed to check that |yield addPluginDirectory()| does return a bool as expected, though. 2) factor the (Add|Remove|RemoveAndDelete)PluginDirectory() functions; but I'd like to first check that current code actually works.

According to cpearce in comment 38 of bug 1046052: > we fire the observer service notification "gmp-path-added" when > mozIGeckoMediaPluginService.AddPluginDirectory() has succeeded. In addition to this, we may not even have to call addPluginDirectory() at all in Mochitest of same bug. So is adding the promise still relevant here? If it is, could someone help me to find a way to test current code (it compiles and runs, but I don't know how to check that the promise is actually working). Flagging Georg, but there is no emergency here since it does not seem to block bug 1046052 anymore.

If bug 1046052 works out with that now, we can at least not block it on this bug. We may still want to leave it open because an observer doesn't allow us to find out when an individual addPluginDirectory call succeeded, which would actually be useful for the addon manager part that uses it.

Same as for bug 1046052: > I've been unable to work on this bug lately, plus attention for this bug seemsto have dropped for > now. > > I'm unassigning, but you may ping me if it comes relevant again (and if someone has some time to > mentor me about it).

Mass change P3->P4 to align with new Mozilla triage process.