1253479 - [e10s] Make widget/tests/test_assign_event_data.html work under e10s

Status: RESOLVED FIXED

(Core :: DOM: Events, defect)

Description

[Drew Willcoxon :adw]

Attachment: patch

[e10s] Make widget/tests/test_assign_event_data.html work under e10s.

Masayuki, would you mind reviewing?  It looks like composition events must happen in the parent, but I don't understand why.  Anyway, this patch allows this test to work under e10s for me locally, but I don't know if it's the right way to fix this.

Comment 1

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4165ec791863

Comment 2

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

(In reply to Drew Willcoxon :adw from comment #0)
> Created attachment 8726508 [details] [diff] [review]
> patch
> 
> [e10s] Make widget/tests/test_assign_event_data.html work under e10s.
> 
> Masayuki, would you mind reviewing?  It looks like composition events must
> happen in the parent, but I don't understand why.  Anyway, this patch allows
> this test to work under e10s for me locally, but I don't know if it's the
> right way to fix this.

How do the tests fail? Indeed, mozilla::TextComposition class which manages active composition is created both in the chrome process and the content process. However, for this case, only created in the content process should work. It seems that the failure *might* detect an actual bug.

Comment 3

[Drew Willcoxon :adw]

The test hangs after it starts the "WidgetKeyboardEvent (keyup during composition)" subtest.  The subtest is listening for keyup, but onEvent and gCallback are never called.  At that point the input has focus and it contains ね.  The subtest starts by calling synthesizeCompositionChange with a string of ね, which is correctly shown in the <input>.  Then it calls synthesizeComposition({ type: "compositioncommitasis" }), and unfortunately I don't know enough about composition events to say what happens then.  The ね is not underlined, which I think means the composition was successfully committed?

Do you have any ideas?

Comment 4

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

(In reply to Drew Willcoxon :adw from comment #3)
> The test hangs after it starts the "WidgetKeyboardEvent (keyup during
> composition)" subtest.  The subtest is listening for keyup, but onEvent and
> gCallback are never called.  At that point the input has focus and it
> contains ね.  The subtest starts by calling synthesizeCompositionChange with
> a string of ね, which is correctly shown in the <input>.  Then it calls
> synthesizeComposition({ type: "compositioncommitasis" }), and unfortunately
> I don't know enough about composition events to say what happens then.  The
> ね is not underlined, which I think means the composition was successfully
> committed?

Yes. That's the expected result. It means that <input> element receives "compositionstart", "compositionupdate" (, "text") and "compositionend" events correctly. Before "compositionend", "keyup" for the 'A' key should be fired... I.e.,

1. compositionstart
2. keydown
3. compositionupdate
4. text
5. keyup
6. compositionend

These events should be fired in this order...

When you call synthesizeCompositionChangeAndComposition() with a key, following methods are called:

TextInputProcessor::SetPendingCompositionString()
  TextEventDispatcher::PendingComposition::SetString()
TextInputProcessor::AppendClauseToPendingComposition()
  TextEventDispatcher::PendingComposition::AppendClause()
TextInputProcessor::SetCaretInPendingComposition()
  TextEventDispatcher::PendingComposition::SetCaret()
TextInputProcessor::FlushPendingComposition()
  TextInputProcessor::MaybeDispatchKeydownForComposition()
    TextInputProcessor::KeydownInternal()
      TextEventDispatcher::DispatchKeyboardEvent()
        TextEventDispatcher::DispatchKeyboardEventInternal()
  TextEventDispatcher::PendingComposition::Flush()
  TextInputProcessor::MaybeDispatchKeyupForComposition()
    TextInputProcessor::KeydownInternal()
      TextEventDispatcher::DispatchKeyboardEvent()
        TextEventDispatcher::DispatchKeyboardEventInternal()

In TextEventDispatcher::DispatchKeyboardEventInternal(), "dom.keyboardevent.dispatch_during_composition" is checked (the value is stored into sDispatchKeyEventsDuringComposition with AddBoolVarCache()). So, I guess that the pref value isn't modified in e10s mode yet...

Comment 5

[Drew Willcoxon :adw]

Thanks Masayuki.  The test is using SpecialPowers.setBoolPref(), which actually works in e10s (by sending a message to chrome).  But in e10s only, something is resetting the HasUserValue() flag of dom.keyboardevent.dispatch_during_composition between the time the test sets it to true and the time that TextEventDispatcher gets it.  So PREF_GetBoolPref takes the "default" path and gets its default value instead of the user value.  That doesn't happen in non-e10s.

I'm still trying to figure out what's causing it, but it's not PREF_ClearUserPref, PREF_ClearAllUserPrefs, or pref_HashPref.

Comment 6

[Drew Willcoxon :adw]

Hmm, it's because there are two different values of gHashTable in prefapi.  The one used by the set() in the test is not the one used by the get() in TextEventDispatcher.  Is SpecialPowers really setting prefs in the parent?

Comment 7

[Drew Willcoxon :adw]

Oh... the pref is indeed set in the parent, but TextEventDispatcher's get() is called in the child.  My patch synthesized the events in the parent, which is why it worked I guess.

Comment 8

[Drew Willcoxon :adw]

But nsPrefBranch::SetBoolPref throws if it's called from the child...

Comment 9

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

(In reply to Drew Willcoxon :adw from comment #7)
> Oh... the pref is indeed set in the parent, but TextEventDispatcher's get()
> is called in the child.  My patch synthesized the events in the parent,
> which is why it worked I guess.

Impossible to change prefs of child process sounds bad because a lot of modules use mozilla::Preferences even in a child process. So, I think that it's a test framework's bug...

Comment 10

[Drew Willcoxon :adw]

But nsPrefBranch::SetBoolPref and Preferences::SetBool both ENSURE_MAIN_PROCESS() (i.e., return NS_ERROR_NOT_AVAILABLE) when called from the child, so it's not really a test framework bug.

Given that it's apparently impossible to set preferences in the child, it seems like there are only two ways to fix this: either have TextEventDispatcher get the pref from the parent, not the child; or have the preference(s) synced from the parent to the child.

Comment 11

[Drew Willcoxon :adw]

It looks like there is preference syncing from the parent to the child.  I don't know how to tap into it to tell when the child's preference store is updated, but when I add some setTimeouts to the test after it sets and clears the pref, it passes.  Maybe I can just add a preference observer in the child, in the test.

http://mxr.mozilla.org/mozilla-central/source/dom/ipc/PContent.ipdl#594
http://mxr.mozilla.org/mozilla-central/search?string=PreferenceUpdate
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentParent.cpp#3151
http://mxr.mozilla.org/mozilla-central/source/dom/ipc/ContentChild.cpp#2345

Comment 12

[Drew Willcoxon :adw]

Attachment: patch v2

Yeah, that did it.

Comment 13

[Drew Willcoxon :adw]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=677e49c1d144

Comment 14

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

Comment on attachment 8728736 [details] [diff] [review]
patch v2

Looks great! Thank you for your polite work!

*I* think that the feature should be in specialpowers API. Could you file a follow up bug? (I don't know who is managing specialpowers API, though)

Comment 15

[Drew Willcoxon :adw]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a73b90e53f9c

Comment 16

[Drew Willcoxon :adw]

Thanks Masayuki.  I filed bug 1255649 for SpecialPowers.

Comment 17

[Carsten Book [:Tomcat]]

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