Closed Bug 1253479 Opened 6 years ago Closed 6 years ago

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

Categories

(Core :: DOM: Events, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla48
Tracking Status
e10s + ---
firefox47 --- affected
firefox48 --- fixed

People

(Reporter: adw, Assigned: adw)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(1 file, 1 obsolete file)

Attached patch patch (obsolete) — Splinter Review
[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.
Attachment #8726508 - Flags: review?(masayuki)
Whiteboard: btpp-active
(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.
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?
Flags: needinfo?(masayuki)
(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...
Flags: needinfo?(masayuki)
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.
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?
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.
But nsPrefBranch::SetBoolPref throws if it's called from the child...
(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...
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.
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
Attached patch patch v2Splinter Review
Yeah, that did it.
Attachment #8726508 - Attachment is obsolete: true
Attachment #8726508 - Flags: review?(masayuki)
Attachment #8728736 - Flags: review?(masayuki)
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)
Attachment #8728736 - Flags: review?(masayuki) → review+
See Also: → 1255649
Thanks Masayuki.  I filed bug 1255649 for SpecialPowers.
https://hg.mozilla.org/mozilla-central/rev/a73b90e53f9c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.