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

RESOLVED FIXED in Firefox 48

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: adw, Assigned: adw)

Tracking

(Blocks: 1 bug)

Trunk
mozilla48
Points:
---

Firefox Tracking Flags

(e10s+, firefox47 affected, firefox48 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

3 years ago
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.
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.
(Assignee)

Comment 3

3 years ago
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)

Updated

3 years ago
tracking-e10s: --- → +
(Assignee)

Comment 5

3 years ago
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.
(Assignee)

Comment 6

3 years ago
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?
(Assignee)

Comment 7

3 years ago
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.
(Assignee)

Comment 8

3 years ago
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...
(Assignee)

Comment 10

3 years ago
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.
(Assignee)

Comment 11

3 years ago
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
(Assignee)

Comment 12

3 years ago
Created attachment 8728736 [details] [diff] [review]
patch v2

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

Updated

3 years ago
See Also: → bug 1255649
(Assignee)

Comment 16

3 years ago
Thanks Masayuki.  I filed bug 1255649 for SpecialPowers.

Comment 17

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a73b90e53f9c
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
status-firefox48: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla48
You need to log in before you can comment on or make changes to this bug.