1146349 - Hook up native event synthesization for child processes

Status: RESOLVED FIXED

(Core :: DOM: Content Processes, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

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 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Patch

Comment 2

[Olli Pettay [:smaug][[e-mail]]]

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?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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

Comment 6

[Kartikaya Gupta (email:[e-mail])]

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

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 9

[David Anderson [:dvander] - inactive, e-mail if emergency]

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

Comment 10

[Kartikaya Gupta (email:[e-mail])]

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

Comment 11

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Add support for posting lambdas

Comment 13

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Make native event synth functions async with an observer

Comment 14

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Update widget/ tests to work with async native event synth

Comment 15

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Update dom/ mochitests to deal with async native event synth

Comment 16

[Kartikaya Gupta (email:[e-mail])]

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 17

[Kartikaya Gupta (email:[e-mail])]

Comment on attachment 8587308 [details] [diff] [review]
Part 1 - Add support for posting lambdas

Moved this to bug 1152753.

Comment 18

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Make native event synthesization functions async

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 19

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 2 - Update widget tests

Comment 20

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Update dom mochitests

Comment 21

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Update browser mochitests

Comment 22

[Kartikaya Gupta (email:[e-mail])]

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 23

[:Gijs (he/him)]

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.

Comment 24

[:Gijs (he/him)]

(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)

Comment 25

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 5 (WIP) - Wire up native event synth in child processes

Comment 26

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Make native event synthesization functions async (v2)

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

Comment 27

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 4 - Update browser mochitests (v2)

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 28

[Olli Pettay [:smaug][[e-mail]]]

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);

Comment 29

[Kartikaya Gupta (email:[e-mail])]

(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 30

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 31

[(no longer active)]

(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.

Comment 32

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) 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.

Comment 33

[Botond Ballo [:botond]]

(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.

Comment 34

[Botond Ballo [:botond]]

(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

Comment 35

[Olli Pettay [:smaug][[e-mail]]]

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

Comment 36

[Botond Ballo [:botond]]

(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.

Comment 37

[Olli Pettay [:smaug][[e-mail]]]

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 38

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 0 - Need more support from NS_NewRunnableMethodWithArgs

Comment 39

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Make native event synthesization functions async (v3)

Comment 40

[Olli Pettay [:smaug][[e-mail]]]

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 41

[David Anderson [:dvander] - inactive, e-mail if emergency]

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 42

[:Gijs (he/him)]

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 43

[Nathan Froyd [:froydnj]]

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 44

[Olli Pettay [:smaug][[e-mail]]]

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+

Comment 45

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 1 - Make native event synthesization functions async (v4)

Updated with smaug's comments, carrying r+

Comment 46

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 5 - Wire up native event synth in child processes

https://treeherder.mozilla.org/#/jobs?repo=try&revision=66817d7fc46e

Comment 47

[Kartikaya Gupta (email:[e-mail])]

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

Comment 48

[Olli Pettay [:smaug][[e-mail]]]

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 49

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 50

[Kartikaya Gupta (email:[e-mail])]

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

Comment 51

[Ryan VanderMeulen [:RyanVM]]

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

Comment 52

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a3be7dbf862b