1093153 - e10s - fix browser_aboutHome.js to work in e10s

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[:Gijs (out for now; he/him)]

Currently disabled, trying to enable it gets me:

108 INFO TEST-START | chrome://mochitests/content/browser/browser/base/content/test/general/browser_aboutHome.js
109 INFO Check that clearing cookies does not clear storage
110 INFO Waiting for snippets map
111 INFO Wait tab event: AboutHomeLoadSnippetsCompleted
112 INFO Console message: [JavaScript Error: "Error: Permission denied to access object" {file: "resource://gre/modules/RemoteAddonsParent.jsm" line: 484}]
113 INFO Tab event received: AboutHomeLoadSnippetsCompleted

after which the test hangs indefinitely.

Comment 1

[Mike de Boer [:mikedeboer]]

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

Comment 2

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1: introduce 'BrowserTestUtils#sendCompositionChange' and frame script fixes

Paolo, would you like to review this part?

Comment 3

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: enabled and re-factored browser_aboutHome.js

Marco, do you feel comfortable reviewing this?

Comment 4

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2: no-ws.

Comment 5

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ffc79c86614

Comment 6

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.1: introduce 'BrowserTestUtils#sendCompositionChange' and frame script fixes

s/meta/accel/

Comment 7

[:Paolo Amadini]

Comment on attachment 8725245 [details] [diff] [review]
Patch 1.1: introduce 'BrowserTestUtils#sendCompositionChange' and frame script fixes

Why not just call EventUtils.synthesizeComposition and friend inside a ContentTask?

Seems like we could save quite a few lines of code by doing this...

Comment 8

[Mike de Boer [:mikedeboer]]

(In reply to :Paolo Amadini from comment #7)
> Why not just call EventUtils.synthesizeComposition and friend inside a
> ContentTask?
> 
> Seems like we could save quite a few lines of code by doing this...

I was not aware we could...

Comment 9

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #8)
> I was not aware we could...

That was a bit sparse in info from me ;-) Please read bug 1241532, which explains why it's not supported and why it will not be. We had the same train of thought, surely.

Comment 10

[:Paolo Amadini]

I'll have to take some time to digest the reasoning in bug 1241532, because what I read in this patch looks exactly like calling EventUtils.synthesizeComposition in a frame script... whether it's done by a content task or specialized message passing should not make any difference on the order of the events.

Comment 11

[Marco Bonardo [:mak]]

Comment on attachment 8725219 [details] [diff] [review]
Patch 2: no-ws.

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

it's a lot of changes, and it's still a quite complex test (we should really split it) but overall the changes look sane.
Just please retrigger the appropriate b-c test run on Try at least 20 times, to ensure we're not introducing new intermittent failures, due to the size of changes it's hard to guarantee that just by looking at the patch.

::: browser/base/content/test/general/browser_aboutHome.js
@@ +6,3 @@
>  
>  XPCOMUtils.defineLazyModuleGetter(this, "Promise",
>    "resource://gre/modules/Promise.jsm");

nit: could probably remove this and just rely on DOM promises

@@ +36,3 @@
>      Cc["@mozilla.org/observer-service;1"]
>        .getService(Ci.nsIObserverService)
>        .notifyObservers(null, "cookie-changed", "cleared");

does this work? looks like you are always invoking the setupFn in the content scope, and I'm not sure you can notify like this from content (so maybe this ends up being a no-op)?

I actually wonder if the original test worked at all, since it's the first test, thus the map is likely non existent yet and the clearing is asynchronous so it may run after the test.
Maybe we should move the test to /dom/tests/mochitest/localstorage... I cannot even find anymore the code in DOM Storage that was responsible to not clearing about pages storage. So maybe it just broke, no idea.

