As part of bug 1139155 I wrote a patch to make DOMWindowUtils.sendNativeTouchPoint work in child processes. This allows us to write mochitests (for example) that inject touch events as deep as we can into the gecko flow, and test APZ code with "real" touch events. However I've been having trouble getting the test passing on try, so I'm splitting out the event synthesization bit and landing it so that (a) it doesn't bitrot and (b) it can be used in other places if needed.

Comment on attachment 8581622 [details] [diff] [review] Patch Doesn't this lead DOMWindowUtils methods to be sync in some cases and async in other cases?

Going forward I would like to make these methods all async in DOMWindowUtils (note that sendNativeMouseScrollEvent is already defined to be async in nsDOMWindowUtils.idl). For most platforms we plan on having off-main-thread input to APZ (this is already the case on B2G) and so when these functions are called it will inject the synthesized input event into whatever that thread is and return immediately. The event will be processed asynchronously. But you're right that the change should be explicitly called out. I can write a patch to update the method documentations in nsIDOMWindowUtils.idl and also update any existing users of these function to deal with the async-ness.

Sure, everything probably should be async, but better then actually have everything async. Some implementations being sync and some async would be rather horrible behavior and guaranteed to cause random test failures. To guarantee asyncness it might be enough to use a runnable internally in DOMWindowUtils when the widget layer doesn't deal this stuff asynchronously.

Good point, I'll make sure they are all forced async.

