1255919 - test_bug629172.html fails on e10s

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[(no longer active)]

This test uses synthesizeKey() to invoke <https://dxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#443> and that seems to be doing nothing in e10s.  Neil, is that expected?

Comment 1

[Neil Deakin]

I would guess because synthesizeKey is being called in the child process so the parent never sees the key events.

Maybe we want this to be browser test since it relies on browser UI?

Comment 2

[(no longer active)]

Attachment: Convert the test for bug 629172 into a browser-chrome test that is enabled on e10s

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8730344 [details] [diff] [review]
Convert the test for bug 629172 into a browser-chrome test that is enabled on e10s

Review of attachment 8730344 [details] [diff] [review]:
-----------------------------------------------------------------

There are a couple of shorthands you can use if you want, but this looks great to me otherwise!

::: editor/libeditor/tests/browser_bug629172.js
@@ +9,5 @@
> +    yield ContentTask.spawn(aBrowser, {}, function*() {
> +      var window = content.window.wrappedJSObject;
> +      var document = window.document;
> +
> +      // Note: Using the with keyword, we would have been able to write this as:

with is the "make this run extremely slow" switch, it's been deprecated for as long as I've been at Mozilla and I've never met a JS engine developer who liked it :)

@@ +77,5 @@
> +      });
> +      waitForKeypress = BrowserTestUtils.waitForContentEvent(aBrowser, "keypress");
> +      EventUtils.synthesizeKey("x", {accelKey: true, shiftKey: true});
> +      yield waitForKeypress;
> +      yield ContentTask.spawn(aBrowser, {initialDir: initialDir}, function(data) {

Here and elsewhere: with es6 shorthand, you can just have { initialDir } which means the same thing with 100% less identifier duplication.

Also, with destructuring, you can avoid having to pass objects all the way through. I usually use a pattern like:

ContentTask.spawn(browser, { a, b, c }, ({ a, b, c }) => {
  is(content.blah, a, ...);
  is(content[b], c, ...);
});

Comment 4

[(no longer active)]

(In reply to Blake Kaplan (:mrbkap) (please use needinfo!) from comment #3)
> Comment on attachment 8730344 [details] [diff] [review]
> Convert the test for bug 629172 into a browser-chrome test that is enabled
> on e10s
> 
> Review of attachment 8730344 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> There are a couple of shorthands you can use if you want, but this looks
> great to me otherwise!
> 
> ::: editor/libeditor/tests/browser_bug629172.js
> @@ +9,5 @@
> > +    yield ContentTask.spawn(aBrowser, {}, function*() {
> > +      var window = content.window.wrappedJSObject;
> > +      var document = window.document;
> > +
> > +      // Note: Using the with keyword, we would have been able to write this as:
> 
> with is the "make this run extremely slow" switch, it's been deprecated for
> as long as I've been at Mozilla and I've never met a JS engine developer who
> liked it :)

I know!  But in this case it would have made the test *much* easier to read.

> @@ +77,5 @@
> > +      });
> > +      waitForKeypress = BrowserTestUtils.waitForContentEvent(aBrowser, "keypress");
> > +      EventUtils.synthesizeKey("x", {accelKey: true, shiftKey: true});
> > +      yield waitForKeypress;
> > +      yield ContentTask.spawn(aBrowser, {initialDir: initialDir}, function(data) {
> 
> Here and elsewhere: with es6 shorthand, you can just have { initialDir }
> which means the same thing with 100% less identifier duplication.
> 
> Also, with destructuring, you can avoid having to pass objects all the way
> through. I usually use a pattern like:
> 
> ContentTask.spawn(browser, { a, b, c }, ({ a, b, c }) => {
>   is(content.blah, a, ...);
>   is(content[b], c, ...);
> });

Very nice!

Comment 5

[Pulsebot]

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

Comment 6

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for browser-chrome failures in browser_bug629172.js

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/4dceeab3d7c5

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=cc8b02b24319
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=23708025&repo=mozilla-inbound
14:33:09     INFO -  206 INFO TEST-START | editor/libeditor/tests/browser_bug629172.js
14:33:54     INFO -  TEST-INFO | started process screenshot
14:33:54     INFO -  TEST-INFO | screenshot: exit 0
14:33:54     INFO -  207 INFO *** Start BrowserChrome Test Results ***
14:33:54     INFO -  208 INFO checking window state
14:33:54     INFO -  209 INFO Entering test bound
14:33:54     INFO -  210 INFO Console message: [JavaScript Error: "The character encoding of the HTML document was not declared. The document will render with garbled text in some browser configurations if the document contains characters from outside the US-ASCII range. The character encoding of the page must be declared in the document or in the transfer protocol." {file: "http://example.org/browser/editor/libeditor/tests/bug629172.html" line: 0}]
14:33:54     INFO -  211 INFO TEST-PASS | editor/libeditor/tests/browser_bug629172.js | Textarea should appear correctly before switching the direction (ltr) - true == true -
14:33:54     INFO -  212 INFO TEST-PASS | editor/libeditor/tests/browser_bug629172.js | input event count must be 0 before - 0 == 0 -
14:33:54     INFO -  213 INFO Console message: [JavaScript Warning: "Key event not available on some keyboard layouts: key="c" modifiers="accel,alt"" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 730}]
14:33:54     INFO -  214 INFO Console message: [JavaScript Warning: "Key event not available on some keyboard layouts: key="i" modifiers="accel,alt,shift"" {file: "chrome://mochikit/content/tests/SimpleTest/EventUtils.js" line: 730}]
14:33:54     INFO -  215 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/browser_bug629172.js | Test timed out -
14:33:54     INFO -  MEMORY STAT | vsize 645MB | vsizeMaxContiguous 697MB | residentFast 226MB | heapAllocated 64MB
14:33:54     INFO -  216 INFO TEST-OK | editor/libeditor/tests/browser_bug629172.js | took 45050ms
14:33:54     INFO -  Not taking screenshot here: see the one that was previously logged
14:33:54     INFO -  217 INFO TEST-UNEXPECTED-FAIL | editor/libeditor/tests/browser_bug629172.js | Found a tab after previous test timed out: http://example.org/browser/editor/libeditor/tests/bug629172.html -

Comment 7

[(no longer active)]

Hmm, so it looks like waitForChromeEvent only works for e10s, and waitForEvent only works for non-e10s.

What should I do now?

Comment 8

[(no longer active)]

I tried a million different things, and then noticed that tests are already using gMultiProcessBrowser (thanks to George), so let's just hack around this.

Comment 9

[(no longer active)]

Attachment: Convert the test for bug 629172 into a browser-chrome test that is enabled on e10s

Comment 10

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 8730799 [details] [diff] [review]
Convert the test for bug 629172 into a browser-chrome test that is enabled on e10s

It's possible too that waitForContentEvent is async (since it uses the message manager) and ordering is guaranteed on e10s but on non-e10s we fire the key event first giving us a race condition. This is a fine work-around though.

Comment 11

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/25bad67aed1c

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/25bad67aed1c