@@ +529,5 @@
> +  });
> +});
> +
> +add_task(function* () {
> +  info("Pressing Space while the Addons button is focussed should activate it");

while here, typo "focussed"

@@ +535,2 @@
>      // Skip this test on Mac, because Space doesn't activate the button there.
> +  if (appInfo.OS == "Darwin") {

Why not using AppConstants.platform == 'macosx' and remove the appInfo getter?

@@ +561,5 @@
>   * @param aSetupFn
>   *        The setup function to be run.
>   * @return {Promise} resolved when the snippets are ready.  Gets the snippets map.
>   */
> +function* ensureSnippetsMapThen(setupFn, testFn, testArgs = null) {

The fact this is names exactly like the method in about:home is confusing and complicates searches through code... I'd rather name this differently, maybe withSnippetsMap?

Comment 12

[:Paolo Amadini]

Comment on attachment 8725245 [details] [diff] [review]
Patch 1.1: introduce 'BrowserTestUtils#sendCompositionChange' and frame script fixes

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

I see that EventUtils needs some special initialization, done in AsyncUtilsContent. Given that the scope of AsyncUtilsContent is currently separate from ContentTask, I don't see why for now we shouldn't just add another type of event using the same pattern, like this patch does. Since Neil wrote the module initially, I'm redirecting the review to him.

::: testing/mochitest/BrowserTestUtils/BrowserTestUtils.jsm
@@ +821,5 @@
>     * @returns {Promise}
>     * @resolves True if the keypress event was synthesized.
>     */
> +  sendChar(char, accelKey = false, browser) {
> +    // Support the two-argument style invocation.

I think it's just easier to overload the first argument to be either the character or an "options" object structured like { char, accelKey }. The second argument would always be the browser.

Comment 13

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=047042a32220

Comment 14

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.1: enabled and re-factored browser_aboutHome.js

Re-added ignoreAllUncaughtExceptions() and implemented review comments. Thanks Marco!

Carrying over r=mak.

Comment 15

[Mike de Boer [:mikedeboer]]

(In reply to Mike de Boer [:mikedeboer] from comment #13)
> https://treeherder.mozilla.org/#/jobs?repo=try&revision=047042a32220

Ugh, looks like this one's dirty due to other fx-team landings.

Comment 16

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4a1af328ca8b

Comment 17

[Mike de Boer [:mikedeboer]]

Marco, I see tests timing out with the following error:

JavaScript error: chrome://browser/content/abouthome/aboutHome.js, line 155: NotFoundError: The operation failed because the requested database object could not be found. For example, an object store did not exist but was being opened.


Have you seen this before?

https://hg.mozilla.org/integration/fx-team/file/tip/browser/base/content/abouthome/aboutHome.js#l155

Comment 18

[Mike de Boer [:mikedeboer]]

(Win7 OPT M-E10S)

Comment 19

[Marco Bonardo [:mak]]

not off-hand, it may happen if a schema upgrade is ongoing and something else tries to access the db...

Comment 20

[Mike de Boer [:mikedeboer]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b14d9ca7d66

Comment 21

[Mike de Boer [:mikedeboer]]

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

Comment 22

[Mike de Boer [:mikedeboer]]

OK, that looks nice and green.

Comment 23

[Mike de Boer [:mikedeboer]]

Comment on attachment 8725245 [details] [diff] [review]
Patch 1.1: introduce 'BrowserTestUtils#sendCompositionChange' and frame script fixes

Paolo, Neil is swamped atm. Are you sure you can't review this? Please?

Comment 24

[:Paolo Amadini]

For some reason I didn't receive the bugmail for comment 23. Sure, I can review this!

Comment 25

[Neil Deakin]

Comment on attachment 8725245 [details] [diff] [review]
Patch 1.1: introduce 'BrowserTestUtils#sendCompositionChange' and frame script fixes

This seems to be several unrelated changes in one patch so it is a bit hard to follow.

For the 'accelKey' change: sendChar is used only for sending printable characters. I would prefer that synthesizeKey was added to BrowserTestUtils (which is how it should have been done originally.)

I'd also prefer if the BrowserTestUtils version of synthesizeComposition just called the EventUtils one rather than having a different name and behaviour.

Comment 26

[:Paolo Amadini]

Comment on attachment 8725245 [details] [diff] [review]
Patch 1.1: introduce 'BrowserTestUtils#sendCompositionChange' and frame script fixes

Clearing the flag since Neil looked at this.

Comment 27

[Mike de Boer [:mikedeboer]]

Attachment: Patch 1.2: introduce 'BrowserTestUtils#synthesizeKey', 'BrowserTestUtils#synthesizeComposition', 'BrowserTestUtils#synthesizeCompositionChange' and frame script fixes

Comment 28

[Mike de Boer [:mikedeboer]]

Attachment: Patch 2.2: enabled and re-factored browser_aboutHome.js

EventUtils usage fixes, only API surface. Carrying over r=mak.

Comment 29

[Neil Deakin]

Comment on attachment 8729039 [details] [diff] [review]
Patch 1.2: introduce 'BrowserTestUtils#synthesizeKey', 'BrowserTestUtils#synthesizeComposition', 'BrowserTestUtils#synthesizeCompositionChange' and frame script fixes

You might want to add a followup to merge the mostly duplicated synthesizeXXX functions.

Comment 30

[Mike de Boer [:mikedeboer]]

(In reply to Neil Deakin from comment #29)
> You might want to add a followup to merge the mostly duplicated
> synthesizeXXX functions.

What do you mean with 'merge', exactly?

Comment 31

[Neil Deakin]

The synthesizeXXX functions are mostly copies of each other. They could be combined into a helper function.

Comment 32

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/afe0a444c1824bc921242c86de7af4d47405df8a
Bug 1093153: introduce 'BrowserTestUtils#synthesizeKey', 'BrowserTestUtils#synthesizeComposition' and 'BrowserTestUtils#synthesizeCompositionChange' to allow mochitests to remotely invoke EventUtils' text composition utilities. Changes to EventUtils include 1) removed dependency on the 'navigator' object when 'nsIXULRuntime' is available and 2) make '_getKeyboardEvent' more robust when used in frame scripts. r=Enn

https://hg.mozilla.org/integration/fx-team/rev/5fa79c7ea7ae43359998dfed5fc1109b7ccc758b
Bug 1093153: enabled and re-factored browser_aboutHome.js to work correctly in e10s mode. r=mak

Comment 33

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

Backed out for Mochitest 2 failures.

Backouts:
https://hg.mozilla.org/integration/fx-team/rev/2c45f42f52c2
https://hg.mozilla.org/integration/fx-team/rev/59d67fe6d470

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=fx-team&revision=5fa79c7ea7ae

07:54:24     INFO -  17 INFO TEST-START | dom/events/test/test_bug1096146.html
07:54:25     INFO -  ###################################### BrowserElementCopyPaste.js loaded
07:54:25     INFO -  ############################### browserElementPanningAPZDisabled.js loaded
07:54:25     INFO -  ############################### browserElementPanning.js loaded
07:54:25     INFO -  ######################## BrowserElementChildPreload.js loaded
07:54:25     INFO -  ######################## extensions.js loaded
07:54:25     INFO -  TEST-INFO | started process screenshot
07:54:25     INFO -  TEST-INFO | screenshot: exit 0
07:54:25     INFO -  18 INFO TEST-UNEXPECTED-FAIL | dom/events/test/test_bug1096146.html | uncaught exception - Error: Permission denied to access property "navigator" at http://mochi.test:8888/tests/SimpleTest/EventUtils.js:57
07:54:25     INFO -      simpletestOnerror@SimpleTest/SimpleTest.js:1543:11
07:54:25     INFO -  JavaScript error: http://mochi.test:8888/tests/SimpleTest/EventUtils.js, line 57: Error: Permission denied to access property "navigator"
07:59:50     INFO -  Not taking screenshot here: see the one that was previously logged
07:59:50     INFO -  19 INFO TEST-UNEXPECTED-FAIL | dom/events/test/test_bug1096146.html | Test timed out.
07:59:50     INFO -      reportError@SimpleTest/TestRunner.js:114:7
07:59:50     INFO -      TestRunner._checkForHangs@SimpleTest/TestRunner.js:134:7
07:59:50     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:59:50     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:59:50     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:59:50     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:59:50     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:59:50     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:59:50     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:59:50     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:59:50     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:59:50     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:59:50     INFO -      setTimeout handler*TestRunner._checkForHangs@SimpleTest/TestRunner.js:155:5
07:59:50     INFO -      TestRunner.runTests@SimpleTest/TestRunner.js:366:5
07:59:50     INFO -      RunSet.runtests@SimpleTest/setup.js:186:3
07:59:50     INFO -      RunSet.runall@SimpleTest/setup.js:165:5
07:59:50     INFO -      hookupTests@SimpleTest/setup.js:258:5
07:59:50     INFO -  parseTestManifest@http://mochi.test:8888/manifestLibrary.js:36:5
07:59:50     INFO -  getTestManifest/req.onload@http://mochi.test:8888/manifestLibrary.js:49:11
07:59:50     INFO -  EventHandlerNonNull*getTestManifest@http://mochi.test:8888/manifestLibrary.js:45:3
07:59:50     INFO -      hookup@SimpleTest/setup.js:238:5
07:59:50     INFO -  EventHandlerNonNull*@http://mochi.test:8888/tests?autorun=1&closeWhenDone=1&consoleLevel=INFO&hideResultsTable=1&manifestFile=tests.json&dumpOutputDirectory=c%3A%5Cdocume%7E1%5Ccltbld%7E1.t-x%5Clocals%7E1%5Ctemp:11:1

Would importing AppConstants and checking AppConstants.platform work?

Comment 34

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

The M(oth)s are also failing:
https://treeherder.mozilla.org/logviewer.html#?job_id=7991156&repo=fx-team
09:16:01     INFO -  84 INFO TEST-UNEXPECTED-FAIL | chrome://mochitests/content/a11y/accessible/tests/mochitest/events/test_attrs.html | uncaught exception - TypeError: aWindow is null at chrome://mochikit/content/tests/SimpleTest/EventUtils.js:57

Comment 35

[Mike de Boer [:mikedeboer]]

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

Comment 36

[Mike de Boer [:mikedeboer]]

Thanks for the backout Sebastian - I just pushed an updated EventUtils.js to try, which should fix the issues reported here. The change I wanted to introduce is in fact trying to rely on AppConstants.jsm when we can and fall back back to user agent sniffing when not.

Just like you mentioned in comment 33 ;-)

Comment 37

[Mike de Boer [:mikedeboer]]

https://hg.mozilla.org/integration/fx-team/rev/5e4d9f0ec8cffc596378d76de48d3523f4dbe9b6
Bug 1093153: introduce 'BrowserTestUtils#synthesizeKey', 'BrowserTestUtils#synthesizeComposition' and 'BrowserTestUtils#synthesizeCompositionChange' to allow mochitests to remotely invoke EventUtils' text composition utilities. Changes to EventUtils include 1) removed dependency on the 'navigator' object when 'nsIXULRuntime' is available and 2) make '_getKeyboardEvent' more robust when used in frame scripts. r=Enn

https://hg.mozilla.org/integration/fx-team/rev/1664c4c4fd7d43d7616208282405d8eeffddb573
Bug 1093153: enabled and re-factored browser_aboutHome.js to work correctly in e10s mode. r=mak

Comment 38

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5e4d9f0ec8cf
https://hg.mozilla.org/mozilla-central/rev/1664c4c4fd7d

Comment 39

[Mike de Boer [:mikedeboer]]

Comment on attachment 8729040 [details] [diff] [review]
Patch 2.2: enabled and re-factored browser_aboutHome.js

Approval Request Comment
[Feature/regressing bug #]: e10s
[User impact if declined]: None, since this is test-only. However, since there's a very small change in non-test code, I believe it should be on RelMan's radar.
[Describe test coverage new/current, TreeHerder]: landed on m-c, tests pass.
[Risks and why]: minor.
[String/UUID change made/needed]: n/a.

Comment 40

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8729040 [details] [diff] [review]
Patch 2.2: enabled and re-factored browser_aboutHome.js

Mostly test change, plus handling an exception in ship code, seems safe. Aurora47+

Comment 41

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7a9e58c8c8b8
https://hg.mozilla.org/releases/mozilla-aurora/rev/5781664ff8a9