I have patches to force all the native event synthesization to be async, and as expected they're causing some test failures [1]. The problem I'm having is that in order to make the tests pass I need to be able to detect when the event is done its dispatch, which is hard for the case of key events. I can't just listen for keyup events because in some cases the native events being generated don't actually generate keyup events. The most reliable option at the moment seems to be to associate a guid with each synthesized event and then dispatch another event/notification with that guid to indicate completion. Not sure if there are any better options. [1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d1c6eb56364

I ran into this problem for wheel events. "onwheel" ended up sufficing, but I was thinking we would need a general "this synthetic event has been dispatched" notification. For most tests we wouldn't even care about the guid since they run serially.

What I'm trying now is that the caller of nsIDOMWindowUtils can optionally pass in an nsIObserver implementation which will get called once the event is done dispatching. This gives us a little wiggle room if say we want to send multiple notifications for complex events, we can just notify the observer multiple times with different topics or whatever. Will probably have to use a guid anyway across the IPC boundary but it felt nicer not to expose the guid to JS.

That's a good idea, seems harder to mess up writing tests that way.

New try push with some updated patches: https://treeherder.mozilla.org/#/jobs?repo=try&revision=86c6c3a84606. Still working my way through test failures.

(This comment is a note-to-future-self) Latest try pushes at: https://treeherder.mozilla.org/#/jobs?repo=try&revision=808238ee9c0e https://treeherder.mozilla.org/#/jobs?repo=try&revision=fd93f7794acf The first one of these uses my original approach of making the native event synthesization async, which was to run the actual event synth code inside a NS_DispatchToMainThread runnable. However, that caused browser_newtab_drag_drop.js to permafail on OS X. After much investigation (the results of which are described in bug 1150452) I came to the conclusion that we can't use NS_DispatchToMainThread if we want to have browser_newtab_drag_drop.js have any hope of passing without completely rewriting it. Instead I changed my patches to use MessageLoop::current()->PostTask which is different in that it posts things to the inner event loop rather than the outermost event loop. With this approach, browser_newtab_drag_drop.js sometimes passes, but the intermittent failure rate is significantly higher than it was before. Again, bug 1150452 explains why.

Success! https://treeherder.mozilla.org/#/jobs?repo=try&revision=d0535941d821 I'm going to clean up these patches and get them posted for review.

Comment on attachment 8587308 [details] [diff] [review] Part 1 - Add support for posting lambdas Moved this to bug 1152753.

The PuppetWidget bits will be fixed up properly in a future patch; it's just that bug 1085567 landed recently and I had to rebase around it.

Comment on attachment 8581622 [details] [diff] [review] Patch I'm finishing up the patch to make this work in content processes, will post that once it's done as well.

Comment on attachment 8590239 [details] [diff] [review] Part 4 - Update browser mochitests Review of attachment 8590239 [details] [diff] [review]: ----------------------------------------------------------------- Can you use promises instead? Make the synthesization code create a promise resolved by the observer notification, and use: synthesizeNative...(x).then(aCallback, Cu.reportError); at the callsites.

(In reply to :Gijs Kruitbosch from comment #23) > Comment on attachment 8590239 [details] [diff] [review] > Part 4 - Update browser mochitests > > Review of attachment 8590239 [details] [diff] [review]: > ----------------------------------------------------------------- > > Can you use promises instead? Make the synthesization code create a promise > resolved by the observer notification, and use: > > synthesizeNative...(x).then(aCallback, Cu.reportError); > > at the callsites. (which actually could probably do with being returned (.then returns another (resolved) promise, IIRC) so that the actual testcases can be updated to use promises some other time)

I switched to using NS_DispatchToMainThread and things work fine. Green try push is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=9a6cf1852fff

My first time using Promise stuff in JS, so please take a close look and let me know what needs to be fixed up.

Comment on attachment 8590801 [details] [diff] [review] Part 1 - Make native event synthesization functions async (v2) I don't understand the semantics of NS_NewRunnableFunction. Where is it defined? Which all variables does it, or the lambda, keep alive and how long? Looks rather security hazard if one happens to use stack-only variables - and I'd prefer to not start using such security risky features more often if possible. But maybe I just don't understand how "NS_NewRunnableFunction([=]() {" actually works. Why not use NS_NewRunnableMethodWithArgs? Something like NS_DispatchToMainThread(NS_NewRunnableMethodWithArgs<nsIntPoint, int32_t, nsIObserver>(widget, &nsIWidget::SynthesizeNativeTouchTap, nsIntPoint(aScreenX, aScreenY), aLongTap), aObserver);

(In reply to Olli Pettay [:smaug] from comment #28) > Comment on attachment 8590801 [details] [diff] [review] > Part 1 - Make native event synthesization functions async (v2) > > I don't understand the semantics of NS_NewRunnableFunction. Where is it > defined? Sorry, I just added landed it on inbound, in bug 1152753 which I ni?d you on yesterday. You can see it at https://hg.mozilla.org/integration/mozilla-inbound/rev/35cae02f1552. > Which all variables does it, or the lambda, keep alive and how long? The lambda makes copies of variables that are not references, and discards them after running, so it basically behaves like you would expect, and pretty much equivalent to what NS_NewRunnableMethodWithArgs does, except it's more readable. You may find [1] to be useful in understanding how it works - I did. [1] http://en.cppreference.com/w/cpp/language/lambda > Looks rather security hazard if one happens to use stack-only variables - > and I'd prefer to not start using such security risky features more often if > possible. But maybe I just don't understand how > "NS_NewRunnableFunction([=]() {" actually works. > > > Why not use NS_NewRunnableMethodWithArgs? Something like > NS_DispatchToMainThread(NS_NewRunnableMethodWithArgs<nsIntPoint, int32_t, > nsIObserver>(widget, &nsIWidget::SynthesizeNativeTouchTap, > nsIntPoint(aScreenX, aScreenY), aLongTap), aObserver); If you really want me to do this I probably can, I just find a lot less readable because of all the boilerplate.

Comment on attachment 8590801 [details] [diff] [review] Part 1 - Make native event synthesization functions async (v2) >+ nsCOMPtr<nsIObserver> observer(aObserver); >+ NS_DispatchToMainThread(NS_NewRunnableFunction([=]() { >+ widget->SynthesizeNativeKeyEvent(aNativeKeyboardLayout, aNativeKeyCode, >+ aModifiers, characters, unmodifiedCharacters, observer); The requirement to pass variable of type nsCOM/RefPtr is clearly a rather horrible security hazard. Why wouldn't one expect that passing aObserver is fine? Also, just passing aObserver would work most cases, but then if something happens during event loop spinning which kills the object, you have UAF, sec-critical bug. So, no, I don't like this setup.

(In reply to Olli Pettay [:smaug] from comment #30) > Comment on attachment 8590801 [details] [diff] [review] > Part 1 - Make native event synthesization functions async (v2) > > >+ nsCOMPtr<nsIObserver> observer(aObserver); > >+ NS_DispatchToMainThread(NS_NewRunnableFunction([=]() { > >+ widget->SynthesizeNativeKeyEvent(aNativeKeyboardLayout, aNativeKeyCode, > >+ aModifiers, characters, unmodifiedCharacters, observer); > The requirement to pass variable of type nsCOM/RefPtr is clearly a rather > horrible security hazard. > Why wouldn't one expect that passing aObserver is fine? > Also, just passing aObserver would work most cases, but then if something > happens during event loop spinning > which kills the object, you have UAF, sec-critical bug. FWIW the real offender here is the closure created by the lambda accessing the refcounted object. Without the nsCOMPtr, that's equivalent to having a class like: struct Closure { nsIObserve* mObserver; // raw ptr, dangerous }; which we end up passing to the callback.

(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #29) > > Which all variables does it, or the lambda, keep alive and how long? > > The lambda makes copies of variables that are not references If you specify '=' in the capture list, the lambda makes copies are variables that are references, too.

(In reply to Olli Pettay [:smaug] from comment #30) > Comment on attachment 8590801 [details] [diff] [review] > Part 1 - Make native event synthesization functions async (v2) > > >+ nsCOMPtr<nsIObserver> observer(aObserver); > >+ NS_DispatchToMainThread(NS_NewRunnableFunction([=]() { > >+ widget->SynthesizeNativeKeyEvent(aNativeKeyboardLayout, aNativeKeyCode, > >+ aModifiers, characters, unmodifiedCharacters, observer); > The requirement to pass variable of type nsCOM/RefPtr is clearly a rather > horrible security hazard. > Why wouldn't one expect that passing aObserver is fine? Because it's a raw pointer, and you're storing it in an object that outlives this function call. > Also, just passing aObserver would work most cases, but then if something > happens during event loop spinning > which kills the object, you have UAF, sec-critical bug. > > So, no, I don't like this setup. Some projects that have adopted C++11 lambdas have also adopted a policy that capture-defaults cannot be used; instead, all captured variables must be listed explicitly in the capture-list. This forces the code author/reviewer to think about what happens to each of those variables. If this helps address address your concerns, we could do that here, and consider proposing it as a broader guideline.

(In reply to Olli Pettay [:smaug] from comment #28) > Why not use NS_NewRunnableMethodWithArgs? Something like > NS_DispatchToMainThread(NS_NewRunnableMethodWithArgs<nsIntPoint, int32_t, > nsIObserver>(widget, &nsIWidget::SynthesizeNativeTouchTap, > nsIntPoint(aScreenX, aScreenY), aLongTap), aObserver); IIUC, the only reason this use of |aObserver| doesn't have the same problem as using it with the lambda would, is that NS_NewRunnableMethodWithArgs has special support for storing pointers to nsISupports-derived objects by nsRefPtr [1]. That seems fragile, too - what if my class is reference-counted by doesn't inherit from nsISupports? [1] http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsThreadUtils.h?rev=07234a94ed48#541

That is true. We should fix it to do the right thing with any refcounted objects (some template magic to detect AddRef/Release methods?).

(In reply to Olli Pettay [:smaug] from comment #35) > That is true. We should fix it to do the right thing with any refcounted > objects > (some template magic to detect AddRef/Release methods?). We could improve it, yes. My point, though, is that I don't think it will ever become a substitute for the caller *thinking* about the lifetime of their objects.

Filed Bug 1153295. We have had so many sec-critical bugs because of use of Foo* as a member variable (which lambdas effectively may do), that we should try to avoid use of code patterns which hide that as effectively as what lambdas do. I'm not saying NS_NewRunnableMethodWithArgs is perfect either, but once it deals with all the refcounted objects properly, it seems safe-enough to me. If your object isn't refcounted, you anyway need to think about its lifetime more carefully.

Comment on attachment 8590236 [details] [diff] [review] Part 2 - Update widget tests Oh, surprising, yield doesn't actually make this code any harder to understand :) Masayuki is about to change some of the synthesize*Key functions. (whoever lands the patches later will need to do some merging) Mostly rs+

Comment on attachment 8590801 [details] [diff] [review] Part 1 - Make native event synthesization functions async (v2) Review of attachment 8590801 [details] [diff] [review]: ----------------------------------------------------------------- This seems like a good approach to me.

Comment on attachment 8590802 [details] [diff] [review] Part 4 - Update browser mochitests (v2) Review of attachment 8590802 [details] [diff] [review]: ----------------------------------------------------------------- LGTM. Looks like returning a promise from the drag operation is not trivial (at least in the Linux case), so let's leave that for now.

Comment on attachment 8590935 [details] [diff] [review] Part 0 - Need more support from NS_NewRunnableMethodWithArgs Review of attachment 8590935 [details] [diff] [review]: ----------------------------------------------------------------- Ow, my eyes!

Comment on attachment 8590937 [details] [diff] [review] Part 1 - Make native event synthesization functions async (v3) >+ NS_DispatchToMainThread(NS_NewRunnableMethodWithArgs >+ <int32_t, int32_t, uint32_t, nsString, nsString, nsCOMPtr<nsIObserver>> nsIObserver* should be fine, no? Same also elsewhere >- virtual nsresult SynthesizeNativeMouseMove(mozilla::LayoutDeviceIntPoint aPoint) override; >+ virtual nsresult SynthesizeNativeMouseMove(mozilla::LayoutDeviceIntPoint aPoint, >+ nsCOMPtr<nsIObserver> aObserver) override; Passing nsCOMPtr<nsIFoo> as value? You want nsIObserver* aObserver. Same also with other similar methods >+struct AutoObserverNotifier { >+ AutoObserverNotifier(const nsCOMPtr<nsIObserver> aObserver, and here > public: > LongTapInfo(int32_t aPointerId, nsIntPoint& aPoint, >- mozilla::TimeDuration aDuration) : >+ mozilla::TimeDuration aDuration, >+ const nsCOMPtr<nsIObserver>& aObserver) : and this all those fixed, r+

Updated with smaug's comments, carrying r+

I also verified that I didn't regress bug 1085567 with part 5.

Comment on attachment 8591011 [details] [diff] [review] Part 5 - Wire up native event synth in child processes >+child: >+ NativeSynthesisResponse(uint64_t aObserverId, nsString aResponse); Why not nsCString here? That way you wouldn't need to convert to UTF16 on parent side and back to UTF8 on child. >+ NS_IMETHODIMP Observe(nsISupports *aSubject, >+ const char *aTopic, >+ const char16_t *aData) * goes with the type, not with the argument name. So, nsISupports* aSubject etc. >+private: >+ virtual ~SynthesizedEventObserver() { } >+ >+ TabParent* mTabParent; For safety, I would use nsRefPtr here - especially since this isn't hot code. >+ AutoSynthesizedEventResponder responder(this, aObserverId, "keyevent"); >+ nsCOMPtr<nsIWidget> widget = GetWidget(); >+ if (widget) { >+ widget->SynthesizeNativeKeyEvent(aNativeKeyboardLayout, aNativeKeyCode, >+ aModifierFlags, aCharacters, aUnmodifiedCharacters, >+ responder.GetObserver()); odd indentation. 2 spaces please, or align params under aNativeKeyboardLayout >+TabParent::RecvSynthesizeNativeMouseEvent(const LayoutDeviceIntPoint& aPoint, >+ const uint32_t& aNativeMessage, >+ const uint32_t& aModifierFlags, >+ const uint64_t& aObserverId) >+{ >+ AutoSynthesizedEventResponder responder(this, aObserverId, "mouseevent"); >+ nsCOMPtr<nsIWidget> widget = GetWidget(); >+ if (widget) { >+ widget->SynthesizeNativeMouseEvent(aPoint, aNativeMessage, aModifierFlags, >+ responder.GetObserver()); ditto, and also elsewhere.

Comment on attachment 8590236 [details] [diff] [review] Part 2 - Update widget tests Sorry for the delay to review. > @@ -334,2909 +337,2936 @@ function runKeyEventTests() > // preferences widget to enable other keyboard layouts and select them from the > // input menu to see what keyboard events they generate. > // Note that it's possible to send bogus key events here, e.g. > // {keyCode:0, chars:"z", unmodifiedChars:"P"} --- sendNativeKeyEvent > // makes no attempt to verify that the keyCode matches the characters. So only > // test key event records that you saw Cocoa send. > > // Ctrl keys > - testKey({layout:KEYBOARD_LAYOUT_EN_US, keyCode:MAC_VK_ANSI_A, > + yield testKey({layout:KEYBOARD_LAYOUT_EN_US, keyCode:MAC_VK_ANSI_A, > modifiers:{ctrlKey:1}, chars:"\u0001", unmodifiedChars:"a"}, > nsIDOMKeyEvent.DOM_VK_A, "a", SHOULD_DELIVER_ALL, KeyboardEvent.DOM_KEY_LOCATION_STANDARD); nit: indent 6 spaces in the following lines because of you adding "yield ". Same as similar changes in this patch.

Addressed review comments in comment 48 and comment 49 and landed: remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/e4132183a4fa remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/30b7130a25e4 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/0490493b12e8 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/45168f9387c7 remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/024cf684488b remote: https://hg.mozilla.org/integration/mozilla-inbound/rev/371a7026a1df

https://hg.mozilla.org/mozilla-central/rev/e4132183a4fa https://hg.mozilla.org/mozilla-central/rev/30b7130a25e4 https://hg.mozilla.org/mozilla-central/rev/0490493b12e8 https://hg.mozilla.org/mozilla-central/rev/45168f9387c7 https://hg.mozilla.org/mozilla-central/rev/024cf684488b https://hg.mozilla.org/mozilla-central/rev/371a7026a1df